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

[Slava Ovsiienko <viacheslavo@mellanox.com>]

Hi, 

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Thursday, October 31, 2019 17:51
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
> Darawsheh <rasland@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; arybchenko@solarflare.com; Ori Kam
> <orika@mellanox.com>
> Subject: Re: [PATCH v7 2/2] ethdev: move egress metadata to dynamic field
> 
> Hi,
> 
> On Thu, Oct 31, 2019 at 01:05:21PM +0000, Viacheslav Ovsiienko wrote:
> > The dynamic mbuf fields were introduced by [1]. The egress metadata is
> > good candidate to be moved 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.
> >
> > DEV_TX_OFFLOAD_MATCH_METADATA offload and configuration flag is
> removed.
> > The metadata support in PMDs is engaged on dynamic field registration.
> >
> > Metadata feature is getting complex. We might have some set of actions
> > and items that might be supported by PMDs in multiple combinations,
> > the supported values and masks are the subjects to query by perfroming
> > trials (with rte_flow_validate).
> >

[snip]

> > +/* Mbuf dynamic flags for metadata. */
> >  #define PKT_RX_DYNF_METADATA (rte_flow_dynf_metadata_mask)
> > +#define PKT_TX_DYNF_METADATA (rte_flow_dynf_metadata_mask)
> 
> Should we have 2 defines pointing to the same mask? Shall we use
> PKT_DYNF_METADATA for both?
It is just a style issue, we have two sets of PKT_RX_xxxx and PKT_TX_xxx flags,
it just looks nice to use appropriate flags in RX/TX parts of datapath.

> >
> >  __rte_experimental
> >  static inline uint32_t
> > diff --git a/lib/librte_mbuf/rte_mbuf.c b/lib/librte_mbuf/rte_mbuf.c
> > index 8c51dc1..35df1c4 100644
> > --- a/lib/librte_mbuf/rte_mbuf.c
> > +++ b/lib/librte_mbuf/rte_mbuf.c
> > @@ -670,7 +670,6 @@ const char *rte_get_tx_ol_flag_name(uint64_t
> mask)
> >  	case PKT_TX_SEC_OFFLOAD: return "PKT_TX_SEC_OFFLOAD";
> >  	case PKT_TX_UDP_SEG: return "PKT_TX_UDP_SEG";
> >  	case PKT_TX_OUTER_UDP_CKSUM: return
> "PKT_TX_OUTER_UDP_CKSUM";
> > -	case PKT_TX_METADATA: return "PKT_TX_METADATA";
> >  	default: return NULL;
> >  	}
> >  }
> > @@ -707,7 +706,6 @@ const char *rte_get_tx_ol_flag_name(uint64_t
> mask)
> >  		{ PKT_TX_SEC_OFFLOAD, PKT_TX_SEC_OFFLOAD, NULL },
> >  		{ PKT_TX_UDP_SEG, PKT_TX_UDP_SEG, NULL },
> >  		{ PKT_TX_OUTER_UDP_CKSUM,
> PKT_TX_OUTER_UDP_CKSUM, NULL },
> > -		{ PKT_TX_METADATA, PKT_TX_METADATA, NULL },
> >  	};
> >  	const char *name;
> >  	unsigned int i;
> > diff --git a/lib/librte_mbuf/rte_mbuf_core.h
> > b/lib/librte_mbuf/rte_mbuf_core.h index 3022701..edfc7e9 100644
> > --- a/lib/librte_mbuf/rte_mbuf_core.h
> > +++ b/lib/librte_mbuf/rte_mbuf_core.h
> > @@ -192,11 +192,6 @@
> >  /* add new TX flags here, don't forget to update PKT_LAST_FREE  */
> >
> >  /**
> > - * Indicate that the metadata field in the mbuf is in use.
> > - */
> > -#define PKT_TX_METADATA	(1ULL << 40)
> > -
> > -/**
> 
> You should also update PKT_LAST_FREE just above.
> 
Yes, my bad, will fix.

[snip] 

With best regards, Slava