From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id AA980C35A for ; Thu, 4 Jun 2015 16:27:51 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP; 04 Jun 2015 07:27:40 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,552,1427785200"; d="scan'208";a="502729165" Received: from orsmsx106.amr.corp.intel.com ([10.22.225.133]) by FMSMGA003.fm.intel.com with ESMTP; 04 Jun 2015 07:27:41 -0700 Received: from fmsmsx154.amr.corp.intel.com (10.18.116.70) by ORSMSX106.amr.corp.intel.com (10.22.225.133) with Microsoft SMTP Server (TLS) id 14.3.224.2; Thu, 4 Jun 2015 07:27:40 -0700 Received: from fmsmsx113.amr.corp.intel.com ([169.254.13.213]) by FMSMSX154.amr.corp.intel.com ([169.254.6.53]) with mapi id 14.03.0224.002; Thu, 4 Jun 2015 07:27:39 -0700 From: "Wiles, Keith" To: Neil Horman Thread-Topic: [dpdk-dev] [RFC PATCH] eal:Add new API for parsing args at rte_eal_init time Thread-Index: AQHQnltA/wkwK4V9H0iqXqFk9uBgcZ2cXhyAgAB20AD//7UYAA== Date: Thu, 4 Jun 2015 14:27:37 +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-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.252.134.161] Content-Type: text/plain; charset="us-ascii" Content-ID: <197676AD7E5A7445A1CAB7A6D84A8C05@intel.com> 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 14:27:52 -0000 Hi Neil and Stephen, On 6/4/15, 8:55 AM, "Neil Horman" wrote: >On Thu, Jun 04, 2015 at 11:50:33AM +0000, Wiles, Keith wrote: >> Hi Stephen >>=20 >> On 6/3/15, 7:12 PM, "Stephen Hemminger" >>wrote: >>=20 >> >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. >>=20 >> 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? >>=20 >> Regards, >> ++Keith >> > >>=20 >>=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 from >main(), >then after the call to rte_ela_init_parse, argc would be reduced by ret >and argv >would point forward in memory ret bytes in the main function, but they >wont. It >doesn't matter though, because you return ret, so the caller can do that >movement themselves. As you note, it works. > >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 >prototype >to be: >int rte_eal_init_parse(int *argc, char ***argv) My intent was not to alter the argc and argv values as that is not a reasonable use case, correct? > >and do a dereference when modifying the parameters so the change is seen >at the >call site. > >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. >While >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 >applications >in the wild that violate that assumption are enjoined from using this > >It also doesn't really save any code. If we pick an example app (I'll us >l2fwd-jobstats), We currently have this: > > /* 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; > > /* 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"); > >With your new API we would get this: > > ret =3D rte_eal_init_parse(argc, argv, l2fwd_parse_args) > if (ret < 0) > rte_exit(EXIT_FAILURE, "Invalid arguments - not sure >what\n"); > >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 >determine if >it was a DPDK argument or an application argument that failed. I agree this is not saving instructions and adding performance, but of code clutter and providing a layered model for the developer. The rte_eal_init() routine still exists and I was not trying to remove that API only layer a convenient API for common constructs. > >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 >enhance >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 >instead. I can see we can create an API to add support for doing the applications args first or after, but would that even be acceptable? ++Keith > >Neil >