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

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

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].

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