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 <ys...@mellanox.com>
> Signed-off-by: Viacheslav Ovsiienko <viachesl...@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
