[dpdk-dev] [PATCH v4 01/13] eal: add param register infrastructure

Laatz, Kevin kevin.laatz at intel.com
Tue Oct 16 16:20:13 CEST 2018


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);
>
>



More information about the dev mailing list