From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 74B83A0487 for ; Mon, 29 Jul 2019 17:06:10 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E448B1BF71; Mon, 29 Jul 2019 17:06:09 +0200 (CEST) Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by dpdk.org (Postfix) with ESMTP id A26F51BF04 for ; Mon, 29 Jul 2019 17:06:08 +0200 (CEST) Received: by mail-wr1-f65.google.com with SMTP id g17so62242602wrr.5 for ; Mon, 29 Jul 2019 08:06:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=WAnpUFVKz7VszCfDeMNB9ESVqxVk07it6nmTHYR1jMQ=; b=aSmeRp1pp8SzeP7RvLcsPy9qS+FH3bA9C4gXO6andczxoCWRuolDWWUMGk/pY4pVpj AebF1gi4D9wOK+mXl+eEw9RgmgZBGuDWG2XD6hTlak0/Mmf3AHrLz69eGc46ao6PxoQ9 87tWS9HSMkf1AA/nyhadP91nctRB0M1b67fggH/aYeOe4jJAIjmNDEKqNxpTsiovAaAX 6aslBzQIGyAvRSvjx6hnq3hg1ZOyZqv+5bumod2tJrQJw0oDQpz0F2dQ3jw5H1Y/iDBs w0g6yYWjT+cbyfFt5g1C13gVU68E1SF0OljXxGj68iJSjl1tYDkAHwaYkcV0VvkPhiFS RLSQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=WAnpUFVKz7VszCfDeMNB9ESVqxVk07it6nmTHYR1jMQ=; b=EQ6gyRYpVQUBuSLg+cgkRKYVaKRS2DNC4p0qKF6TzbYhGhVIrRP09Ji2v/n3xA4vF+ aqJZbzNRFmrXS6bTvnN7g1N2UMESBXCcq8wUZQDIFVwjkPFIv0OdvUoNbaRZNILu7jj7 N9IrUax5kvlbdH4NC1kM7ujWfKihFAuvSRLL5RP+ODDNxpiZSyOPJKsYPRiR2wnfb4hS NsH5lmrnJ51itUaZWE+H0b6aYZX4XPrqeCeA3YyR3MpVU/WWHHcksg00i3FrOHl9o0tf 959Eoat4p5wXZkUCaunnIeCwzAaP5u/w5ENcNrck2oYNwjgzXekMbrkO3KATJ2JODtR4 0Umw== X-Gm-Message-State: APjAAAXeCNmKsUrgDiABWCHFYm7xlGLZomceLIOsx76qcP+qqgVE5iep JlrFohIfZgfxGTNoa4TqsBfU2A== X-Google-Smtp-Source: APXvYqzN8RpP9Wwt17sXeZXlo+O8CkDKbwM6UvUBzio5La8QUBTJnAFgWyqy+22zZVoZismAN2uSng== X-Received: by 2002:adf:f812:: with SMTP id s18mr50889460wrp.32.1564412768360; Mon, 29 Jul 2019 08:06:08 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id b19sm44386054wmj.13.2019.07.29.08.06.07 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 29 Jul 2019 08:06:07 -0700 (PDT) Date: Mon, 29 Jul 2019 17:06:05 +0200 From: Adrien Mazarguil To: Andrew Rybchenko Cc: Yongseok Koh , Thomas Monjalon , Olivier Matz , Bruce Richardson , Shahaf Shuler , Ferruh Yigit , dev , Slava Ovsiienko Message-ID: <20190729150605.GA4512@6wind.com> References: <20190603213231.27020-1-yskoh@mellanox.com> <20190710100743.z5ioyxish4wnh3s4@glumotte.dev.6wind.com> <20190710120128.GC505@bricha3-MOBL.ger.corp.intel.com> <6507101.CIctlFmaPS@xps> <5D78C242-D970-4001-B8CE-268D4E444BC6@mellanox.com> <20190711074418.GT4512@6wind.com> <4d0ad1e8-10d6-d5ff-d3b3-a94379d60662@solarflare.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4d0ad1e8-10d6-d5ff-d3b3-a94379d60662@solarflare.com> Subject: Re: [dpdk-dev] [PATCH] ethdev: extend flow metadata X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 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 > > > > > > > > > --- 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