DPDK patches and discussions
 help / color / mirror / Atom feed
From: Olivier Matz <olivier.matz@6wind.com>
To: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
Cc: dev@dpdk.org, matan@mellanox.com, rasland@mellanox.com,
	thomas@monjalon.net, Yongseok Koh <yskoh@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH v2] ethdev: extend flow metadata
Date: Fri, 18 Oct 2019 11:22:19 +0200	[thread overview]
Message-ID: <20191018092219.z5yoldgnicltypjr@platinum> (raw)
In-Reply-To: <1570723359-24650-1-git-send-email-viacheslavo@mellanox.com>

Hi Viacheslav,

Few comments on the dynamic mbuf part below.

On Thu, Oct 10, 2019 at 04:02:39PM +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>
> ---
> 
>   This patch uses dynamic mbuf field and must be applied after:
>   http://patches.dpdk.org/patch/59343/
>   "mbuf: support dynamic fields and flags"
> 
>   v2: - rebased
>       - relies on dynamic mbuf field feature
> 
>   v1: http://patches.dpdk.org/patch/56103/
> 
>   rfc: http://patches.dpdk.org/patch/54270/
> 
>  app/test-pmd/cmdline_flow.c              | 57 ++++++++++++++++++++-
>  app/test-pmd/util.c                      |  5 ++
>  doc/guides/prog_guide/rte_flow.rst       | 57 +++++++++++++++++++++
>  doc/guides/rel_notes/release_19_11.rst   |  8 +++
>  lib/librte_ethdev/rte_ethdev.h           |  1 -
>  lib/librte_ethdev/rte_ethdev_version.map |  6 +++
>  lib/librte_ethdev/rte_flow.c             | 41 +++++++++++++++
>  lib/librte_ethdev/rte_flow.h             | 87 ++++++++++++++++++++++++++++++--
>  lib/librte_mbuf/rte_mbuf_dyn.h           |  8 +++
>  9 files changed, 265 insertions(+), 5 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c
> index b26b8bf..078f256 100644
> --- a/app/test-pmd/cmdline_flow.c
> +++ b/app/test-pmd/cmdline_flow.c
> @@ -305,6 +305,9 @@ enum index {
>  	ACTION_DEC_TCP_ACK_VALUE,
>  	ACTION_RAW_ENCAP,
>  	ACTION_RAW_DECAP,
> +	ACTION_SET_META,
> +	ACTION_SET_META_DATA,
> +	ACTION_SET_META_MASK,
>  };
>  
>  /** Maximum size for pattern in struct rte_flow_item_raw. */
> @@ -994,6 +997,7 @@ struct parse_action_priv {
>  	ACTION_DEC_TCP_ACK,
>  	ACTION_RAW_ENCAP,
>  	ACTION_RAW_DECAP,
> +	ACTION_SET_META,
>  	ZERO,
>  };
>  
> @@ -1180,6 +1184,13 @@ struct parse_action_priv {
>  	ZERO,
>  };
>  
> +static const enum index action_set_meta[] = {
> +	ACTION_SET_META_DATA,
> +	ACTION_SET_META_MASK,
> +	ACTION_NEXT,
> +	ZERO,
> +};
> +
>  static int parse_set_raw_encap_decap(struct context *, const struct token *,
>  				     const char *, unsigned int,
>  				     void *, unsigned int);
> @@ -1238,6 +1249,10 @@ static int parse_vc_action_raw_encap(struct context *,
>  static int parse_vc_action_raw_decap(struct context *,
>  				     const struct token *, const char *,
>  				     unsigned int, void *, unsigned int);
> +static int parse_vc_action_set_meta(struct context *ctx,
> +				    const struct token *token, const char *str,
> +				    unsigned int len, void *buf,
> +				    unsigned int size);
>  static int parse_destroy(struct context *, const struct token *,
>  			 const char *, unsigned int,
>  			 void *, unsigned int);
> @@ -3222,7 +3237,31 @@ static int comp_vc_action_rss_queue(struct context *, const struct token *,
>  		.help = "set raw decap data",
>  		.next = NEXT(next_item),
>  		.call = parse_set_raw_encap_decap,
> -	}
> +	},
> +	[ACTION_SET_META] = {
> +		.name = "set_meta",
> +		.help = "set metadata",
> +		.priv = PRIV_ACTION(SET_META,
> +			sizeof(struct rte_flow_action_set_meta)),
> +		.next = NEXT(action_set_meta),
> +		.call = parse_vc_action_set_meta,
> +	},
> +	[ACTION_SET_META_DATA] = {
> +		.name = "data",
> +		.help = "metadata value",
> +		.next = NEXT(action_set_meta, NEXT_ENTRY(UNSIGNED)),
> +		.args = ARGS(ARGS_ENTRY_HTON
> +			     (struct rte_flow_action_set_meta, data)),
> +		.call = parse_vc_conf,
> +	},
> +	[ACTION_SET_META_MASK] = {
> +		.name = "mask",
> +		.help = "mask for metadata value",
> +		.next = NEXT(action_set_meta, NEXT_ENTRY(UNSIGNED)),
> +		.args = ARGS(ARGS_ENTRY_HTON
> +			     (struct rte_flow_action_set_meta, mask)),
> +		.call = parse_vc_conf,
> +	},
>  };
>  
>  /** Remove and return last entry from argument stack. */
> @@ -4592,6 +4631,22 @@ static int comp_vc_action_rss_queue(struct context *, const struct token *,
>  	return ret;
>  }
>  
> +static int
> +parse_vc_action_set_meta(struct context *ctx, const struct token *token,
> +			 const char *str, unsigned int len, void *buf,
> +			 unsigned int size)
> +{
> +	int ret;
> +
> +	ret = parse_vc(ctx, token, str, len, buf, size);
> +	if (ret < 0)
> +		return ret;
> +	ret = rte_flow_dynf_metadata_register();
> +	if (ret < 0)
> +		return -1;
> +	return len;
> +}
> +
>  /** Parse tokens for destroy command. */
>  static int
>  parse_destroy(struct context *ctx, const struct token *token,
> diff --git a/app/test-pmd/util.c b/app/test-pmd/util.c
> index 1570270..39ff07b 100644
> --- a/app/test-pmd/util.c
> +++ b/app/test-pmd/util.c
> @@ -81,6 +81,11 @@
>  			       mb->vlan_tci, mb->vlan_tci_outer);
>  		else if (ol_flags & PKT_RX_VLAN)
>  			printf(" - VLAN tci=0x%x", mb->vlan_tci);
> +		if (ol_flags & PKT_TX_METADATA)
> +			printf(" - Tx metadata: 0x%x", mb->tx_metadata);
> +		if (ol_flags & PKT_RX_DYNF_METADATA)
> +			printf(" - Rx metadata: 0x%x",
> +			       *RTE_FLOW_DYNF_METADATA(mb));
>  		if (mb->packet_type) {
>  			rte_get_ptype_name(mb->packet_type, buf, sizeof(buf));
>  			printf(" - hw ptype: %s", buf);
> diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
> index ff6fb11..45fc041 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
>  ~~~~~~~~~~~~~~~~~~~~~~~~
>  
> @@ -2415,6 +2441,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/release_19_11.rst b/doc/guides/rel_notes/release_19_11.rst
> index 8921cfd..904746e 100644
> --- a/doc/guides/rel_notes/release_19_11.rst
> +++ b/doc/guides/rel_notes/release_19_11.rst
> @@ -95,6 +95,14 @@ New Features
>    for specific offload features, where adding a static field or flag
>    in the mbuf is not justified.
>  
> +* **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.
> +
>  Removed Items
>  -------------
>  
> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d937fb4..9a6432c 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -1013,7 +1013,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)
> diff --git a/lib/librte_ethdev/rte_ethdev_version.map b/lib/librte_ethdev/rte_ethdev_version.map
> index 6df42a4..3d9cafc 100644
> --- a/lib/librte_ethdev/rte_ethdev_version.map
> +++ b/lib/librte_ethdev/rte_ethdev_version.map
> @@ -283,4 +283,10 @@ EXPERIMENTAL {
>  
>  	# added in 19.08
>  	rte_eth_read_clock;
> +
> +	# added in 19.11
> +	rte_flow_dynf_metadata_offs;
> +	rte_flow_dynf_metadata_flag;
> +	rte_flow_dynf_metadata_avail;
> +	rte_flow_dynf_metadata_register;
>  };
> diff --git a/lib/librte_ethdev/rte_flow.c b/lib/librte_ethdev/rte_flow.c
> index cc03b15..9cbda75 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.
>   */
> @@ -153,8 +161,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 = MBUF_DYNF_METADATA_SIZE,
> +		.align = MBUF_DYNF_METADATA_ALIGN,
> +		.flags = MBUF_DYNF_METADATA_FLAGS,
> +	};
> +	static const struct rte_mbuf_dynflag desc_flag = {
> +		.name = MBUF_DYNF_METADATA_NAME,
> +	};

I don't see think we need #defines.
You can directly use the name, sizeof() and __alignof__() here.
If the information is used externally, the structure shall
be made global non-static.


> +
> +	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:
> +	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 391a44a..a27e619 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -27,6 +27,8 @@
>  #include <rte_udp.h>
>  #include <rte_byteorder.h>
>  #include <rte_esp.h>
> +#include <rte_mbuf.h>
> +#include <rte_mbuf_dyn.h>
>  
>  #ifdef __cplusplus
>  extern "C" {
> @@ -417,7 +419,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,
> @@ -1213,9 +1216,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
>   *
> - * 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;
> @@ -1813,6 +1824,13 @@ enum rte_flow_action_type {
>  	 * undefined behavior.
>  	 */
>  	RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK,
> +
> +	/**
> +	 * Set metadata on ingress or egress path.
> +	 *
> +	 * See struct rte_flow_action_set_meta.
> +	 */
> +	RTE_FLOW_ACTION_TYPE_SET_META,
>  };
>  
>  /**
> @@ -2300,6 +2318,43 @@ 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;
> +};
> +
> +/* 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)
> +

I wonder if helpers like this wouldn't be better, because
they combine the flag and the field:

/**
 * Set metadata dynamic field and flag in mbuf.
 *
 * rte_flow_dynf_metadata_register() must have been called first.
 */
__rte_experimental
static inline void rte_mbuf_dyn_metadata_set(struct rte_mbuf *m,
                                       uint32_t metadata)
{
       *RTE_MBUF_DYNFIELD(m, rte_flow_dynf_metadata_offs,
                       uint32_t *) = metadata;
       m->ol_flags |= rte_flow_dynf_metadata_mask;
}

/**
 * Get metadata dynamic field value in mbuf.
 *
 * rte_flow_dynf_metadata_register() must have been called first.
 */
__rte_experimental
static inline int rte_mbuf_dyn_metadata_get(const struct rte_mbuf *m,
                                       uint32_t *metadata)
{
       if ((m->ol_flags & rte_flow_dynf_metadata_mask) == 0)
               return -1;
       *metadata = *RTE_MBUF_DYNFIELD(m, rte_flow_dynf_metadata_offs,
                               uint32_t *);
       return 0;
}

/**
 * Delete the metadata dynamic flag in mbuf.
 *
 * rte_flow_dynf_metadata_register() must have been called first.
 */
__rte_experimental
static inline void rte_mbuf_dyn_metadata_del(struct rte_mbuf *m)
{
       m->ol_flags &= ~rte_flow_dynf_metadata_mask;
}


>  /*
>   * Definition of a single action.
>   *
> @@ -2533,6 +2588,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) {
> +	return !!rte_flow_dynf_metadata_mask;
> +}

_registered() instead of _avail() ?

> +
> +/**
> + * 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 6e2c816..4ff33ac 100644
> --- a/lib/librte_mbuf/rte_mbuf_dyn.h
> +++ b/lib/librte_mbuf/rte_mbuf_dyn.h
> @@ -160,4 +160,12 @@ int rte_mbuf_dynflag_lookup(const char *name,
>   */
>  #define RTE_MBUF_DYNFIELD(m, offset, type) ((type)((uintptr_t)(m) + (offset)))
>  
> +/**
> + * Flow metadata dynamic field definitions.
> + */
> +#define MBUF_DYNF_METADATA_NAME "flow-metadata"
> +#define MBUF_DYNF_METADATA_SIZE sizeof(uint32_t)
> +#define MBUF_DYNF_METADATA_ALIGN __alignof__(uint32_t)
> +#define MBUF_DYNF_METADATA_FLAGS 0

If this flag is only to be used in rte_flow, it can stay in rte_flow.
The name should follow the function name conventions, I suggest
"rte_flow_metadata".

If the flag is going to be used in several places in dpdk (rte_flow,
pmd, app, ...), I wonder if it shouldn't be defined it in
rte_mbuf_dyn.c. I mean:

====
/* rte_mbuf_dyn.c */
const struct rte_mbuf_dynfield rte_mbuf_dynfield_flow_metadata = {
   ...
};
int rte_mbuf_dynfield_flow_metadata_offset = -1;
const struct rte_mbuf_dynflag rte_mbuf_dynflag_flow_metadata = {
   ...
};
int rte_mbuf_dynflag_flow_metadata_bitnum = -1;

int rte_mbuf_dyn_flow_metadata_register(void)
{
...
}

/* rte_mbuf_dyn.h */
extern const struct rte_mbuf_dynfield rte_mbuf_dynfield_flow_metadata;
extern int rte_mbuf_dynfield_flow_metadata_offset;
extern const struct rte_mbuf_dynflag rte_mbuf_dynflag_flow_metadata;
extern int rte_mbuf_dynflag_flow_metadata_bitnum;

...helpers to set/get metadata...
===

Centralizing the definitions of non-private dynamic fields/flags in
rte_mbuf_dyn may help other people to reuse a field that is well
described if it match their use-case.

In your case, what is carried by metadata? Could it be reused by
others? I think some more description is needed.


Regards,
Olivier

  reply	other threads:[~2019-10-18  9:22 UTC|newest]

Thread overview: 98+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-03 21:32 [dpdk-dev] [RFC 1/3] " Yongseok Koh
2019-06-03 21:32 ` [dpdk-dev] [RFC 2/3] ethdev: add flow modify mark action Yongseok Koh
2019-06-06 10:35   ` Jerin Jacob Kollanukkaran
2019-06-06 18:33     ` Yongseok Koh
2019-06-03 21:32 ` [dpdk-dev] [RFC 3/3] ethdev: add flow tag Yongseok Koh
2019-07-04 23:23   ` [dpdk-dev] [PATCH] " Yongseok Koh
2019-07-05 13:54     ` Adrien Mazarguil
2019-07-05 18:05       ` Yongseok Koh
2019-07-08 23:32         ` Yongseok Koh
2019-07-09  8:38         ` Adrien Mazarguil
2019-07-11  1:59           ` Yongseok Koh
2019-10-08 12:57             ` Yigit, Ferruh
2019-10-08 13:18               ` Slava Ovsiienko
2019-10-10 16:09     ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
2019-10-24 13:12       ` [dpdk-dev] [PATCH v3] " Viacheslav Ovsiienko
2019-10-27 16:38         ` Ori Kam
2019-10-27 18:42         ` [dpdk-dev] [PATCH v4] " Viacheslav Ovsiienko
2019-10-27 19:11           ` Ori Kam
2019-10-31 18:57             ` Ferruh Yigit
2019-06-09 14:23 ` [dpdk-dev] [RFC 1/3] ethdev: extend flow metadata Andrew Rybchenko
2019-06-10  3:19   ` Wang, Haiyue
2019-06-10  7:20     ` Andrew Rybchenko
2019-06-11  0:06       ` Yongseok Koh
2019-06-19  9:05         ` Andrew Rybchenko
2019-07-04 23:21 ` [dpdk-dev] [PATCH] " Yongseok Koh
2019-07-10  9:31   ` Olivier Matz
2019-07-10  9:55     ` Bruce Richardson
2019-07-10 10:07       ` Olivier Matz
2019-07-10 12:01         ` Bruce Richardson
2019-07-10 12:26           ` Thomas Monjalon
2019-07-10 16:37             ` Yongseok Koh
2019-07-11  7:44               ` Adrien Mazarguil
2019-07-14 11:46                 ` Andrew Rybchenko
2019-07-29 15:06                   ` Adrien Mazarguil
2019-10-08 12:51                     ` Yigit, Ferruh
2019-10-08 13:17                       ` Slava Ovsiienko
2019-10-10 16:02   ` [dpdk-dev] [PATCH v2] " Viacheslav Ovsiienko
2019-10-18  9:22     ` Olivier Matz [this message]
2019-10-19 19:47       ` Slava Ovsiienko
2019-10-21 16:37         ` Olivier Matz
2019-10-24  6:49           ` Slava Ovsiienko
2019-10-24  9:22             ` Olivier Matz
2019-10-24 12:30               ` Slava Ovsiienko
2019-10-24 13:08     ` [dpdk-dev] [PATCH v3] " Viacheslav Ovsiienko
2019-10-27 16:56       ` Ori Kam
2019-10-27 18:40       ` [dpdk-dev] [PATCH v4] " Viacheslav Ovsiienko
2019-10-27 19:10         ` Ori Kam
2019-10-29 16:22         ` Andrew Rybchenko
2019-10-29 17:19           ` Slava Ovsiienko
2019-10-29 18:30             ` Thomas Monjalon
2019-10-29 18:35               ` Slava Ovsiienko
2019-10-30  6:28               ` Andrew Rybchenko
2019-10-30  7:35             ` Andrew Rybchenko
2019-10-30  8:59               ` Slava Ovsiienko
2019-10-30  9:20                 ` Andrew Rybchenko
2019-10-30 10:05                   ` Slava Ovsiienko
2019-10-30 10:03                 ` Slava Ovsiienko
2019-10-30 15:49               ` Olivier Matz
2019-10-31  9:25                 ` Andrew Rybchenko
2019-10-29 16:25         ` Olivier Matz
2019-10-29 16:33           ` Olivier Matz
2019-10-29 17:53             ` Slava Ovsiienko
2019-10-29 17:43           ` Slava Ovsiienko
2019-10-29 19:31         ` [dpdk-dev] [PATCH v5] " Viacheslav Ovsiienko
2019-10-30  8:02           ` Andrew Rybchenko
2019-10-30 14:40             ` Slava Ovsiienko
2019-10-30 14:46               ` Slava Ovsiienko
2019-10-30 15:20                 ` Olivier Matz
2019-10-30 15:57                   ` Thomas Monjalon
2019-10-30 15:58                   ` Slava Ovsiienko
2019-10-30 16:13                     ` Olivier Matz
2019-10-30  8:35           ` Ori Kam
2019-10-30 17:12           ` [dpdk-dev] [PATCH v6 0/2] extend flow metadata feature Viacheslav Ovsiienko
2019-10-30 17:12             ` [dpdk-dev] [PATCH v6 1/2] ethdev: extend flow metadata Viacheslav Ovsiienko
2019-10-31  9:19               ` Andrew Rybchenko
2019-10-31 13:05               ` [dpdk-dev] [PATCH v7 0/2] extend flow metadata feature Viacheslav Ovsiienko
2019-10-31 13:05                 ` [dpdk-dev] [PATCH v7 1/2] ethdev: extend flow metadata Viacheslav Ovsiienko
2019-10-31 15:47                   ` Olivier Matz
2019-10-31 16:13                     ` Slava Ovsiienko
2019-10-31 16:48                   ` [dpdk-dev] [PATCH v8 0/2] extend flow metadata feature Viacheslav Ovsiienko
2019-10-31 16:48                     ` [dpdk-dev] [PATCH v8 1/2] ethdev: extend flow metadata Viacheslav Ovsiienko
2019-11-04  6:13                       ` [dpdk-dev] [PATCH v9 0/2] extend flow metadata feature Viacheslav Ovsiienko
2019-11-04  6:13                         ` [dpdk-dev] [PATCH v9 1/2] ethdev: extend flow metadata Viacheslav Ovsiienko
2019-11-05 14:19                           ` [dpdk-dev] [PATCH v10 0/2] extend flow metadata feature Viacheslav Ovsiienko
2019-11-05 14:19                             ` [dpdk-dev] [PATCH v10 1/2] ethdev: extend flow metadata Viacheslav Ovsiienko
2019-11-05 14:19                             ` [dpdk-dev] [PATCH v10 2/2] ethdev: move egress metadata to dynamic field Viacheslav Ovsiienko
2019-11-06 15:49                             ` [dpdk-dev] [PATCH v10 0/2] extend flow metadata feature Ferruh Yigit
2019-11-04  6:13                         ` [dpdk-dev] [PATCH v9 2/2] ethdev: move egress metadata to dynamic field Viacheslav Ovsiienko
2019-10-31 16:48                     ` [dpdk-dev] [PATCH v8 " Viacheslav Ovsiienko
2019-10-31 17:21                       ` Olivier Matz
2019-11-01 12:34                       ` Andrew Rybchenko
2019-10-31 13:05                 ` [dpdk-dev] [PATCH v7 " Viacheslav Ovsiienko
2019-10-31 13:33                   ` Ori Kam
2019-10-31 15:51                   ` Olivier Matz
2019-10-31 16:07                     ` Slava Ovsiienko
2019-10-30 17:12             ` [dpdk-dev] [PATCH v6 " Viacheslav Ovsiienko
2019-10-31  9:01               ` Andrew Rybchenko
2019-10-31 10:54                 ` Slava Ovsiienko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191018092219.z5yoldgnicltypjr@platinum \
    --to=olivier.matz@6wind.com \
    --cc=dev@dpdk.org \
    --cc=matan@mellanox.com \
    --cc=rasland@mellanox.com \
    --cc=thomas@monjalon.net \
    --cc=viacheslavo@mellanox.com \
    --cc=yskoh@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).