Re: [dpdk-dev] [PATCH 1/2] app/testpmd: add dynamic flag support

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 1/13/2020 9:29 AM, Ori Kam wrote:
> DPDK now supports registration of dynamic flags (dynf) to the mbuf.
> dynf can be given any name, and can be used with a supporting PMD or
> supporting application.
> 
> Due to the generic concept of the dynf, it is impossible and meaningless,
> to define register set/get function for each flag.
> This commit introduce a generic way to register and set/clear such flags.
> 
> The basic syntax:
> port config <port id> dynf <name> <set|clear>

+1 to command

> 
> The first step the new flag is registered. Regardless if the action is
> set or clear.
> There is no way to unregister the flag, after registring it.
> 
> The second step, if the action is set then we set the requested flag.
> If this is the first flag that is enabled we also register a call back
> for the Tx. In this call back we set the flag.
> If the action is clear the requested flag is cleared, and if there
> are no more flags that are set, the call back is removed.
> 
> The reason that the set is only applied in Tx is that in case of Rx
> it is assumed that the value comes from the PMD.
> 
> If log is enabled the name of the flag, and value will be printed
> in the packet info.
> In order for the log to work correcly the registration of the flag
> must be done before setting verbose.
> 
> Signed-off-by: Ori Kam <orika@mellanox.com>
> Acked-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

<...>

> +++ b/app/test-pmd/cmdline.c
> @@ -40,6 +40,7 @@
>  #include <rte_devargs.h>
>  #include <rte_flow.h>
>  #include <rte_gro.h>
> +#include <rte_mbuf_dyn.h>
>  
>  #include <cmdline_rdline.h>
>  #include <cmdline_parse.h>
> @@ -70,6 +71,8 @@
>  #include "cmdline_tm.h"
>  #include "bpf_cmd.h"
>  
> +char dynf_names[64][RTE_MBUF_DYN_NAMESIZE];
> +

I don't think 'cmdline.c' is good place for this global variable, can you please
move it to 'testpmd.c' among other global variables and can you please add some
comment as others do in that same file.

<...>

> +static void
> +cmd_config_dynf_specific_parsed(void *parsed_result,
> +				__attribute__((unused)) struct cmdline *cl,
> +				__attribute__((unused)) void *data)
> +{
> +	struct cmd_config_tx_dynf_specific_result *res = parsed_result;
> +	struct rte_mbuf_dynflag desc_flag;
> +	int flag;
> +	uint64_t old_port_flags;
> +
> +	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> +		return;
> +	flag = rte_mbuf_dynflag_lookup(res->name, NULL);
> +	if (flag <= 0) {
> +		strcpy(desc_flag.name, res->name);
> +		desc_flag.flags = 0;
> +		flag = rte_mbuf_dynflag_register(&desc_flag);
> +		if (flag < 0) {
> +			printf("Can't register flag");

"\n" is missing, which prevents the io buffer to be flushed and the log
displayed (at least for a long time).

<...>

> @@ -193,6 +200,9 @@ struct rte_port {
>  	/**< metadata value to insert in Tx packets. */
>  	uint32_t		tx_metadata;
>  	const struct rte_eth_rxtx_callback *tx_set_md_cb[RTE_MAX_QUEUES_PER_PORT+1];
> +	/**< dynamic flags. */
> +	uint64_t		dynf;

Everywhe in this patch, variables/descriptions referred as 'dynf' or "dynamic
flags", I think it would be better to prefix 'mbuf' to it, in case in the fure
we throw more dynamic stuff, just "dynamic flags" missing context. Yes, it will
make variable names longer but I think it will be more clear.

Not sure on the testpmd command though, no strong optinion but there I think
context is clear enough to continue with 'dynf' ("port config <port id> dynf
<name> set|clear").

<...>