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 558AFA00E6 for ; Wed, 10 Jul 2019 14:01:38 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 4026D2C60; Wed, 10 Jul 2019 14:01:37 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id C052E1C01 for ; Wed, 10 Jul 2019 14:01:35 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Jul 2019 05:01:34 -0700 X-IronPort-AV: E=Sophos;i="5.63,474,1557212400"; d="scan'208";a="249449007" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.51]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Jul 2019 05:01:31 -0700 Date: Wed, 10 Jul 2019 13:01:29 +0100 From: Bruce Richardson To: Olivier Matz Cc: Yongseok Koh , shahafs@mellanox.com, thomas@monjalon.net, ferruh.yigit@intel.com, arybchenko@solarflare.com, adrien.mazarguil@6wind.com, dev@dpdk.org, viacheslavo@mellanox.com Message-ID: <20190710120128.GC505@bricha3-MOBL.ger.corp.intel.com> References: <20190603213231.27020-1-yskoh@mellanox.com> <20190704232122.19477-1-yskoh@mellanox.com> <20190710093156.va3rk5jmz4oj7hfx@glumotte.dev.6wind.com> <20190710095533.GA505@bricha3-MOBL.ger.corp.intel.com> <20190710100743.z5ioyxish4wnh3s4@glumotte.dev.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190710100743.z5ioyxish4wnh3s4@glumotte.dev.6wind.com> User-Agent: Mutt/1.11.4 (2019-03-13) 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 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: > > > Hi Yongseok, > > > > > > 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 > > > > @@ -200,6 +200,11 @@ extern "C" { > > > > > > > > /* add new RX flags here */ > > > > > > > > +/** > > > > + * Indicate that mbuf has metadata from device. > > > > + */ > > > > +#define PKT_RX_METADATA (1ULL << 23) > > > > + > > > > /* add new TX flags here */ > > > > > > > > /** > > > > @@ -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. /Bruce