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

From: Slava Ovsiienko <viacheslavo@mellanox.com>
To: Andrew Rybchenko <arybchenko@solarflare.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: Matan Azrad <matan@mellanox.com>,
	Raslan Darawsheh <rasland@mellanox.com>,
	 Thomas Monjalon <thomas@monjalon.net>,
	"olivier.matz@6wind.com" <olivier.matz@6wind.com>
Subject: Re: [dpdk-dev] [PATCH v5] ethdev: extend flow metadata
Date: Wed, 30 Oct 2019 14:46:06 +0000	[thread overview]
Message-ID: <AM4PR05MB326516BD7BFE274B44162F33D2600@AM4PR05MB3265.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <AM4PR05MB3265168B2E9056BE5F549931D2600@AM4PR05MB3265.eurprd05.prod.outlook.com>

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

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.

With best regards, Slava