On Sun, Jul 14, 2019 at 02:46:58PM +0300, Andrew Rybchenko wrote:
> On 11.07.2019 10:44, Adrien Mazarguil wrote:
> > On Wed, Jul 10, 2019 at 04:37:46PM +0000, Yongseok Koh wrote:
> > > > On Jul 10, 2019, at 5:26 AM, Thomas Monjalon <tho...@monjalon.net>
> > > > wrote:
> > > >
> > > > 10/07/2019 14:01, Bruce Richardson:
> > > > > 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:
> > > > > > > > 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
> > > > > > > > > @@ -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.
> > > > We could add the new field and keep the old one unused,
> > > > so it does not break the ABI.
> > > Still breaks ABI if PKT_TX_METADATA is set. :-) In order not to break it,
> > > I can
> > > keep the current union'd field (tx_metadata) as is with PKT_TX_METADATA,
> > > add
> > > the new one at the end and make it used with the new PKT_RX_METADATA.
> > >
> > > > However I suppose everybody will prefer a version using dynamic fields.
> > > > Is someone against using dynamic field for such usage?
> > > However, given that the amazing dynamic fields is coming soon (thanks for
> > > your
> > > effort, Olivier and Thomas!), I'd be honored to be the first user of it.
> > >
> > > Olivier, I'll take a look at your RFC.
> > Just got a crazy idea while reading this thread... How about repurposing
> > that "reserved" field as "rx_metadata" in the meantime?
>
> It overlaps with hash.fdir.hi which has RSS hash.
While it does overlap with hash.fdir.hi, isn't the RSS hash stored in the
"rss" field overlapping with hash.fdir.lo? (see struct rte_flow_action_rss)
hash.fdir.hi was originally used by FDIR and later repurposed by rte_flow
for its MARK action, which neatly qualifies as Rx metadata so renaming
"reserved" as "rx_metadata" could already make sense.
That is, assuming users do not need two different kinds of Rx metadata
returned simultaneously with their packets. I think it's safe.
> > I know reserved fields are cursed and no one's ever supposed to touch them
> > but this risk is mitigated by having the end user explicitly request its
> > use, so the patch author (and his relatives) should be safe from the
> > resulting bad juju.
> >
> > Joke aside, while I like the idea of Tx/Rx META, I think the similarities
> > with MARK (and TAG eventually) is a problem. I wasn't available and couldn't
> > comment when META was originally added to the Tx path, but there's a lot of
> > overlap between these items/actions, without anything explaining to the end
> > user how and why they should pick one over the other, if they can be
> > combined at all and what happens in that case.
> >
> > All this must be documented, then we should think about unifying their
> > respective features and deprecate the less capable items/actions. In my
> > opinion, users need exactly one method to mark/match some mark while
> > processing Rx/Tx traffic and *optionally* have that mark read from/written
> > to the mbuf, which may or may not be possible depending on HW features.
Thoughts regarding this suggestion? From a user perspective I think all
these actions should be unified but maybe there are good reasons to keep
them separate?
--
Adrien Mazarguil
6WIND
