From: "Yigit, Ferruh" <ferruh.yigit@linux.intel.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>,
Andrew Rybchenko <arybchenko@solarflare.com>
Cc: Yongseok Koh <yskoh@mellanox.com>,
Thomas Monjalon <thomas@monjalon.net>,
Olivier Matz <olivier.matz@6wind.com>,
Bruce Richardson <bruce.richardson@intel.com>,
Shahaf Shuler <shahafs@mellanox.com>,
Ferruh Yigit <ferruh.yigit@intel.com>, dev <dev@dpdk.org>,
Slava Ovsiienko <viacheslavo@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH] ethdev: extend flow metadata
Date: Tue, 8 Oct 2019 13:51:07 +0100 [thread overview]
Message-ID: <12d8ad96-0626-a6df-91b3-10287a08b2fd@linux.intel.com> (raw)
In-Reply-To: <20190729150605.GA4512@6wind.com>
On 7/29/2019 4:06 PM, Adrien Mazarguil wrote:
> 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 <thomas@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 <yskoh@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?
>
I think more recent plan is introducing dynamic fields for the remaining 16
bytes in the second cacheline.
I will update the patch as rejected, is there any objection?
next prev parent reply other threads:[~2019-10-08 12:51 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 [this message]
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
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=12d8ad96-0626-a6df-91b3-10287a08b2fd@linux.intel.com \
--to=ferruh.yigit@linux.intel.com \
--cc=adrien.mazarguil@6wind.com \
--cc=arybchenko@solarflare.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=olivier.matz@6wind.com \
--cc=shahafs@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).