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

[Iremonger, Bernard]

Hi Pablo,

<snip>

> > 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.

Ok, fixes tag is not necessary here.

> > 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/

Option 2 looks like the best choice here, to drop this patch in favour of patch
http://dpdk.org/dev/patchwork/patch/13132/
which is already acked.

> >  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.

Ok, was just clearing this on exit as it had been set previously.

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