DPDK patches and discussions
 help / color / mirror / Atom feed
From: Yongseok Koh <yskoh@mellanox.com>
To: Thomas Monjalon <thomas@monjalon.net>,
	Olivier Matz <olivier.matz@6wind.com>
Cc: Bruce Richardson <bruce.richardson@intel.com>,
	Andrew Rybchenko <arybchenko@solarflare.com>,
	Adrien Mazarguil <adrien.mazarguil@6wind.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: Wed, 10 Jul 2019 16:37:46 +0000	[thread overview]
Message-ID: <5D78C242-D970-4001-B8CE-268D4E444BC6@mellanox.com> (raw)
In-Reply-To: <6507101.CIctlFmaPS@xps>


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


Thanks,
Yongseok


  reply	other threads:[~2019-07-10 16:37 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 [this message]
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
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=5D78C242-D970-4001-B8CE-268D4E444BC6@mellanox.com \
    --to=yskoh@mellanox.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 \
    /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).