On Wed, Jul 10, 2019 at 12:07:43PM +0200, Olivier Matz wrote:
> On Wed, Jul 10, 2019 at 10:55:34AM +0100, Bruce Richardson wrote:
> > On Wed, Jul 10, 2019 at 11:31:56AM +0200, Olivier Matz wrote:
> > > Hi Yongseok,
> > >
> > > On Thu, Jul 04, 2019 at 04:21:22PM -0700, Yongseok Koh wrote:
> > > > Currently, metadata can be set on egress path via mbuf tx_meatadata
> > > > field
> > > > with PKT_TX_METADATA flag and RTE_FLOW_ITEM_TYPE_RX_META matches
> > > > metadata.
> > > >
> > > > This patch extends the 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 packet Rx. 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 mbuf metadata field with
> > > > PKT_RX_METADATA ol_flag.
> > > >
> > > > For this purpose, mbuf->tx_metadata is moved as a separate new field and
> > > > renamed to 'metadata' to support both Rx and Tx metadata.
> > > >
> > > > For loopback/hairpin packet, metadata set on Rx/Tx may or may not be
> > > > propagated to the other path depending on HW capability.
> > > >
> > > > Signed-off-by: Yongseok Koh <ys...@mellanox.com>
> > >
> > > (...)
> > >
> > > > --- a/lib/librte_mbuf/rte_mbuf.h
> > > > +++ b/lib/librte_mbuf/rte_mbuf.h
> > > > @@ -200,6 +200,11 @@ extern "C" {
> > > >
> > > > /* add new RX flags here */
> > > >
> > > > +/**
> > > > + * Indicate that mbuf has metadata from device.
> > > > + */
> > > > +#define PKT_RX_METADATA (1ULL << 23)
> > > > +
> > > > /* add new TX flags here */
> > > >
> > > > /**
> > > > @@ -648,17 +653,6 @@ struct rte_mbuf {
> > > > /**< User defined tags. See
> > > > rte_distributor_process() */
> > > > uint32_t usr;
> > > > } hash; /**< hash information */
> > > > - struct {
> > > > - /**
> > > > - * Application specific metadata value
> > > > - * for egress flow rule match.
> > > > - * Valid if PKT_TX_METADATA is set.
> > > > - * Located here to allow conjunct use
> > > > - * with hash.sched.hi.
> > > > - */
> > > > - uint32_t tx_metadata;
> > > > - uint32_t reserved;
> > > > - };
> > > > };
> > > >
> > > > /** Outer VLAN TCI (CPU order), valid if PKT_RX_QINQ is set. */
> > > > @@ -727,6 +721,11 @@ struct rte_mbuf {
> > > > */
> > > > struct rte_mbuf_ext_shared_info *shinfo;
> > > >
> > > > + /** Application specific metadata value for flow rule match.
> > > > + * Valid if PKT_RX_METADATA or PKT_TX_METADATA is set.
> > > > + */
> > > > + uint32_t metadata;
> > > > +
> > > > } __rte_cache_aligned;
> > >
> > > This will break the ABI, so we cannot put it in 19.08, and we need a
> > > deprecation notice.
> > >
> > Does it actually break the ABI? Adding a new field to the mbuf should only
> > break the ABI if it either causes new fields to move or changes the
> > structure size. Since this is at the end, it's not going to move any older
> > fields, and since everything is cache-aligned I don't think the structure
> > size changes either.
>
> I think it does break the ABI: in previous version, when the PKT_TX_METADATA
> flag is set, the associated value is put in m->tx_metadata (offset 44 on
> x86-64), and in the next version, it will be in m->metadata (offset 112). So,
> these 2 versions are not binary compatible.
>
> Anyway, at least it breaks the API.
Ok, I misunderstood. I thought it was the structure change itself you were
saying broke the ABI. Yes, putting the data in a different place is indeed
an ABI break.
/Bruce
