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

[Slava Ovsiienko <viacheslavo@mellanox.com>]

Hi, Olivier

Thanks a lot for the review.

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Tuesday, October 29, 2019 18:25
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Matan
> Azrad <matan@mellanox.com>; Ori Kam <orika@mellanox.com>; Yongseok
> Koh <yskoh@mellanox.com>
> Subject: Re: [PATCH v4] ethdev: extend flow metadata
> 
> Hi Slava,
> 
> Looks good to me overall. Few minor comments below.
> 
> On Sun, Oct 27, 2019 at 06:40:36PM +0000, 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().
> > 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 to use SET_META action.
> >
> > The availability of dynamic mbuf metadata field can be checked with
> > rte_flow_dynf_metadata_avail() routine.
> >
> > For loopback/hairpin packet, metadata set on Rx/Tx may or may not be
> > propagated to the other path depending on hardware capability.
> >
> > Signed-off-by: Yongseok Koh <yskoh@mellanox.com>
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> 
> (...)
> 
> > diff --git a/lib/librte_ethdev/rte_ethdev.h
> > b/lib/librte_ethdev/rte_ethdev.h index c36c1b6..b19c86b 100644
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> > @@ -1048,7 +1048,6 @@ struct rte_eth_conf {
> >  #define DEV_RX_OFFLOAD_KEEP_CRC		0x00010000
> >  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
> >  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
> > -
> >  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM
> | \
> >  				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> >  				 DEV_RX_OFFLOAD_TCP_CKSUM)
> 
> Undue removed line here.

Right, will fix.

> 
> (...)
> 
> > +/* Mbuf dynamic field offset for metadata. */ extern int
> > +rte_flow_dynf_metadata_offs;
> > +
> > +/* Mbuf dynamic field flag mask for metadata. */ extern uint64_t
> > +rte_flow_dynf_metadata_mask;
> > +
> > +/* Mbuf dynamic field pointer for metadata. */ #define
> > +RTE_FLOW_DYNF_METADATA(m) \
> > +	RTE_MBUF_DYNFIELD((m), rte_flow_dynf_metadata_offs, uint32_t
> *)
> > +
> > +/* Mbuf dynamic flag for metadata. */ #define PKT_RX_DYNF_METADATA
> > +(rte_flow_dynf_metadata_mask)
> > +
> > +__rte_experimental
> > +static inline uint32_t
> > +rte_flow_dynf_metadata_get(struct rte_mbuf *m) {
> > +	return *RTE_FLOW_DYNF_METADATA(m);
> > +}
> > +
> > +__rte_experimental
> > +static inline void
> > +rte_flow_dynf_metadata_set(struct rte_mbuf *m, uint32_t v) {
> > +	*RTE_FLOW_DYNF_METADATA(m) = v;
> > +}
> > +
> 
> (...)
> 
> > +__rte_experimental
> > +static inline int
> > +rte_flow_dynf_metadata_avail(void) {
> > +       return !!rte_flow_dynf_metadata_mask; }
> 
> I think, in DPDK:
> 
> 	static inline void
> 	rte_flow_dynf_metadata_set(struct rte_mbuf *m, uint32_t v)
> 	{
> 		...
> 
> is prefered over:
> 
> 	static inline void
> 	rte_flow_dynf_metadata_set(struct rte_mbuf *m, uint32_t v) {

Right. It is strange it passed the validator. Will fix.
> 		...
> 
> > --- a/lib/librte_mbuf/rte_mbuf_dyn.h
> > +++ b/lib/librte_mbuf/rte_mbuf_dyn.h
> > @@ -234,6 +234,10 @@ int rte_mbuf_dynflag_lookup(const char *name,
> > __rte_experimental  void rte_mbuf_dyn_dump(FILE *out);
> >
> > -/* Placeholder for dynamic fields and flags declarations. */
> > -
> > +/*
> > + * Placeholder for dynamic fields and flags declarations.
> > + * This is centralizing point to gather all field names
> > + * and parameters together.
> > + */
> > +#define MBUF_DYNF_METADATA_NAME "rte_flow_dynfield_metadata"
> >  #endif
> 
> The RTE_ prefix is missing. Also, thi name is called dynfield but it is used for
> both field and flag. I suggest RTE_MBUF_DYNFIELD_METADATA_NAME and
> RTE_MBUF_DYNFLAG_METADATA_NAME, to be consistent with the other
> naming conventions in rte_mbuf_dyn.[ch].

Well, it makes sense for complex features, say, with multiple dynaflags.
But it is OK for me, will update.

> 
> One more comment: as previously discussed, changing the size or alignement
> of a dynamic field should not be allowed, because it can break the users of
> the field.
> 
> Depending on how it is implemented (is the registration function inline?
> is the rte_mbuf_dynfield structure private, shared, or static const in a .h? are
> we using #defines for name, size, align?), I think the impact on users will be
> different. This is something we need to think about for next versions: how to
> detect these changes before pushing the commit, and/or at runtime?
> 
I'm not sure if we will have a lot of implementation invariants for dynamic fields.
These ones are intended to be used in datapath, performance is a king. I think if 
someone invent the more efficient way to handle dynafields, we'll update the
metadata code either.


> Regards,
> Olivier