From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id E41402949 for ; Fri, 10 Jun 2016 17:58:02 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP; 10 Jun 2016 08:58:02 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,450,1459839600"; d="scan'208";a="717054059" Received: from irsmsx154.ger.corp.intel.com ([163.33.192.96]) by FMSMGA003.fm.intel.com with ESMTP; 10 Jun 2016 08:58:01 -0700 Received: from irsmsx111.ger.corp.intel.com (10.108.20.4) by IRSMSX154.ger.corp.intel.com (163.33.192.96) with Microsoft SMTP Server (TLS) id 14.3.248.2; Fri, 10 Jun 2016 16:58:00 +0100 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.183]) by irsmsx111.ger.corp.intel.com ([169.254.2.182]) with mapi id 14.03.0248.002; Fri, 10 Jun 2016 16:58:00 +0100 From: "Iremonger, Bernard" To: "De Lara Guarch, Pablo" , "dev@dpdk.org" Thread-Topic: [PATCH v2 4/8] app/testpmd: reconfigure forwarding after changing portlist Thread-Index: AQHRpuR9aQFXjFoaI0OBbGG7HJzjqZ/fwl2AgALdalCAAHEFoA== Date: Fri, 10 Jun 2016 15:58:00 +0000 Message-ID: <8CEF83825BEC744B83065625E567D7C21A038267@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> <8CEF83825BEC744B83065625E567D7C21A037FA5@IRSMSX108.ger.corp.intel.com> In-Reply-To: <8CEF83825BEC744B83065625E567D7C21A037FA5@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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Jun 2016 15:58:03 -0000 Hi Pablo, > > > 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 configuratio= n. > > > > > > 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. >=20 > Ok, fixes tag is not necessary here. >=20 > > > Signed-off-by: Bernard Iremonger > > > --- > > > 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 =3D (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=3D0xca4160 > > ) 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.=20 > > 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 reconfi= gure 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 se= nse: > > http://dpdk.org/dev/patchwork/patch/13132/ >=20 > 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 wh= at is happening. fwd_config_display() is renamed to fwd_config_setup_display(), it does not = separate the setup from the display. =20 > > > 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; > > > > Is this really necessary? I have removed it and I can quit testpmd > > with no problem. >=20 > 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