From: "Iremonger, Bernard" <bernard.iremonger@intel.com>
To: "De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2 4/8] app/testpmd: reconfigure forwarding after changing portlist
Date: Fri, 10 Jun 2016 15:58:00 +0000 [thread overview]
Message-ID: <8CEF83825BEC744B83065625E567D7C21A038267@IRSMSX108.ger.corp.intel.com> (raw)
In-Reply-To: <8CEF83825BEC744B83065625E567D7C21A037FA5@IRSMSX108.ger.corp.intel.com>
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
next prev parent reply other threads:[~2016-06-10 15:58 UTC|newest]
Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-20 12:52 [dpdk-dev] [PATCH 0/4] testpmd forwarding Bernard Iremonger
2016-04-20 12:52 ` [dpdk-dev] [PATCH 1/4] testpmd: add function port_is_forwarding Bernard Iremonger
2016-04-20 12:52 ` [dpdk-dev] [PATCH 2/4] testpmd: don't update fwding config when attaching/detaching a port Bernard Iremonger
2016-04-20 12:52 ` [dpdk-dev] [PATCH 3/4] testpmd: check port is not forwarding in stop_port and close_port Bernard Iremonger
2016-04-20 12:52 ` [dpdk-dev] [PATCH 4/4] testpmd: reconfigure forwarding after changing portlist Bernard Iremonger
2016-05-05 15:39 ` [dpdk-dev] [PATCH v2 0/8] app/testpmd: forwarding Bernard Iremonger
2016-05-05 15:39 ` [dpdk-dev] [PATCH v2 1/8] app/testpmd: add function port_is_forwarding Bernard Iremonger
2016-05-05 15:39 ` [dpdk-dev] [PATCH v2 2/8] app/testpmd: don't update fwding config when attaching/detaching a port Bernard Iremonger
2016-05-05 15:39 ` [dpdk-dev] [PATCH v2 3/8] app/testpmd: check port is not forwarding in stop_port and close_port Bernard Iremonger
2016-05-05 15:39 ` [dpdk-dev] [PATCH v2 4/8] app/testpmd: reconfigure forwarding after changing portlist Bernard Iremonger
2016-06-08 14:17 ` De Lara Guarch, Pablo
2016-06-10 9:22 ` Iremonger, Bernard
2016-06-10 15:58 ` Iremonger, Bernard [this message]
2016-05-05 15:39 ` [dpdk-dev] [PATCH v2 5/8] app/testpmd: add function port_is_bonding_slave Bernard Iremonger
2016-05-05 15:39 ` [dpdk-dev] [PATCH v2 6/8] app/testpmd: move call to init_fwd_streams Bernard Iremonger
2016-05-05 15:39 ` [dpdk-dev] [PATCH v2 7/8] app/testpmd: check for valid socket id when attaching port Bernard Iremonger
2016-05-05 15:39 ` [dpdk-dev] [PATCH v2 8/8] app/testpmd: check for valid mbuf pool Bernard Iremonger
2016-06-12 15:23 ` [dpdk-dev] [PATCH v3 0/9] app/testpmd: forwarding Bernard Iremonger
2016-06-12 15:23 ` [dpdk-dev] [PATCH v3 1/9] app/testpmd: add function port_is_forwarding Bernard Iremonger
2016-06-12 15:23 ` [dpdk-dev] [PATCH v3 2/9] app/testpmd: don't update fwding config when attaching/detaching a port Bernard Iremonger
2016-06-12 15:23 ` [dpdk-dev] [PATCH v3 3/9] app/testpmd: check port is not forwarding in stop_port and close_port Bernard Iremonger
2016-06-12 15:23 ` [dpdk-dev] [PATCH v3 4/9] app/testpmd: remove fwd_config_setup from fwd_config_display Bernard Iremonger
2016-06-13 15:57 ` De Lara Guarch, Pablo
2016-06-13 16:07 ` Iremonger, Bernard
2016-06-14 9:18 ` Iremonger, Bernard
2016-06-12 15:23 ` [dpdk-dev] [PATCH v3 5/9] app/testpmd: add function port_is_bonding_slave Bernard Iremonger
2016-06-12 15:23 ` [dpdk-dev] [PATCH v3 6/9] app/testpmd: move call to init_fwd_streams Bernard Iremonger
2016-06-12 15:23 ` [dpdk-dev] [PATCH v3 7/9] app/testpmd: check for valid socket id when attaching port Bernard Iremonger
2016-06-12 15:23 ` [dpdk-dev] [PATCH v3 8/9] app/testpmd: check for valid mbuf pool Bernard Iremonger
2016-06-12 15:23 ` [dpdk-dev] [PATCH v3 9/9] app/testpmd: stop forwarding on exit Bernard Iremonger
2016-06-14 12:28 ` [dpdk-dev] [PATCH v4 0/9] app/testpmd: forwarding Bernard Iremonger
2016-06-14 12:28 ` [dpdk-dev] [PATCH v4 1/9] app/testpmd: add function port_is_forwarding Bernard Iremonger
2016-06-14 12:28 ` [dpdk-dev] [PATCH v4 2/9] app/testpmd: don't update fwding config when attaching/detaching a port Bernard Iremonger
2016-06-14 12:28 ` [dpdk-dev] [PATCH v4 3/9] app/testpmd: check port is not forwarding in stop_port and close_port Bernard Iremonger
2016-06-14 12:28 ` [dpdk-dev] [PATCH v4 4/9] app/testpmd: remove fwd_config_setup from fwd_config_display Bernard Iremonger
2016-06-14 13:00 ` De Lara Guarch, Pablo
2016-06-14 14:14 ` Iremonger, Bernard
2016-06-14 14:30 ` De Lara Guarch, Pablo
2016-06-14 12:28 ` [dpdk-dev] [PATCH v4 5/9] app/testpmd: add function port_is_bonding_slave Bernard Iremonger
2016-06-14 12:28 ` [dpdk-dev] [PATCH v4 6/9] app/testpmd: move call to init_fwd_streams Bernard Iremonger
2016-06-14 12:28 ` [dpdk-dev] [PATCH v4 7/9] app/testpmd: check for valid socket id when attaching port Bernard Iremonger
2016-06-14 12:28 ` [dpdk-dev] [PATCH v4 8/9] app/testpmd: check for valid mbuf pool Bernard Iremonger
2016-06-14 12:28 ` [dpdk-dev] [PATCH v4 9/9] app/testpmd: stop forwarding on exit Bernard Iremonger
2016-06-14 15:35 ` [dpdk-dev] [PATCH v5 0/9] app/testpmd: forwarding Bernard Iremonger
2016-06-14 15:35 ` [dpdk-dev] [PATCH v5 1/9] app/testpmd: add new function Bernard Iremonger
2016-06-14 15:35 ` [dpdk-dev] [PATCH v5 2/9] app/testpmd: do not update forwarding config Bernard Iremonger
2016-06-14 15:35 ` [dpdk-dev] [PATCH v5 3/9] app/testpmd: check port is not forwarding Bernard Iremonger
2016-06-14 15:35 ` [dpdk-dev] [PATCH v5 4/9] app/testpmd: separate fwd config setup from display Bernard Iremonger
2016-06-14 15:35 ` [dpdk-dev] [PATCH v5 5/9] app/testpmd: add another new function Bernard Iremonger
2016-06-14 15:35 ` [dpdk-dev] [PATCH v5 6/9] app/testpmd: move fwd streams initialisation Bernard Iremonger
2016-06-14 15:35 ` [dpdk-dev] [PATCH v5 7/9] app/testpmd: check for valid socket id when attaching port Bernard Iremonger
2016-06-14 15:35 ` [dpdk-dev] [PATCH v5 8/9] app/testpmd: check for valid mbuf pool Bernard Iremonger
2016-06-14 15:35 ` [dpdk-dev] [PATCH v5 9/9] app/testpmd: stop forwarding on exit Bernard Iremonger
2016-06-14 15:39 ` [dpdk-dev] [PATCH v5 0/9] app/testpmd: forwarding De Lara Guarch, Pablo
2016-06-16 8:15 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8CEF83825BEC744B83065625E567D7C21A038267@IRSMSX108.ger.corp.intel.com \
--to=bernard.iremonger@intel.com \
--cc=dev@dpdk.org \
--cc=pablo.de.lara.guarch@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).