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 49658A00BE; Tue, 29 Oct 2019 17:25:26 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E48801BED5; Tue, 29 Oct 2019 17:25:25 +0100 (CET) Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by dpdk.org (Postfix) with ESMTP id A04141BEAF for ; Tue, 29 Oct 2019 17:25:24 +0100 (CET) Received: by mail-wr1-f66.google.com with SMTP id q13so14311380wrs.12 for ; Tue, 29 Oct 2019 09:25:24 -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:user-agent; bh=h02DuQUATASqZOhVwoK9KiCCTwxtbqKQGJsC1I90g0E=; b=ch73tCH8Mx4BudJiuswjyfHMP5yBXSEiQcD80/D22OhihsnFVsCkC8qlj4O2JXOCEQ s0zcrTfUDc37uCA/EncsmPA752ZK/sjuO7Kt9sIKHCjTTgQxWIsKv4N4iSN9Lrdxshqa f2Sncl7Y08NYrg30+0A+GIeWcXqZB6jzW6BAt6on31TCZrd/kgQNk7c8O94VUO0L2acn M1EvXVAC97r/4CINEK8deqhrjspW8q6GgSBKpr29bXqrt3KQJkr7rU+X+/X2RsD+sySu BHGjRJ1MgF1uWjwD6Bk1r5lhLAvVq3iF//gMPOZlI6ylc0maUeQG9Jn+hLrnQbj4Xc8D MNVg== 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:user-agent; bh=h02DuQUATASqZOhVwoK9KiCCTwxtbqKQGJsC1I90g0E=; b=oHg9bPqtxxiUFnMTNuBHU6NYUnt49aKdr19acgCxCHwSj4FG5g1FSQjM+pqLyQGEn+ LXXgBX5sfffLuUH960WRCRif2uMMGFaf3rqvB67QHOY2PzUO6r/q4V3RibSazjz3w6h8 QRoGfxtil9i8iYK90BqgCnBo+1kECyKIqwkJ159D1/8Gtk27+YzCdUjl+4eC0n8F1c/n m+c4aXl1bx4KWTrQvfqN/hMEBcHard/1/rr8f9obYXj26iZEl1bTUlKoEr0zCExVOFWt BjcMfOdl7QciDdgejpsex+PYlVcN36i8m/vrvmkDI7K5Q4LLNRN10S123uN7Xbct9Sw/ dQdQ== X-Gm-Message-State: APjAAAX/DMejOFuyXQZgazjywlqoPr2UpplUhvlQ3O608w9TiY6hLZCP XCTQKUC0b5H3hUWLg6xRECo7VA== X-Google-Smtp-Source: APXvYqwHPeIJXDCY/EPiglr1RS9U0hLJfXUrfIy7Hon6pXbIDVohcmIvRHolaexdyApn8UTu41Abrw== X-Received: by 2002:a05:6000:101:: with SMTP id o1mr2261029wrx.394.1572366324282; Tue, 29 Oct 2019 09:25:24 -0700 (PDT) Received: from 6wind.com (2a01cb0c0005a6000226b0fffeed02fc.ipv6.abo.wanadoo.fr. [2a01:cb0c:5:a600:226:b0ff:feed:2fc]) by smtp.gmail.com with ESMTPSA id 74sm18506975wrm.92.2019.10.29.09.25.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 29 Oct 2019 09:25:22 -0700 (PDT) Date: Tue, 29 Oct 2019 17:25:22 +0100 From: Olivier Matz To: Viacheslav Ovsiienko Cc: dev@dpdk.org, thomas@monjalon.net, matan@mellanox.com, orika@mellanox.com, Yongseok Koh Message-ID: <20191029162522.ozj724j7pz7hz753@platinum> References: <1571922495-4588-1-git-send-email-viacheslavo@mellanox.com> <1572201636-16374-1-git-send-email-viacheslavo@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1572201636-16374-1-git-send-email-viacheslavo@mellanox.com> User-Agent: NeoMutt/20180716 Subject: Re: [dpdk-dev] [PATCH v4] 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" Hi Slava, Looks good to me overall. Few minor comments below. On Sun, Oct 27, 2019 at 06:40:36PM +0000, Viacheslav Ovsiienko wrote: > Currently, metadata can be set on egress path via mbuf tx_metadata field > with PKT_TX_METADATA flag and RTE_FLOW_ITEM_TYPE_META matches metadata. > > This patch extends the metadata feature 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 ingress. 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 metadata dynamic field of > mbuf which can be accessed by RTE_FLOW_DYNF_METADATA(). > PKT_RX_DYNF_METADATA flag will be set along with the data. > > The mbuf dynamic field must be registered by calling > rte_flow_dynf_metadata_register() prior to use SET_META action. > > The availability of dynamic mbuf metadata field can be checked > with rte_flow_dynf_metadata_avail() routine. > > For loopback/hairpin packet, metadata set on Rx/Tx may or may not be > propagated to the other path depending on hardware capability. > > Signed-off-by: Yongseok Koh > Signed-off-by: Viacheslav Ovsiienko (...) > diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h > index c36c1b6..b19c86b 100644 > --- a/lib/librte_ethdev/rte_ethdev.h > +++ b/lib/librte_ethdev/rte_ethdev.h > @@ -1048,7 +1048,6 @@ struct rte_eth_conf { > #define DEV_RX_OFFLOAD_KEEP_CRC 0x00010000 > #define DEV_RX_OFFLOAD_SCTP_CKSUM 0x00020000 > #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM 0x00040000 > - > #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \ > DEV_RX_OFFLOAD_UDP_CKSUM | \ > DEV_RX_OFFLOAD_TCP_CKSUM) Undue removed line here. (...) > +/* Mbuf dynamic field offset for metadata. */ > +extern int rte_flow_dynf_metadata_offs; > + > +/* Mbuf dynamic field flag mask for metadata. */ > +extern uint64_t rte_flow_dynf_metadata_mask; > + > +/* Mbuf dynamic field pointer for metadata. */ > +#define RTE_FLOW_DYNF_METADATA(m) \ > + RTE_MBUF_DYNFIELD((m), rte_flow_dynf_metadata_offs, uint32_t *) > + > +/* Mbuf dynamic flag for metadata. */ > +#define PKT_RX_DYNF_METADATA (rte_flow_dynf_metadata_mask) > + > +__rte_experimental > +static inline uint32_t > +rte_flow_dynf_metadata_get(struct rte_mbuf *m) { > + return *RTE_FLOW_DYNF_METADATA(m); > +} > + > +__rte_experimental > +static inline void > +rte_flow_dynf_metadata_set(struct rte_mbuf *m, uint32_t v) { > + *RTE_FLOW_DYNF_METADATA(m) = v; > +} > + (...) > +__rte_experimental > +static inline int > +rte_flow_dynf_metadata_avail(void) { > + return !!rte_flow_dynf_metadata_mask; > +} I think, in DPDK: static inline void rte_flow_dynf_metadata_set(struct rte_mbuf *m, uint32_t v) { ... is prefered over: static inline void rte_flow_dynf_metadata_set(struct rte_mbuf *m, uint32_t v) { ... > --- a/lib/librte_mbuf/rte_mbuf_dyn.h > +++ b/lib/librte_mbuf/rte_mbuf_dyn.h > @@ -234,6 +234,10 @@ int rte_mbuf_dynflag_lookup(const char *name, > __rte_experimental > void rte_mbuf_dyn_dump(FILE *out); > > -/* Placeholder for dynamic fields and flags declarations. */ > - > +/* > + * Placeholder for dynamic fields and flags declarations. > + * This is centralizing point to gather all field names > + * and parameters together. > + */ > +#define MBUF_DYNF_METADATA_NAME "rte_flow_dynfield_metadata" > #endif The RTE_ prefix is missing. Also, thi name is called dynfield but it is used for both field and flag. I suggest RTE_MBUF_DYNFIELD_METADATA_NAME and RTE_MBUF_DYNFLAG_METADATA_NAME, to be consistent with the other naming conventions in rte_mbuf_dyn.[ch]. One more comment: as previously discussed, changing the size or alignement of a dynamic field should not be allowed, because it can break the users of the field. Depending on how it is implemented (is the registration function inline? is the rte_mbuf_dynfield structure private, shared, or static const in a .h? are we using #defines for name, size, align?), I think the impact on users will be different. This is something we need to think about for next versions: how to detect these changes before pushing the commit, and/or at runtime? Regards, Olivier