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 65524A00E6 for ; Thu, 11 Jul 2019 09:44:22 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 34873397D; Thu, 11 Jul 2019 09:44:22 +0200 (CEST) Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by dpdk.org (Postfix) with ESMTP id E3DAC2C60 for ; Thu, 11 Jul 2019 09:44:20 +0200 (CEST) Received: by mail-wr1-f68.google.com with SMTP id f9so5066646wre.12 for ; Thu, 11 Jul 2019 00:44:20 -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=9yb/4wD3ywE/mdw4EIvq4LICYOkhy7wTC1qVFVrSr6g=; b=F+88I+1vqMVjaTb4IS/x0Id5xbdLxRfP6wrZhh/zPfmAQj/M02Rpo4f/sKGV4DumT9 yR8PcMcrFQDQO7O+1aBURA4hGw99ZQDOIgJ6IpNFvnIDcsAS3R5fcD7dfvNE8D/oQ575 DEuqYS4jwPJnA4+Gz+QE5+bmGOzdgrQNHX/m7LyNs/LMdz99ed1uLbMz+W69TVykx2iB wd+2C8auC9nvyLZUpbu1w74yvxSk9tpbH3B8JwU2HBHw9SoYP16mSCzXOwvydJUL5RDT fJO/sdnQTZcS4WfkBsk2YBiyrc4S5IGUPl/PIQNY4b1LeO4fpzP1YLiJITlvph+3i/pa X6Rg== 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=9yb/4wD3ywE/mdw4EIvq4LICYOkhy7wTC1qVFVrSr6g=; b=r+KlY9NSl4lCfU2/5pEgSvclHHh3VXQ6WrdEW4PrK+n9QUDsikr0oVISCHAUrr+zWk wl+wgAL9KfWNsE3el855lwA7wQMpfIFXHTmxcpnS9rgQ5LdJFGHyRU0efKhfBNFqA9fq tlUWf4Ff6YcDtI/2GtTo9DacxPGCYWeihwFBqatB0Eet9EMjYZeQ9uN58OsXEvJ7iZMr LV6w1huYkyyBellh8WkGBIrTfMECi3FuQAHq1s3RmEzfZOhAKRyIM+TJDW/KaRGHm55r E3CSuvG00ibvO7qDT8scO7XmzzWOFOxfW92JmerYVjCfAiTjGLRDfkVR9E3hMyOVpBBA Bowg== X-Gm-Message-State: APjAAAUHnWXRZpsQm3V6dcqpd997BcVCLG2Ed96jv5H55K59ZPGbK27X D6Dslfm5T2nu16nhc3MrWuuh/g== X-Google-Smtp-Source: APXvYqziGM3UuPJwrKxoEefFBw3Lgco6A7u7NOjQM/fxgqgfQRzsK2SiCmS0yPvX86Y97CW0omlXAA== X-Received: by 2002:a5d:4cc5:: with SMTP id c5mr3001302wrt.278.1562831060676; Thu, 11 Jul 2019 00:44:20 -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 c6sm4306460wma.25.2019.07.11.00.44.19 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 11 Jul 2019 00:44:19 -0700 (PDT) Date: Thu, 11 Jul 2019 09:44:18 +0200 From: Adrien Mazarguil To: Yongseok Koh Cc: Thomas Monjalon , Olivier Matz , Bruce Richardson , Andrew Rybchenko , Shahaf Shuler , Ferruh Yigit , dev , Slava Ovsiienko Message-ID: <20190711074418.GT4512@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5D78C242-D970-4001-B8CE-268D4E444BC6@mellanox.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 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? 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. -- Adrien Mazarguil 6WIND