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 4E9C9A00BE; Tue, 29 Oct 2019 17:33:48 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A61EE1BEF7; Tue, 29 Oct 2019 17:33:47 +0100 (CET) Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by dpdk.org (Postfix) with ESMTP id 90E7B1BEF0 for ; Tue, 29 Oct 2019 17:33:46 +0100 (CET) Received: by mail-wr1-f68.google.com with SMTP id z11so14340512wro.11 for ; Tue, 29 Oct 2019 09:33:46 -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=F/ckU5kiJWNNXQZcwme3+Vjx3DQX5JF7e1rUqggF7+A=; b=NulUDPgaO9pME2u61grJJV0xuBqDyp3rrYUtgHjV36vfb6vWClMS8pxnvzwVZcHikS AoMNOJ3iHm2UIvLqX+Asf8sYg6XWdd8SqLIY/hPknNaY/kiF46GX9Ox2fc5WVdkqjwI0 2qmQAxDK/qHQFY5SlzdxtrFVjnH+aCsnJlKWd0CSTMMgkkJuvfIYnz++8qyYRPGx07u/ nzFKvYGU9JNnNktTja4WWDZ1Fg1Kv357rVHcjiOQKxxytBk2vgtp/HxFfIPrYXumOane mVQtiQNd/Lebupi51pdnr7qyeObYBjN6+YKLFKCM3qws92g14gx6pwxdeKIl1VYBLetf UjTw== 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=F/ckU5kiJWNNXQZcwme3+Vjx3DQX5JF7e1rUqggF7+A=; b=g3beyXXj5lAm+ydClrFs3lNZtqO3mkaHPkr9I8u/pDgcze/GRruy+3ojJGgvmSrgag LNOgjE6T3YVlkxHK+3vZv1rArYtp3gm2uZvpaYEF5Tt1P6IvHpmO6noU7a5kzOMPD+j5 4uo3xS1t6RKe5FBaeU+wD0ycrQ4Y62ct2CyZUNK+FTfNv3kifctwWsBlxmw8AP4NQ1SO /ktmtjsflsecO4tKRoRrgKSqS+j8hsjV3cXIZM+L/7haQJeTVu2OdREfxd9BsKFjOvFu 3u4RQXpV5CfPUBqnocoUR92hpw8NcB7M5/BmkiYzPlMI5VwYXVc1uQHWsfHvToG0l8UQ 6eJw== X-Gm-Message-State: APjAAAU2DnzkebUk3OVBxsLhTLEkWsl8ozwDcgoHM6ugH+wLfem2BQJ+ LCdhGAn+yQKOQxQHZI7djlUV9w== X-Google-Smtp-Source: APXvYqyMs6X7tXv0v4yCFoyVL/E1uGGEsZpbW8G1hK08pS3UQXuesBsjMuqIxMUQRc76iSIc/jSf4w== X-Received: by 2002:adf:dd10:: with SMTP id a16mr818711wrm.213.1572366826143; Tue, 29 Oct 2019 09:33:46 -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 x127sm4003294wmx.18.2019.10.29.09.33.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 29 Oct 2019 09:33:44 -0700 (PDT) Date: Tue, 29 Oct 2019 17:33:44 +0100 From: Olivier Matz To: Viacheslav Ovsiienko Cc: dev@dpdk.org, thomas@monjalon.net, matan@mellanox.com, orika@mellanox.com, Yongseok Koh Message-ID: <20191029163344.5mlwhjx6rban6xka@platinum> References: <1571922495-4588-1-git-send-email-viacheslavo@mellanox.com> <1572201636-16374-1-git-send-email-viacheslavo@mellanox.com> <20191029162522.ozj724j7pz7hz753@platinum> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191029162522.ozj724j7pz7hz753@platinum> 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" On Tue, Oct 29, 2019 at 05:25:22PM +0100, Olivier Matz wrote: > 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]. I forgot: can you please document the goal/usage of these field and flag here? Not necessarily a detailed explanation, but a high level view: what is transported, when it is registered, ... > 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