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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 10/27/19 9:40 PM, 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>

Above explanations lack information about "meta" vs "mark" which
may be set on Rx as well and delivered in other mbuf field.
It should be explained by one more field is required and rules
defined. Otherwise we can endup in half PMDs supporting
mark only, half PMDs supporting meta only and applications
in an interesting situation to make a choice which one to use.

[snip]

> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index 159ce19..c943aca 100644
> --- a/doc/guides/prog_guide/rte_flow.rst
> +++ b/doc/guides/prog_guide/rte_flow.rst
> @@ -658,6 +658,32 @@ the physical device, with virtual groups in the PMD or not at all.
>      | ``mask`` | ``id``   | zeroed to match any value |
>      +----------+----------+---------------------------+
>   
> +Item: ``META``
> +^^^^^^^^^^^^^^^^^
> +
> +Matches 32 bit metadata item set.
> +
> +On egress, metadata can be set either by mbuf metadata field with
> +PKT_TX_METADATA flag or ``SET_META`` action. On ingress, ``SET_META``
> +action sets metadata for a packet and the metadata will be reported via
> +``metadata`` dynamic field of ``rte_mbuf`` with PKT_RX_DYNF_METADATA flag.
> +
> +- Default ``mask`` matches the specified Rx metadata value.
> +
> +.. _table_rte_flow_item_meta:
> +
> +.. table:: META
> +
> +   +----------+----------+---------------------------------------+
> +   | Field    | Subfield | Value                                 |
> +   +==========+==========+=======================================+
> +   | ``spec`` | ``data`` | 32 bit metadata value                 |
> +   +----------+----------+---------------------------------------+
> +   | ``last`` | ``data`` | upper range value                     |
> +   +----------+----------+---------------------------------------+
> +   | ``mask`` | ``data`` | bit-mask applies to "spec" and "last" |
> +   +----------+----------+---------------------------------------+
> +
>   Data matching item types
>   ~~~~~~~~~~~~~~~~~~~~~~~~
>   
> @@ -1232,21 +1258,6 @@ Matches a PPPoE session protocol identifier.
>   - ``proto_id``: PPP protocol identifier.
>   - Default ``mask`` matches proto_id only.
>   
> -
> -.. _table_rte_flow_item_meta:
> -
> -.. table:: META
> -
> -   +----------+----------+---------------------------------------+
> -   | Field    | Subfield | Value                                 |
> -   +==========+==========+=======================================+
> -   | ``spec`` | ``data`` | 32 bit metadata value                 |
> -   +----------+--------------------------------------------------+
> -   | ``last`` | ``data`` | upper range value                     |
> -   +----------+----------+---------------------------------------+
> -   | ``mask`` | ``data`` | bit-mask applies to "spec" and "last" |
> -   +----------+----------+---------------------------------------+
> -
>   Item: ``NSH``
>   ^^^^^^^^^^^^^
>   
> @@ -2466,6 +2477,37 @@ Value to decrease TCP acknowledgment number by is a big-endian 32 bit integer.
>   
>   Using this action on non-matching traffic will result in undefined behavior.
>   
> +Action: ``SET_META``
> +^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Set metadata. Item ``META`` matches metadata.
> +
> +Metadata set by mbuf metadata field with PKT_TX_METADATA flag on egress will be
> +overridden by this action. On ingress, the metadata will be carried by
> +``metadata`` dynamic field of ``rte_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.
> +
> +Altering partial bits is supported with ``mask``. For bits which have never been
> +set, unpredictable value will be seen depending on driver implementation. For
> +loopback/hairpin packet, metadata set on Rx/Tx may or may not be propagated to
> +the other path depending on HW capability.
> +
> +.. _table_rte_flow_action_set_meta:
> +
> +.. table:: SET_META
> +
> +   +----------+----------------------------+
> +   | Field    | Value                      |
> +   +==========+============================+
> +   | ``data`` | 32 bit metadata value      |
> +   +----------+----------------------------+
> +   | ``mask`` | bit-mask applies to "data" |
> +   +----------+----------------------------+
> +
>   Negative types
>   ~~~~~~~~~~~~~~
>   
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 3aa1634..9d54d8e 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -106,6 +106,10 @@ Deprecation Notices
>     struct ``rte_eth_dev_info`` for the port capability and in struct
>     ``rte_eth_rxmode`` for the port configuration.
>   
> +* ethdev: DEV_TX_OFFLOAD_MATCH_METADATA will be removed, static metadata
> +  mbuf field will be removed in 20.02, metadata feature will use dynamic mbuf
> +  field and flag instead.
> +

Isn't it breaking stable API/ABI? Should target release be 20.11?

I think that DEV_TX_OFFLOAD_MATCH_METADATA should marked
as deprecated now as well as tx_metadata field in mbuf.

>   * cryptodev: support for using IV with all sizes is added, J0 still can
>     be used but only when IV length in following structs ``rte_crypto_auth_xform``,
>     ``rte_crypto_aead_xform`` is set to zero. When IV length is greater or equal
> diff --git a/doc/guides/rel_notes/release_19_11.rst b/doc/guides/rel_notes/release_19_11.rst
> index 0e5bb5d..6d331f6 100644
> --- a/doc/guides/rel_notes/release_19_11.rst
> +++ b/doc/guides/rel_notes/release_19_11.rst
> @@ -232,6 +232,21 @@ New Features
>       gives ability to print port supported ptypes in different protocol layers.
>   
>   
> +* **Add support of support dynamic fields and flags in mbuf.**
> +
> +  This new feature adds the ability to dynamically register some room
> +  for a field or a flag in the mbuf structure. This is typically used
> +  for specific offload features, where adding a static field or flag
> +  in the mbuf is not justified.
> +

I guess above is just incorrect merge.

> +* **Extended metadata support in rte_flow.**
> +
> +  Flow metadata is extended to both Rx and Tx.
> +
> +  * Tx metadata can also be set by SET_META action of rte_flow.
> +  * Rx metadata is delivered to host via a dynamic field of ``rte_mbuf`` with
> +    PKT_RX_DYNF_METADATA.
> +

Two empty lines are required before the next section.

>   Removed Items
>   -------------
>   
> 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
> -

Unrelated change.

>   #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
>   				 DEV_RX_OFFLOAD_UDP_CKSUM | \
>   				 DEV_RX_OFFLOAD_TCP_CKSUM)

[snip]

> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index ca0f680..6090177 100644
> --- a/lib/librte_ethdev/rte_flow.c
> +++ b/lib/librte_ethdev/rte_flow.c
> @@ -12,10 +12,18 @@
>   #include <rte_errno.h>
>   #include <rte_branch_prediction.h>
>   #include <rte_string_fns.h>
> +#include <rte_mbuf.h>
> +#include <rte_mbuf_dyn.h>
>   #include "rte_ethdev.h"
>   #include "rte_flow_driver.h"
>   #include "rte_flow.h"
>   
> +/* Mbuf dynamic field name for metadata. */
> +int rte_flow_dynf_metadata_offs = -1;
> +
> +/* Mbuf dynamic field flag bit number for metadata. */
> +uint64_t rte_flow_dynf_metadata_mask;
> +
>   /**
>    * Flow elements description tables.
>    */
> @@ -157,8 +165,41 @@ struct rte_flow_desc_data {
>   	MK_FLOW_ACTION(DEC_TCP_SEQ, sizeof(rte_be32_t)),
>   	MK_FLOW_ACTION(INC_TCP_ACK, sizeof(rte_be32_t)),
>   	MK_FLOW_ACTION(DEC_TCP_ACK, sizeof(rte_be32_t)),
> +	MK_FLOW_ACTION(SET_META, sizeof(struct rte_flow_action_set_meta)),
>   };
>   
> +int
> +rte_flow_dynf_metadata_register(void)
> +{
> +	int offset;
> +	int flag;
> +
> +	static const struct rte_mbuf_dynfield desc_offs = {
> +		.name = MBUF_DYNF_METADATA_NAME,
> +		.size = sizeof(uint32_t),
> +		.align = __alignof__(uint32_t),
> +		.flags = 0,

I think flags initialization to 0 is redundant.

> +	};
> +	static const struct rte_mbuf_dynflag desc_flag = {
> +		.name = MBUF_DYNF_METADATA_NAME,
> +	};
> +
> +	offset = rte_mbuf_dynfield_register(&desc_offs);
> +	if (offset < 0)
> +		goto error;
> +	flag = rte_mbuf_dynflag_register(&desc_flag);
> +	if (flag < 0)
> +		goto error;
> +	rte_flow_dynf_metadata_offs = offset;
> +	rte_flow_dynf_metadata_mask = (1ULL << flag);
> +	return 0;
> +
> +error:

Just an observation...
Impossibility to unregister hits here. Field may be registered,
but will be used.

> +	rte_flow_dynf_metadata_offs = -1;
> +	rte_flow_dynf_metadata_mask = 0ULL;
> +	return -rte_errno;
> +}
> +
>   static int
>   flow_err(uint16_t port_id, int ret, struct rte_flow_error *error)
>   {
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 4fee105..b821557 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -28,6 +28,8 @@
>   #include <rte_byteorder.h>
>   #include <rte_esp.h>
>   #include <rte_higig.h>
> +#include <rte_mbuf.h>
> +#include <rte_mbuf_dyn.h>
>   
>   #ifdef __cplusplus
>   extern "C" {
> @@ -418,7 +420,8 @@ enum rte_flow_item_type {
>   	/**
>   	 * [META]
>   	 *
> -	 * Matches a metadata value specified in mbuf metadata field.
> +	 * Matches a metadata value.
> +	 *
>   	 * See struct rte_flow_item_meta.
>   	 */
>   	RTE_FLOW_ITEM_TYPE_META,
> @@ -1263,9 +1266,17 @@ struct rte_flow_item_icmp6_nd_opt_tla_eth {
>   #endif
>   
>   /**
> - * RTE_FLOW_ITEM_TYPE_META.
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice

Is it allowed to make experimental back?

>    *
> - * Matches a specified metadata value.
> + * RTE_FLOW_ITEM_TYPE_META
> + *
> + * Matches a specified metadata value. On egress, metadata can be set either by
> + * mbuf tx_metadata field with PKT_TX_METADATA flag or
> + * RTE_FLOW_ACTION_TYPE_SET_META. On ingress, RTE_FLOW_ACTION_TYPE_SET_META sets
> + * metadata for a packet and the metadata will be reported via mbuf metadata
> + * dynamic field with PKT_RX_DYNF_METADATA flag. The dynamic mbuf field must be
> + * registered in advance by rte_flow_dynf_metadata_register().
>    */
>   struct rte_flow_item_meta {
>   	rte_be32_t data;

[snip]

> @@ -2429,6 +2447,55 @@ struct rte_flow_action_set_mac {
>   	uint8_t mac_addr[RTE_ETHER_ADDR_LEN];
>   };
>   
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this structure may change without prior notice
> + *
> + * RTE_FLOW_ACTION_TYPE_SET_META
> + *
> + * Set metadata. Metadata set by mbuf tx_metadata field with
> + * PKT_TX_METADATA flag on egress will be overridden by this action. On
> + * ingress, the metadata will be carried by mbuf metadata dynamic field
> + * with PKT_RX_DYNF_METADATA flag if set.  The dynamic mbuf field must be
> + * registered in advance by rte_flow_dynf_metadata_register().
> + *
> + * Altering partial bits is supported with mask. For bits which have never
> + * been set, unpredictable value will be seen depending on driver
> + * implementation. For loopback/hairpin packet, metadata set on Rx/Tx may
> + * or may not be propagated to the other path depending on HW capability.
> + *
> + * RTE_FLOW_ITEM_TYPE_META matches metadata.
> + */
> +struct rte_flow_action_set_meta {
> +	rte_be32_t data;
> +	rte_be32_t mask;

As I understand tx_metadata is host endian. Just double-checking.
Is a new dynamic field host endian or big endian?
I definitely would like to see motivation in comments why data/mask
are big-endian 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;

These two global variables look frightening to me.
It does not look good to me.

> +
> +/* 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) {

Above curly bracket should be on its own line in the case of function
definition.

Shouldn't m be 'const struct rte_mbuf *'?

> +	return *RTE_FLOW_DYNF_METADATA(m);
> +}
> +
> +__rte_experimental
> +static inline void
> +rte_flow_dynf_metadata_set(struct rte_mbuf *m, uint32_t v) {

Above curly bracket should be on its own line in the case of function
definition.

> +	*RTE_FLOW_DYNF_METADATA(m) = v;
> +}
> +
>   /*
>    * Definition of a single action.
>    *
> @@ -2662,6 +2729,32 @@ enum rte_flow_conv_op {
>   };
>   
>   /**
> + * Check if mbuf dynamic field for metadata is registered.
> + *
> + * @return
> + *   True if registered, false otherwise.
> + */
> +__rte_experimental
> +static inline int
> +rte_flow_dynf_metadata_avail(void) {

Above curly bracket should be on its own line in the case of function
definition.

> +	return !!rte_flow_dynf_metadata_mask;
> +}
> +
> +/**
> + * Register mbuf dynamic field and flag for metadata.
> + *
> + * This function must be called prior to use SET_META action in order to
> + * register the dynamic mbuf field. Otherwise, the data cannot be delivered to
> + * application.
> + *
> + * @return
> + *   0 on success, a negative errno value otherwise and rte_errno is set.
> + */
> +__rte_experimental
> +int
> +rte_flow_dynf_metadata_register(void);
> +
> +/**
>    * Check whether a flow rule can be created on a given port.
>    *
>    * The flow rule is validated for correctness and whether it could be accepted
> diff --git a/lib/librte_mbuf/rte_mbuf_dyn.h b/lib/librte_mbuf/rte_mbuf_dyn.h
> index 2e9d418..a4a0cf5 100644
> --- 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.
> + */

It is not a comment for below define. So, I think empty line is
required to separate the comment from below define.
I'm not sure that the clarification is required, but it is up to Olivier.

> +#define MBUF_DYNF_METADATA_NAME "rte_flow_dynfield_metadata"

Empty line is missing here

>   #endif