From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 8D419376C for ; Thu, 4 Jun 2015 23:27:08 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP; 04 Jun 2015 14:27:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,554,1427785200"; d="scan'208";a="582332278" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by orsmga003.jf.intel.com with ESMTP; 04 Jun 2015 14:27:06 -0700 Received: from irsmsx106.ger.corp.intel.com ([169.254.8.189]) by IRSMSX101.ger.corp.intel.com ([169.254.1.217]) with mapi id 14.03.0224.002; Thu, 4 Jun 2015 22:27:05 +0100 From: "Chilikin, Andrey" To: Neil Horman , "Wiles, Keith" Thread-Topic: [dpdk-dev] [RFC PATCH] eal:Add new API for parsing args at rte_eal_init time Thread-Index: AQHQns5D9YqjHe4XWUCCG7CZpmo0SZ2c2gpQ Date: Thu, 4 Jun 2015 21:27:04 +0000 Message-ID: References: <1433357393-54434-1-git-send-email-keith.wiles@intel.com> <20150603171255.545e0df8@urahara> <20150604135542.GC24585@hmsreliant.think-freely.org> In-Reply-To: <20150604135542.GC24585@hmsreliant.think-freely.org> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [RFC PATCH] eal:Add new API for parsing args at rte_eal_init time 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: Thu, 04 Jun 2015 21:27:09 -0000 > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman > Sent: Thursday, June 4, 2015 2:56 PM > To: Wiles, Keith > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [RFC PATCH] eal:Add new API for parsing args at > rte_eal_init time >=20 > On Thu, Jun 04, 2015 at 11:50:33AM +0000, Wiles, Keith wrote: > > Hi Stephen > > > > On 6/3/15, 7:12 PM, "Stephen Hemminger" > wrote: > > > > >On Wed, 3 Jun 2015 13:49:53 -0500 > > >Keith Wiles wrote: > > > > > >> +/* Launch threads, called at application init() and parse app > > >> +args. */ int rte_eal_init_parse(int argc, char **argv, > > >> + int (*parse)(int, char **)) > > >> +{ > > >> + int ret; > > >> + > > >> + ret =3D rte_eal_init(argc, argv); > > >> + if ((ret >=3D 0) && (parse !=3D NULL)) { > > >> + argc -=3D ret; > > >> + argv +=3D ret; > > > > > >This won't work C is call by value. > > > > I tested this routine with Pktgen (again), which has a number of > > application options and it appears to work correctly. Can you explain > > why this will not work? > > > > Regards, > > ++Keith > > > > > > > >=20 > Stephen was thinking that your intent was to have argc, and argv modified= at > the call site of this function (i.e. if you called rte_eal_init_parse fro= m main(), > then after the call to rte_ela_init_parse, argc would be reduced by ret a= nd argv > would point forward in memory ret bytes in the main function, but they wo= nt. > It doesn't matter though, because you return ret, so the caller can do th= at > movement themselves. As you note, it works. >=20 > Note that if it was your intention to have argc and argv modified at the = call site, > then Stephen is right and this is broken, you need to modify the prototyp= e to be: > int rte_eal_init_parse(int *argc, char ***argv) >=20 > and do a dereference when modifying the parameters so the change is seen = at > the call site. >=20 > That said, I'm not sure theres much value in adding this to the API. For= one, it > implies that dpdk arguments need to come first on the command line. Whil= e > all the example applications do that, theres no requirement that they do = so, > and this function silently implies that they have to, so any existing app= lications > in the wild that violate that assumption are enjoined from using this >=20 > It also doesn't really save any code. If we pick an example app (I'll us= l2fwd- > jobstats), We currently have this: >=20 > /* init EAL */ > ret =3D rte_eal_init(argc, argv); > if (ret < 0) > rte_exit(EXIT_FAILURE, "Invalid EAL arguments\n"); > argc -=3D ret; > argv +=3D ret; >=20 > /* parse application arguments (after the EAL ones) */ > ret =3D l2fwd_parse_args(argc, argv); > if (ret < 0) > rte_exit(EXIT_FAILURE, "Invalid L2FWD arguments\n"); >=20 > With your new API we would get this: >=20 > ret =3D rte_eal_init_parse(argc, argv, l2fwd_parse_args) > if (ret < 0) > rte_exit(EXIT_FAILURE, "Invalid arguments - not sure what= \n"); >=20 > Its definately 5 fewer lines of source, but it doesn't save any execution > instructions, and for the effort of that, you loose the ability to determ= ine if it > was a DPDK argument or an application argument that failed. >=20 > Its not a bad addition, I'm just not sure its worth having to take on the > additional API surface to include. I'd be more supportive if you could e= nhance > the function to allow the previously mentioned before/after flexibiilty. = Then we > could just deprecate rte_eal_init as an API call entirely, and use this i= nstead. Before/after would be very useful, a lot of applications use only "-c" and = "-n" EAL command line parameters and "-c" in many cases is redundant as app= lication can calculate core mask from its own parameters, and "-n" just a r= equired parameter which can be defaulted to a platform specific value. So i= n addition to rte_set_application_usage_hook() it would be nice to have som= e more general way of overwriting eal initialization parameters.=20 >=20 > Neil