Hi, Olivier
Thanks a lot for the review.
> -----Original Message-----
> From: Olivier Matz <olivier.m...@6wind.com>
> Sent: Tuesday, October 29, 2019 18:25
> To: Slava Ovsiienko <viachesl...@mellanox.com>
> Cc: dev@dpdk.org; Thomas Monjalon <tho...@monjalon.net>; Matan
> Azrad <ma...@mellanox.com>; Ori Kam <or...@mellanox.com>; Yongseok
> Koh <ys...@mellanox.com>
> Subject: Re: [PATCH v4] ethdev: extend flow metadata
>
> 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.
Right, will fix.
>
> (...)
>
> > +/* 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) {
Right. It is strange it passed the validator. Will fix.
> ...
>
> > --- 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].
Well, it makes sense for complex features, say, with multiple dynaflags.
But it is OK for me, will update.
>
> 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?
>
I'm not sure if we will have a lot of implementation invariants for dynamic
fields.
These ones are intended to be used in datapath, performance is a king. I think
if
someone invent the more efficient way to handle dynafields, we'll update the
metadata code either.
> Regards,
> Olivier
