DPDK patches and discussions
 help / color / Atom feed
From: Andrew Rybchenko <arybchenko@solarflare.com>
To: Adrien Mazarguil <adrien.mazarguil@6wind.com>,
	Yongseok Koh <yskoh@mellanox.com>
Cc: 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: Sun, 14 Jul 2019 14:46:58 +0300
Message-ID: <4d0ad1e8-10d6-d5ff-d3b3-a94379d60662@solarflare.com> (raw)
In-Reply-To: <20190711074418.GT4512@6wind.com>

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.

> 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.
>

      reply index

Thread overview: 25+ 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-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 [this message]

Reply instructions:

You may reply publically 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=4d0ad1e8-10d6-d5ff-d3b3-a94379d60662@solarflare.com \
    --to=arybchenko@solarflare.com \
    --cc=adrien.mazarguil@6wind.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

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox