From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <bernard.iremonger@intel.com>
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
by dpdk.org (Postfix) with ESMTP id AAD332BAE
for <dev@dpdk.org>; Fri, 10 Jun 2016 11:22:59 +0200 (CEST)
Received: from fmsmga003.fm.intel.com ([10.253.24.29])
by orsmga103.jf.intel.com with ESMTP; 10 Jun 2016 02:22:59 -0700
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.26,449,1459839600"; d="scan'208";a="716913109"
Received: from irsmsx154.ger.corp.intel.com ([163.33.192.96])
by FMSMGA003.fm.intel.com with ESMTP; 10 Jun 2016 02:22:59 -0700
Received: from irsmsx155.ger.corp.intel.com (163.33.192.3) by
IRSMSX154.ger.corp.intel.com (163.33.192.96) with Microsoft SMTP Server (TLS)
id 14.3.248.2; Fri, 10 Jun 2016 10:22:57 +0100
Received: from irsmsx108.ger.corp.intel.com ([169.254.11.183]) by
irsmsx155.ger.corp.intel.com ([169.254.14.34]) with mapi id 14.03.0248.002;
Fri, 10 Jun 2016 10:22:56 +0100
From: "Iremonger, Bernard" <bernard.iremonger@intel.com>
To: "De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>, "dev@dpdk.org"
<dev@dpdk.org>
Thread-Topic: [PATCH v2 4/8] app/testpmd: reconfigure forwarding after
changing portlist
Thread-Index: AQHRpuR9aQFXjFoaI0OBbGG7HJzjqZ/fwl2AgALdalA=
Date: Fri, 10 Jun 2016 09:22:56 +0000
Message-ID: <8CEF83825BEC744B83065625E567D7C21A037FA5@IRSMSX108.ger.corp.intel.com>
References: <1461156779-24737-1-git-send-email-bernard.iremonger@intel.com>
<1462462795-18767-1-git-send-email-bernard.iremonger@intel.com>
<1462462795-18767-5-git-send-email-bernard.iremonger@intel.com>
<E115CCD9D858EF4F90C690B0DCB4D8973C95C7A1@IRSMSX108.ger.corp.intel.com>
In-Reply-To: <E115CCD9D858EF4F90C690B0DCB4D8973C95C7A1@IRSMSX108.ger.corp.intel.com>
Accept-Language: en-GB, en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNzZiODc5MDAtMzI4Yi00NGQyLWE3ODktM2VjYzFhMzg0NzYzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6ImgzZ0dQcU1qeFVibkxZNTRoZEo0SndnWHQ5ZXdzRU55bTFtUzNDb2NpR0U9In0=
x-ctpclassification: CTP_IC
x-originating-ip: [163.33.239.180]
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [PATCH v2 4/8] app/testpmd: reconfigure forwarding
after changing portlist
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
X-List-Received-Date: Fri, 10 Jun 2016 09:23:00 -0000
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")
>=20
> 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");
> > }
>=20
> The problem of doing this is that if user starts testpmd, it is not possi=
ble to
> show the configuration of the ports directly, since fwd_config_setup() ha=
s
> 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 initializati=
on.
>=20
> >
> > 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 =3D (portid_t) nb_pt;
> > }
> > +
> > + fwd_config_setup();
> > }
>=20
> 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:
>=20
> 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=3D0xca4160
> <cur_fwd_config>) at /tmp/dpdk-latest/app/test-pmd/config.c:1073
>=20
> 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 reconfigu=
re it.
> That's why I think it was decided to reconfigure the configuration when
> starting the forwarding or when showing the configuration.
>=20
> 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 sens=
e:
> http://dpdk.org/dev/patchwork/patch/13132/
Option 2 looks like the best choice here, to drop this patch in favour of p=
atch
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 !=3D NULL) {
> > no_link_check =3D 1;
> > + nb_fwd_ports =3D 0;
>=20
> Is this really necessary? I have removed it and I can quit testpmd with n=
o
> problem.
Ok, was just clearing this on exit as it had been set previously.
>=20
> > FOREACH_PORT(pt_id, ports) {
> > printf("\nShutting down port %d...\n", pt_id);
> > fflush(stdout);
> > --
> > 2.6.3