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

[Slava Ovsiienko <viacheslavo@mellanox.com>]

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@linux.intel.com>
> Sent: Tuesday, October 8, 2019 15:51
> To: Adrien Mazarguil <adrien.mazarguil@6wind.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>
> Cc: Yongseok Koh <yskoh@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; Olivier Matz <olivier.matz@6wind.com>; Bruce
> Richardson <bruce.richardson@intel.com>; Shahaf Shuler
> <shahafs@mellanox.com>; Ferruh Yigit <ferruh.yigit@intel.com>; dev
> <dev@dpdk.org>; Slava Ovsiienko <viacheslavo@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: extend flow metadata
> 
> On 7/29/2019 4:06 PM, Adrien Mazarguil wrote:
> > On Sun, Jul 14, 2019 at 02:46:58PM +0300, Andrew Rybchenko wrote:
> >> On 11.07.2019 10:44, Adrien Mazarguil wrote:
> >>> On Wed, Jul 10, 2019 at 04:37:46PM +0000, Yongseok Koh wrote:
> >>>>> On Jul 10, 2019, at 5:26 AM, Thomas Monjalon
> <thomas@monjalon.net> wrote:
> >>>>>
> >>>>> 10/07/2019 14:01, Bruce Richardson:
> >>>>>> On Wed, Jul 10, 2019 at 12:07:43PM +0200, Olivier Matz wrote:
> >>>>>>> On Wed, Jul 10, 2019 at 10:55:34AM +0100, Bruce Richardson
> wrote:
> >>>>>>>> On Wed, Jul 10, 2019 at 11:31:56AM +0200, Olivier Matz wrote:
> >>>>>>>>> On Thu, Jul 04, 2019 at 04:21:22PM -0700, Yongseok Koh wrote:
> >>>>>>>>>> Currently, metadata can be set on egress path via mbuf
> >>>>>>>>>> tx_meatadata field with PKT_TX_METADATA flag and
> RTE_FLOW_ITEM_TYPE_RX_META matches metadata.
> >>>>>>>>>>
> >>>>>>>>>> This patch extends the 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 packet Rx. 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 mbuf metadata field with PKT_RX_METADATA
> ol_flag.
> >>>>>>>>>>
> >>>>>>>>>> For this purpose, mbuf->tx_metadata is moved as a separate
> >>>>>>>>>> new field and renamed to 'metadata' to support both Rx and Tx
> metadata.
> >>>>>>>>>>
> >>>>>>>>>> For loopback/hairpin packet, metadata set on Rx/Tx may or
> may
> >>>>>>>>>> not be propagated to the other path depending on HW
> capability.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> >>>>>>>>>> --- a/lib/librte_mbuf/rte_mbuf.h
> >>>>>>>>>> +++ b/lib/librte_mbuf/rte_mbuf.h
> >>>>>>>>>> @@ -648,17 +653,6 @@ struct rte_mbuf {
> >>>>>>>>>> 			/**< User defined tags. See
> rte_distributor_process() */
> >>>>>>>>>> 			uint32_t usr;
> >>>>>>>>>> 		} hash;                   /**< hash information */
> >>>>>>>>>> -		struct {
> >>>>>>>>>> -			/**
> >>>>>>>>>> -			 * Application specific metadata value
> >>>>>>>>>> -			 * for egress flow rule match.
> >>>>>>>>>> -			 * Valid if PKT_TX_METADATA is set.
> >>>>>>>>>> -			 * Located here to allow conjunct use
> >>>>>>>>>> -			 * with hash.sched.hi.
> >>>>>>>>>> -			 */
> >>>>>>>>>> -			uint32_t tx_metadata;
> >>>>>>>>>> -			uint32_t reserved;
> >>>>>>>>>> -		};
> >>>>>>>>>> 	};
> >>>>>>>>>>
> >>>>>>>>>> 	/** Outer VLAN TCI (CPU order), valid if PKT_RX_QINQ is set.
> >>>>>>>>>> */ @@ -727,6 +721,11 @@ struct rte_mbuf {
> >>>>>>>>>> 	 */
> >>>>>>>>>> 	struct rte_mbuf_ext_shared_info *shinfo;
> >>>>>>>>>>
> >>>>>>>>>> +	/** Application specific metadata value for flow rule match.
> >>>>>>>>>> +	 * Valid if PKT_RX_METADATA or PKT_TX_METADATA is set.
> >>>>>>>>>> +	 */
> >>>>>>>>>> +	uint32_t metadata;
> >>>>>>>>>> +
> >>>>>>>>>> } __rte_cache_aligned;
> >>>>>>>>> This will break the ABI, so we cannot put it in 19.08, and we
> >>>>>>>>> need a deprecation notice.
> >>>>>>>>>
> >>>>>>>> Does it actually break the ABI? Adding a new field to the mbuf
> >>>>>>>> should only break the ABI if it either causes new fields to
> >>>>>>>> move or changes the structure size. Since this is at the end,
> >>>>>>>> it's not going to move any older fields, and since everything
> >>>>>>>> is cache-aligned I don't think the structure size changes either.
> >>>>>>> I think it does break the ABI: in previous version, when the
> >>>>>>> PKT_TX_METADATA flag is set, the associated value is put in
> >>>>>>> m->tx_metadata (offset 44 on x86-64), and in the next version,
> >>>>>>> it will be in m->metadata (offset 112). So, these 2 versions are not
> binary compatible.
> >>>>>>>
> >>>>>>> Anyway, at least it breaks the API.
> >>>>>> Ok, I misunderstood. I thought it was the structure change itself
> >>>>>> you were saying broke the ABI. Yes, putting the data in a
> >>>>>> different place is indeed an ABI break.
> >>>>> We could add the new field and keep the old one unused, so it does
> >>>>> not break the ABI.
> >>>> Still breaks ABI if PKT_TX_METADATA is set. :-) In order not to
> >>>> break it, I can keep the current union'd field (tx_metadata) as is
> >>>> with PKT_TX_METADATA, add the new one at the end and make it used
> with the new PKT_RX_METADATA.
> >>>>
> >>>>> However I suppose everybody will prefer a version using dynamic
> fields.
> >>>>> Is someone against using dynamic field for such usage?
> >>>> However, given that the amazing dynamic fields is coming soon
> >>>> (thanks for your effort, Olivier and Thomas!), I'd be honored to be the
> first user of it.
> >>>>
> >>>> Olivier, I'll take a look at your RFC.
> >>> Just got a crazy idea while reading this thread... How about
> >>> repurposing that "reserved" field as "rx_metadata" in the meantime?
> >>
> >> It overlaps with hash.fdir.hi which has RSS hash.
> >
> > While it does overlap with hash.fdir.hi, isn't the RSS hash stored in
> > the "rss" field overlapping with hash.fdir.lo? (see struct
> > rte_flow_action_rss)
> >
> > hash.fdir.hi was originally used by FDIR and later repurposed by
> > rte_flow for its MARK action, which neatly qualifies as Rx metadata so
> > renaming "reserved" as "rx_metadata" could already make sense.
> >
> > That is, assuming users do not need two different kinds of Rx metadata
> > returned simultaneously with their packets. I think it's safe.
> >
> >>> I know reserved fields are cursed and no one's ever supposed to
> >>> touch them but this risk is mitigated by having the end user
> >>> explicitly request its use, so the patch author (and his relatives)
> >>> should be safe from the resulting bad juju.
> >>>
> >>> Joke aside, while I like the idea of Tx/Rx META, I think the
> >>> similarities with MARK (and TAG eventually) is a problem. I wasn't
> >>> available and couldn't comment when META was originally added to the
> >>> Tx path, but there's a lot of overlap between these items/actions,
> >>> without anything explaining to the end user how and why they should
> >>> pick one over the other, if they can be combined at all and what happens
> in that case.
> >>>
> >>> All this must be documented, then we should think about unifying
> >>> their respective features and deprecate the less capable
> >>> items/actions. In my opinion, users need exactly one method to
> >>> mark/match some mark while processing Rx/Tx traffic and *optionally*
> >>> have that mark read from/written to the mbuf, which may or may not be
> possible depending on HW features.
> >
> > Thoughts regarding this suggestion? From a user perspective I think
> > all these actions should be unified but maybe there are good reasons
> > to keep them separate?
> >
> 
> I think more recent plan is introducing dynamic fields for the remaining 16
> bytes in the second cacheline.
> 
> I will update the patch as rejected, is there any objection?

v2 is coming,  will be based on dynamic mbuf fields.
I think Superseded / Changes Requested is more relevant.

WBR, Slava