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

[Olivier Matz <olivier.matz@6wind.com>]

On Tue, Oct 29, 2019 at 05:25:22PM +0100, Olivier Matz wrote:
> 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.
> 
> (...)
> 
> > +/* 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) {
> 		...
> 
> > --- 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].

I forgot: can you please document the goal/usage of these field and flag
here?  Not necessarily a detailed explanation, but a high level view:
what is transported, when it is registered, ...


> 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?
> 
> Regards,
> Olivier