From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from dpdk.org (dpdk.org [92.243.14.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 51BC0A00BE;
	Tue, 29 Oct 2019 18:43:40 +0100 (CET)
Received: from [92.243.14.124] (localhost [127.0.0.1])
	by dpdk.org (Postfix) with ESMTP id 6D0C01BF7F;
	Tue, 29 Oct 2019 18:43:39 +0100 (CET)
Received: from EUR04-VI1-obe.outbound.protection.outlook.com
 (mail-eopbgr80084.outbound.protection.outlook.com [40.107.8.84])
 by dpdk.org (Postfix) with ESMTP id 8E2641BF7A
 for <dev@dpdk.org>; Tue, 29 Oct 2019 18:43:38 +0100 (CET)
ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none;
 b=Djlth5TR22EWEYw+KOgMCpI+uR6+SHhnVHComdsZaMKTkZChsIno0JH21RkvxH4uG3vzgTDLG6pQAFyt2Klv4okKui5opItVT2gOGdup/CE9IU4AwNNPVjTxSu5EAzoQToK3sVtgLV/N1RNJ0BYQt+Q3Ml9igiVlv/P7F5TXFZOWMKwc5eYzFT0PmCfgztORl9m0+a3p7CiI46bDVDNvlKetrIuDBu2LXV9Qen5JF1hrKnvLNtwQutxw/YKrWEOkI5eKbYbl8FMtIKC3YcGWvsmJ4t54RD7xKGR0KtAQcQohq2gviaU0HksXE6z5RsBbiEdTqr8IY9ozI4P4cqQgNw==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; 
 s=arcselector9901;
 h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;
 bh=j7PRNmOZZwte3tu6K6Tbb2gAS4hOaOR+RzXNDQr76SM=;
 b=ZdYDJXYk4CgIc4yGYZI2GV5JdZqWk2HexhztSjAUe+IX/lkP0znGDCgTjpVCRlyrh0SS73Q4L8ImvZ4c8XhZ/JK9p9XM0rcPj5moMX/rnQGx4d0JJKK6bb/4/UbiSKFYF8ENG5Ku7lNbLl6wdbchdclkxRZJQqrzs9VeZaK2yBKi08+kgiQ09F1ZDEvW7eW5ISRZHlKyV18kRuJkX2AmuN+QHLAWEp7mSVlbWEmXuL9k/rCJuMgUzIme9To2TQPHf/nEiARWF0Tm2VTH2zgGQjrRIEfL68o2f7O9XxVi7kflWRT8SPxM9OBBnITws64yajjfRXkfHdKZ7VBmfavwgg==
ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass
 smtp.mailfrom=mellanox.com; dmarc=pass action=none header.from=mellanox.com;
 dkim=pass header.d=mellanox.com; arc=none
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com;
 s=selector2;
 h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck;
 bh=j7PRNmOZZwte3tu6K6Tbb2gAS4hOaOR+RzXNDQr76SM=;
 b=cRmqAjapSkhMII3DQdmrgQzQvfbY2dULb36Mxcpnw4i5/ey7uP569IkTc1EDdn00bE1y2irmB0lSZx9k9hYggl2EeEGCMabJKYc2IP4U8IAXIZv4OhRLTv5Jd18OwdRBYLGsqeD0tOZJ482wuNWneF4BpB4hz7HmDka5XvQEHak=
Received: from AM4PR05MB3265.eurprd05.prod.outlook.com (10.171.188.154) by
 AM4PR05MB3332.eurprd05.prod.outlook.com (10.171.187.17) with Microsoft SMTP
 Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id
 15.20.2387.20; Tue, 29 Oct 2019 17:43:37 +0000
Received: from AM4PR05MB3265.eurprd05.prod.outlook.com
 ([fe80::edab:529f:d14e:d3b]) by AM4PR05MB3265.eurprd05.prod.outlook.com
 ([fe80::edab:529f:d14e:d3b%7]) with mapi id 15.20.2387.027; Tue, 29 Oct 2019
 17:43:37 +0000
From: Slava Ovsiienko <viacheslavo@mellanox.com>
To: Olivier Matz <olivier.matz@6wind.com>
CC: "dev@dpdk.org" <dev@dpdk.org>, Thomas Monjalon <thomas@monjalon.net>,
 Matan Azrad <matan@mellanox.com>, Ori Kam <orika@mellanox.com>, Yongseok Koh
 <yskoh@mellanox.com>
Thread-Topic: [PATCH v4] ethdev: extend flow metadata
Thread-Index: AQHVjnV4dBh/aEw9UUCRkYoPsCf2Zqdx4XAg
Date: Tue, 29 Oct 2019 17:43:36 +0000
Message-ID: <AM4PR05MB32656FEEE8F1E322471A5599D2610@AM4PR05MB3265.eurprd05.prod.outlook.com>
References: <1571922495-4588-1-git-send-email-viacheslavo@mellanox.com>
 <1572201636-16374-1-git-send-email-viacheslavo@mellanox.com>
 <20191029162522.ozj724j7pz7hz753@platinum>
In-Reply-To: <20191029162522.ozj724j7pz7hz753@platinum>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach: 
X-MS-TNEF-Correlator: 
authentication-results: spf=none (sender IP is )
 smtp.mailfrom=viacheslavo@mellanox.com; 
x-originating-ip: [95.164.10.10]
x-ms-publictraffictype: Email
x-ms-office365-filtering-ht: Tenant
x-ms-office365-filtering-correlation-id: cb82a7ba-f776-4b27-0bde-08d75c9786f6
x-ms-traffictypediagnostic: AM4PR05MB3332:|AM4PR05MB3332:
x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr
x-ms-exchange-transport-forked: True
x-microsoft-antispam-prvs: <AM4PR05MB333215D383F6537F91565BC6D2610@AM4PR05MB3332.eurprd05.prod.outlook.com>
x-ms-oob-tlc-oobclassifiers: OLM:5516;
x-forefront-prvs: 0205EDCD76
x-forefront-antispam-report: SFV:NSPM;
 SFS:(10009020)(4636009)(366004)(376002)(136003)(346002)(396003)(39860400002)(189003)(199004)(13464003)(446003)(4326008)(71190400001)(53546011)(476003)(6506007)(54906003)(316002)(74316002)(7736002)(229853002)(102836004)(6916009)(71200400001)(256004)(66066001)(14454004)(52536014)(478600001)(99286004)(7696005)(305945005)(14444005)(86362001)(76176011)(6436002)(66946007)(5660300002)(66556008)(66476007)(9686003)(11346002)(64756008)(25786009)(8676002)(2906002)(8936002)(186003)(3846002)(6116002)(55016002)(33656002)(66446008)(76116006)(107886003)(81166006)(81156014)(486006)(6246003)(26005)(309714004);
 DIR:OUT; SFP:1101; SCL:1; SRVR:AM4PR05MB3332;
 H:AM4PR05MB3265.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en;
 PTR:InfoNoRecords; MX:1; A:1; 
received-spf: None (protection.outlook.com: mellanox.com does not designate
 permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam: BCL:0;
x-microsoft-antispam-message-info: vcMFzR++DkjG6eV3Zs+PBy7ffTp+B8vqIbw33eEgvt5qtoQZ2R85esMGWEp2hB80L923EO0b1fOblKAZuT7F2ZRZJcL8uVa9SJauxWDMzw/q/byI3Jl3GQYcYQrLnPPACI8M3JU6fFv21amGMxMQGqYB5q+gO4rMhRmKgOubxZOjqhAtggYoPWPSN0+O+49SRsHLIVlY7JYVpDZf7GnOljTHgZfK37hdBuymdGN6QdxBSKl8FZbFqLC6ow8LK3eHyC1Rn1Kogab3sx3LbuUH73YRjYEA3HMQt65O7FQDCzR97duWn1c/8n36oJJf/eL9bL+AkUQk8MjVFxjSFaaGtO6EXeQqmjAo8gCPEYQcYyF2BCwEbFfai3zzyGs3yx1lVwcooigOlUsYdKgVJj2295XKaxL2ES8O50UDV+DCA0d/TuX0uU7JyX8XYYuOdSb0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: Mellanox.com
X-MS-Exchange-CrossTenant-Network-Message-Id: cb82a7ba-f776-4b27-0bde-08d75c9786f6
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Oct 2019 17:43:36.7305 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 1fkds+NDaUbzy62kjQQ3ya2EwdU0CNLHlSOy4CzmuJbEUuc+3DLdBXGKrDXYFJaISEFmwLkRSOtdo6l7oYtmyjruYoan1zx9nzlmJ68tPFA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR05MB3332
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

Hi, Olivier

Thanks a lot for the review.

> -----Original Message-----
> From: Olivier Matz <olivier.matz@6wind.com>
> Sent: Tuesday, October 29, 2019 18:25
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas@monjalon.net>; Matan
> Azrad <matan@mellanox.com>; Ori Kam <orika@mellanox.com>; Yongseok
> Koh <yskoh@mellanox.com>
> Subject: Re: [PATCH v4] ethdev: extend flow metadata
>=20
> Hi Slava,
>=20
> Looks good to me overall. Few minor comments below.
>=20
> 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 <yskoh@mellanox.com>
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
>=20
> (...)
>=20
> > 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)
>=20
> Undue removed line here.

Right, will fix.

>=20
> (...)
>=20
> > +/* 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) =3D v;
> > +}
> > +
>=20
> (...)
>=20
> > +__rte_experimental
> > +static inline int
> > +rte_flow_dynf_metadata_avail(void) {
> > +       return !!rte_flow_dynf_metadata_mask; }
>=20
> I think, in DPDK:
>=20
> 	static inline void
> 	rte_flow_dynf_metadata_set(struct rte_mbuf *m, uint32_t v)
> 	{
> 		...
>=20
> is prefered over:
>=20
> 	static inline void
> 	rte_flow_dynf_metadata_set(struct rte_mbuf *m, uint32_t v) {

Right. It is strange it passed the validator. Will fix.
> 		...
>=20
> > --- 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
>=20
> The RTE_ prefix is missing. Also, thi name is called dynfield but it is u=
sed 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].

Well, it makes sense for complex features, say, with multiple dynaflags.
But it is OK for me, will update.

>=20
> One more comment: as previously discussed, changing the size or alignemen=
t
> of a dynamic field should not be allowed, because it can break the users =
of
> the field.
>=20
> 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 wi=
ll be
> different. This is something we need to think about for next versions: ho=
w to
> detect these changes before pushing the commit, and/or at runtime?
>=20
I'm not sure if we will have a lot of implementation invariants for dynamic=
 fields.
These ones are intended to be used in datapath, performance is a king. I th=
ink if=20
someone invent the more efficient way to handle dynafields, we'll update th=
e
metadata code either.


> Regards,
> Olivier