Hi Ferruh,
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Wednesday, January 15, 2020 3:32 PM
> To: Ori Kam <or...@mellanox.com>; Wenzhuo Lu <wenzhuo...@intel.com>;
> Jingjing Wu <jingjing...@intel.com>; Bernard Iremonger
> <bernard.iremon...@intel.com>; John McNamara
> <john.mcnam...@intel.com>; Marko Kovacevic
> <marko.kovace...@intel.com>
> Cc: dev@dpdk.org; Slava Ovsiienko <viachesl...@mellanox.com>; Matan
> Azrad <ma...@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 <or...@mellanox.com>
> > Acked-by: Viacheslav Ovsiienko <viachesl...@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
