[dpdk-dev,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")

Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 app/test-pmd/config.c  | 8 ++++++--
 app/test-pmd/testpmd.c | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)
  

Hi Bernard,

> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Thursday, May 05, 2016 4:40 PM
> To: dev@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@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
  

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

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