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.
I will investigate this.
> > > 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().
fwd_config_setup() should be moved out of fwd_config_display() .
The display should not setup the config again.
> > 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.
This is probably the best way to go.
> > 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.
I have changed my mind about option 2, this is doing a rename to clarify what
is happening.
fwd_config_display() is renamed to fwd_config_setup_display(), it does not
separate the setup from the display.
> > > 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
