From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 508897CDC for ; Tue, 16 Oct 2018 16:20:18 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Oct 2018 07:20:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,388,1534834800"; d="scan'208";a="83030572" Received: from klaatz-mobl.ger.corp.intel.com (HELO [10.237.220.107]) ([10.237.220.107]) by orsmga006.jf.intel.com with ESMTP; 16 Oct 2018 07:20:14 -0700 To: Thomas Monjalon Cc: dev@dpdk.org, harry.van.haaren@intel.com, stephen@networkplumber.org, gaetan.rivet@6wind.com, shreyansh.jain@nxp.com, mattias.ronnblom@ericsson.com, bruce.richardson@intel.com References: <20181010105134.48079-1-kevin.laatz@intel.com> <20181011165837.81030-1-kevin.laatz@intel.com> <20181011165837.81030-2-kevin.laatz@intel.com> <3367694.NQyIiAbOc1@xps> From: "Laatz, Kevin" Message-ID: <4335c586-cbe6-9564-d6cf-f1ac125b27ab@intel.com> Date: Tue, 16 Oct 2018 15:20:13 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <3367694.NQyIiAbOc1@xps> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH v4 01/13] eal: add param register infrastructure X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Oct 2018 14:20:18 -0000 Hi Thomas, Thanks for reviewing, see replies below. On 16/10/2018 14:42, Thomas Monjalon wrote: > Hi, > > 11/10/2018 18:58, Kevin Laatz: >> This commit adds infrastructure to EAL that allows an application to >> register it's init function with EAL. This allows libraries to be >> initialized at the end of EAL init. >> >> This infrastructure allows libraries that depend on EAL to be initialized >> as part of EAL init, removing circular dependency issues. > Let's try to have a clear documentation for this new infra. Are you looking for a doc file here? Or just better Doxygen comments? > >> +/** >> + * @file >> + * >> + * This API introduces the ability to register callbacks with EAL. When > You should explain when the callback is called, and what is the role of > the callback. I explained this below with telemetry as an example, maybe it needs to be extended a little. > >> + * registering a callback, the application also provides a flag as part of the >> + * struct used to register. If the flag is passed to EAL when ruuning a DPDK > What do you call a flag? Are you talking about an option to be parsed? Agreed. The wording was unfortunate here. Will change to 'option' to make it clear it is related to getopt > >> + * application, the relevant callback will be called at the end of EAL init. >> + * For example, passing --telemetry will make the telemetry init be called at >> + * the end of EAl init. >> + * >> + * The register API can be used to resolve circular dependency issue between >> + * EAL and the library. > You need to explain what is the circular dependency issue. I wrote the Doxygen trying to keep it generic. I can make it telemetry specific if it will be clearer? > > [...] >> +struct rte_param { > Please add a global comment for this struct, explain what it represents. Good spot, thanks. >> + TAILQ_ENTRY(rte_param) next; /** The next entry in the TAILQ*/ >> + char *eal_flag; /** The EAL flag */ > eal_flag, The EAL flag > Hum... > Please use different words to explain what is a flag. > If it is something to be parsed by getopt, it should be called > an option, not a flag. > >> + char *help_text; /** Help text for the callback */ > What the help text is used for? When is it printed? The help text is currently not being used. It was added speculatively. > >> + rte_param_cb cb; /** Function pointer of the callback */ >> + int enabled; /** Enabled flag, should be 0 by default */ > What means enabled in this context? Enabled means that the option, for example --telemetry, was passed and that we need to call the callback at the end of EAL init. If the option was not passed for a given callback, then the callback will not be called. > >> +}; >> + >> +/** >> + * @internal Check if the passed flag is valid >> + * >> + * @param flag >> + * The flag to be parsed > Here too, you need to be more explicit about flag meaning. > >> + * >> + * @return >> + * 0 on success >> + * @return >> + * -1 on fail >> + */ >> +int >> +rte_param_parse(char *flag); >> + >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice >> + * >> + * Register a function with EAL. Registering the function will enable the >> + * function to be called at the end of EAL init. >> + * >> + * @param reg_param >> + * rte_param structure > No, this is not a helpful comment. > >> + */ >> +void __rte_experimental >> +rte_param_register(struct rte_param *reg_param); > >