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
