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

[Olivier Matz <olivier.matz@6wind.com>]

On Wed, Oct 30, 2019 at 03:58:04PM +0000, Slava Ovsiienko wrote:
> > -----Original Message-----
> > From: Olivier Matz <olivier.matz@6wind.com>
> > Sent: Wednesday, October 30, 2019 17:20
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > Cc: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org;
> > Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> > <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>
> > Subject: Re: [PATCH v5] ethdev: extend flow metadata
> > 
> > Hi,
> > 
> > On Wed, Oct 30, 2019 at 02:46:06PM +0000, Slava Ovsiienko wrote:
> > > > -----Original Message-----
> > > > From: Slava Ovsiienko
> > > > Sent: Wednesday, October 30, 2019 16:41
> > > > To: Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org
> > > > Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> > > > <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>;
> > > > olivier.matz@6wind.com
> > > > Subject: RE: [PATCH v5] ethdev: extend flow metadata
> > > >
> > > > > -----Original Message-----
> > > > > From: Andrew Rybchenko <arybchenko@solarflare.com>
> > > > > Sent: Wednesday, October 30, 2019 10:02
> > > > > To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> > > > > Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> > > > > <rasland@mellanox.com>; Thomas Monjalon
> > <thomas@monjalon.net>;
> > > > > olivier.matz@6wind.com; Yongseok Koh <yskoh@mellanox.com>
> > > > > Subject: Re: [PATCH v5] ethdev: extend flow metadata
> > > > >
> > > > > 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.
> 
> Nice idea, thanks. I think existing lookup by name is OK, I'll update
> flag list routines with dynamic flag support.
> 
> > 
> > Maybe we could split the dump into fields and flags, and add a lookup by
> > offset/bitnum. Would it work for your use-case?
> 
> Not needed right now, I think.
> 
> > 
> > > It is interesting to note that despite metadata field looks to be
> > > related to rte_flow, there is no any reference to this field or flags inside
> > rte_flow API implementation.
> > > Only datapath references this field. Metadata is gateway between flow
> > > HW space and datapath, it tends to be mostly on datapath side not on
> > rte_flow.
> > 
> > Yes, only the registration of the field is related to rte_flow. But I don't get
> > where are you going with this. Is it a problem?
> 
> It is not a problem, rather some concern.
> IMO, the datapath related entities (flag mask and field offset) are put in not appropriate place.
> Now we have to add lookup to flag list routines, which we could avoid. The next candidates,
> like timestamp or timesync might introduce some new issues.

If the underlying question is should we centralize or not centralize the
definitions of dynamic fields/flags helpers, I'll tend to say that we
should not centralize. The reason is because it will not always be
possible: an application or an external library is allowed to register a
private dynamic field.

Nevertheless, as you say, introducing the next dynamic fields like
timestamp may show up some new issues, and we should be ready to rework
what has been done when we'll have more experience, with more usages of
dynamic fields.