Re: [dpdk-dev] [PATCH v6 2/2] ethdev: move egress metadata to dynamic field

[Slava Ovsiienko <viacheslavo@mellanox.com>]

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Thursday, October 31, 2019 11: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; Ori Kam <orika@mellanox.com>
> Subject: Re: [PATCH v6 2/2] ethdev: move egress metadata to dynamic field
> 
> On 10/30/19 8:12 PM, Viacheslav Ovsiienko wrote:
> > The dynamic mbuf fields were introduced by [1]. The egress metadata is
> > good candidate to be move from statically allocated field tx_metadata
> > to dynamic one. Because mbufs are used in half-duplex fashion only, it
> > is safe to share this dynamic field with ingress metadata.
> >
> > The shared dynamic field contains either egress (if application going
> > to transmit mbuf with tx_burst) or ingress (if mbuf is received with
> > rx_burst) metadata and 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_TX_DYNF_METADATA/PKT_RX_DYNF_METADATA flag will be
> set
> > along with the data.
> >
> > The mbuf dynamic field must be registered by calling
> > rte_flow_dynf_metadata_register() prior accessing the data.
> >
> > The availability of dynamic mbuf metadata field can be checked with
> > rte_flow_dynf_metadata_avail() routine.
> >
> > [1]
> >
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatch
> >
> es.dpdk.org%2Fpatch%2F62040%2F&amp;data=02%7C01%7Cviacheslavo%4
> 0mellan
> >
> ox.com%7C3c5c76e00ac242bf9b4a08d75de0f4e8%7Ca652971c7d2e4d9ba6
> a4d14925
> >
> 6f461b%7C0%7C0%7C637081093087328482&amp;sdata=6KLkc21qu%2FFBY
> 3n9JRBE67
> > 0es%2FznOn3c2EwFi4i6qf4%3D&amp;reserved=0
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> 
> LGTM
> 
> I think release notes should be updated.
These are. In the first patch of series. There was tx_metadata endianness
altering, so I added the move to dynamic field there. Would you like to split it ?
Or to add  some more details ?

> 
> What I don't understand now is the way for application to understand if Tx
> metadata is supported or not.
> Corresponding offload flag is removed.
Yes, and was crying with bloody tiers while doing that.
Metadata feature is getting complex. We have some set of actions and items
that might be supported by PMDs in multiple combinations, the supported 
values and masks are also the subjects to query - we have no way to describe.
So, trial looks to be the only way to detect supported aspects of metadata
feature in run time.

 I guess the answer is
> rte_flow_validate() with a rule on egress which tries to match meta and do
> something (?).
Yes, it is supposed way, I call it - "trial".

> It should be highlighted in the documentation in any case, but I'd consider to
> keep the offload.
Metadata feature is considered rather as application requirement than
some PMD configuration option. Let's have a glance from other side.

If application neither needs nor supports metadata, it just does not register field,
PMDs are not bothered at all and work without any metadata handling.
If application relies on metadata strongly, requires ones, it does trials and fails
if required metadata aspects not supported.
If application has options to operate with or w/o metadata - it needs trials anyway.
If trials are OK - application registers metadata field and PMDs supporting this feature
become engaged.

With best regards, Slava