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 060FEA0471 for ; Sun, 14 Jul 2019 13:47:28 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C830A3772; Sun, 14 Jul 2019 13:47:27 +0200 (CEST) Received: from dispatch1-us1.ppe-hosted.com (dispatch1-us1.ppe-hosted.com [148.163.129.52]) by dpdk.org (Postfix) with ESMTP id 26060326C for ; Sun, 14 Jul 2019 13:47:26 +0200 (CEST) X-Virus-Scanned: Proofpoint Essentials engine Received: from webmail.solarflare.com (uk.solarflare.com [193.34.186.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by mx1-us5.ppe-hosted.com (PPE Hosted ESMTP Server) with ESMTPS id 9A5C4A40050; Sun, 14 Jul 2019 11:47:23 +0000 (UTC) Received: from [192.168.1.11] (85.187.13.152) by ukex01.SolarFlarecom.com (10.17.10.4) with Microsoft SMTP Server (TLS) id 15.0.1395.4; Sun, 14 Jul 2019 12:47:14 +0100 To: Adrien Mazarguil , Yongseok Koh CC: Thomas Monjalon , Olivier Matz , Bruce Richardson , Shahaf Shuler , Ferruh Yigit , dev , Slava Ovsiienko 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> From: Andrew Rybchenko Message-ID: <4d0ad1e8-10d6-d5ff-d3b3-a94379d60662@solarflare.com> Date: Sun, 14 Jul 2019 14:46:58 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: <20190711074418.GT4512@6wind.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-Originating-IP: [85.187.13.152] X-ClientProxiedBy: ocex03.SolarFlarecom.com (10.20.40.36) To ukex01.SolarFlarecom.com (10.17.10.4) X-TM-AS-Product-Ver: SMEX-12.5.0.1300-8.5.1010-24758.003 X-TM-AS-Result: No-19.892500-8.000000-10 X-TMASE-MatchedRID: I31hiQfYWUPmLzc6AOD8DfHkpkyUphL9amDMhjMSdnkd0WOKRkwsh8IL i3uRdrKWdYxO9VuCM39tf8hHzBZNqLa6/BliGsjAgLf5GeDY2qMGchEhVwJY37FbhhcWxNQOvuF UoBce/NZ/XFK38Ckccw5Hz/vrc8GbqWJyP5BjvgjFW296Y1uTJ3316REeU5CR1y0aXF5eX+itab Krs9q4eUD7xtpsEgiPUVDDZpJlenCqWlVVAQZSvorkmrf0Igi/8dkWy9OCQOF96o8/Nc1oda+O3 p9xFzgDC8E8ZCgtbqVxXkyaE+0EVO57OgItHYSZiJwEp8weVXwrU8f3oY88YCiESHbRTU0WNLWb niBQvReyz/ty7ioMZPrZB0QmVq0yfTF5ARmYvFZuh7qwx+D6T3zjW7ke067QwubD3SFbWztTJ1P 9gaH6FN5UYPZKA/Qlq4Izyz/80+Hyy47Gi4RqGguLP4ROdWHViJ4iimOaq6GvI8MkS87SyjAq87 iL2fL4q+XsKscAOn3RIV1p+3wDut8EQEhbTzxhqg0gbtLVIa/GSb7RikwvVy6Zl5fVYhDw5R82E 5I3a7zorI9ZI3oMzBfRej1lriKkrGcdWfiKiyYsIMJqMNlanYD9w87C9woFFBQ5IKls/A6nZgID Mjh+2JPHTShWvOUtfNX/oG7PlumzTxI0TzvD+8zSKGx9g8xhOyBTDrxRCtibkEl1SMP4VZaWjU7 JwH26IQVF+24TS6FKhpMV+QFOCkElpwBtHoIxngIgpj8eDcC063Wh9WVqgtQdB5NUNSsi1GcRAJ RT6POOhzOa6g8KrZRMZUCEHkRt X-TM-AS-User-Approved-Sender: Yes X-TM-AS-User-Blocked-Sender: No X-TMASE-Result: 10--19.892500-8.000000 X-TMASE-Version: SMEX-12.5.0.1300-8.5.1010-24758.003 X-MDID: 1563104845-uxzJBCLEFosT 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 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. > 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. >