[dpdk-dev] [PATCH v2 4/8] app/testpmd: reconfigure forwarding after changing portlist

[De Lara Guarch, Pablo]

Hi Bernard,

> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Thursday, May 05, 2016 4:40 PM
> To: dev at dpdk.org
> Cc: De Lara Guarch, Pablo; Iremonger, Bernard
> Subject: [PATCH v2 4/8] app/testpmd: reconfigure forwarding after changing
> portlist
> 
> Set nb_fwd_ports to zero on quit.
> Check portlist has been set before displaying forwarding configuration.
> 
> Fixes: d3a274ce9dee ("app/testpmd: handle SIGINT and SIGTERM")
> Fixes: af75078fece3 ("first public release")

This patch is not fixing any issue, right? You are trying to improve the behaviour when changing portlist.
Therefore, you don't need to use Fixes tag.

> 
> Signed-off-by: Bernard Iremonger <bernard.iremonger at intel.com>
> ---
>  app/test-pmd/config.c  | 8 ++++++--
>  app/test-pmd/testpmd.c | 1 +
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index f434999..10ac768 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1424,8 +1424,10 @@ pkt_fwd_config_display(struct fwd_config *cfg)
>  void
>  fwd_config_display(void)
>  {
> -	fwd_config_setup();
> -	pkt_fwd_config_display(&cur_fwd_config);
> +	if (cur_fwd_config.nb_fwd_ports)
> +		pkt_fwd_config_display(&cur_fwd_config);
> +	else
> +		printf("Please set portlist first\n");
>  }

The problem of doing this is that if user starts testpmd, it is not possible to show
the configuration of the ports directly, since fwd_config_setup() has not being called
(because set_fwd_ports_list() has not being called), so it looks like portlist must be set,
but if user starts forwarding directly, then it is not necessary.
What I mean, is that by default, portlist should be all the ports.
Maybe we need to call fwd_config_setup after all the testpmd initialization.

> 
>  int
> @@ -1529,6 +1531,8 @@ set_fwd_ports_list(unsigned int *portlist, unsigned
> int nb_pt)
>  		       (unsigned int) nb_fwd_ports, nb_pt);
>  		nb_fwd_ports = (portid_t) nb_pt;
>  	}
> +
> +	fwd_config_setup();
>  }

I understand what you are doing here, but there is a problem. If you use --portmask parameter,
this function gets called when the arguments are parsed, but at that point, the ports are not configured yet,
and you get the following:

Fail: nb_rxq(1) is greater than max_rx_queues(0)
Program received signal SIGSEGV, Segmentation fault.
0x00000000004835c9 in setup_fwd_config_of_each_lcore (cfg=0xca4160 <cur_fwd_config>) at /tmp/dpdk-latest/app/test-pmd/config.c:1073

Anyway, I like the idea of moving fwd_config_setup out of fwd_config_display().
The problem is that there are other functions that should call this, such as set_fwd_lcores_list
(so, with this patch, if coremask is changed and then we call "show config fwd", we will not see any change).
Basically, all that affects the forwarding configuration should reconfigure it.
That's why I think it was decided to reconfigure the configuration when starting the forwarding or when showing the configuration.

So, we have two options:
1 - We add fwd_config_setup() in all the functions that are changing the configurations.
2 - We leave it as it was, especially with this patch, it makes more sense: http://dpdk.org/dev/patchwork/patch/13132/


> 
>  void
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 11b4cf7..2c58075 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -1560,6 +1560,7 @@ pmd_test_exit(void)
> 
>  	if (ports != NULL) {
>  		no_link_check = 1;
> +		nb_fwd_ports = 0;

Is this really necessary? I have removed it and I can quit testpmd with no problem.

>  		FOREACH_PORT(pt_id, ports) {
>  			printf("\nShutting down port %d...\n", pt_id);
>  			fflush(stdout);
> --
> 2.6.3