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

[Ori Kam <orika@mellanox.com>]

Hi Ferruh,

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, January 15, 2020 3:32 PM
> To: Ori Kam <orika@mellanox.com>; Wenzhuo Lu <wenzhuo.lu@intel.com>;
> Jingjing Wu <jingjing.wu@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>; John McNamara
> <john.mcnamara@intel.com>; Marko Kovacevic
> <marko.kovacevic@intel.com>
> Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@mellanox.com>; Matan
> Azrad <matan@mellanox.com>
> Subject: Re: [PATCH 1/2] app/testpmd: add dynamic flag support
> 
> 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.
> 

Sure will move.

> <...>
> 
> > +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).
> 

Will add missing \n

> <...>
> 
> > @@ -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.
>

O.K will add mbuf_
 
> 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").
> 
I will leave it as it is now.

> <...>

Best,
Ori