Re: [dpdk-dev] [PATCH v5] ethdev: extend flow metadata

[Thomas Monjalon <thomas@monjalon.net>]

30/10/2019 16:20, Olivier Matz:
> > From: Slava Ovsiienko
> > > > On 10/29/19 10:31 PM, Viacheslav Ovsiienko wrote:
> > > > > Currently, metadata can be set on egress path via mbuf tx_metadata
> > > > > field with PKT_TX_METADATA flag and RTE_FLOW_ITEM_TYPE_META
> > > > matches metadata.
> > > > >
> > > > > This patch extends the metadata feature usability.
> > > > >
> > > > > 1) RTE_FLOW_ACTION_TYPE_SET_META
> > > > >
> > > > > When supporting multiple tables, Tx metadata can also be set by a
> > > > > rule and matched by another rule. This new action allows metadata to
> > > > > be set as a result of flow match.
> > > > >
> > > > > 2) Metadata on ingress
> > > > >
> > > > > There's also need to support metadata on ingress. Metadata can be
> > > > > set by SET_META action and matched by META item like Tx. The final
> > > > > value set by the action will be delivered to application via
> > > > > metadata dynamic field of mbuf which can be accessed by
> > > > > RTE_FLOW_DYNF_METADATA() macro or with
> > > > > rte_flow_dynf_metadata_set() and rte_flow_dynf_metadata_get() helper
> > > > > routines. PKT_RX_DYNF_METADATA flag will be set along with the data.
> > > 
> > > We have a problem with PKT_RX_DYNF_METADATA/
> > > PKT_TX_DYNF_METADATA.
> > > These ones are referencing to global variable
> > > "rte_flow_dynf_metadata_mask".
> > > And there are routines in rte_mbuf.c  (rte_get_rx_ol_flag_list) which show
> > > the names of flags. It is not good if rte_mbuf.c would require including of
> > > rte_flow.h and  linking rte_flow.c (not all apps use rte_flow or even ethdev).
> > > 
> > > What do you think? Should we rename rte_flow_dynf_xxxxx variables to
> > > rte_mbuf_dynf_flow_metadata_xxxx and put ones into the  rte_mbuf_dyn.c?
> > > The same about PKT_RX_DYNF_METADATA definition, is it candidate to
> > > move to rte_mbuf_dyn.h ? It would allow not to link or include rte_flow.c/h
> > > into rte_mbuf.c
> > > 
> 
> In rte_mbuf_dyn.c, we maintain a list of registered flags. I think it
> wouldn't be too difficult to introduce the equivalent of
> rte_get_*_ol_flag_list() and *rte_get_*_ol_flag_name() for dynamic
> flags. There is already a dump function (which does both fields and
> flags), and a lookup by name function.
> 
> Maybe we could split the dump into fields and flags, and add a lookup by
> offset/bitnum. Would it work for your use-case?

I think it would not be so useful.
I propose to maintain the function rte_get_rx_ol_flag_list() only
for static mbuf fields, and do not list the dynamic flags.
In short, do nothing :)