From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR04-VI1-obe.outbound.protection.outlook.com (mail-eopbgr80053.outbound.protection.outlook.com [40.107.8.53]) by dpdk.org (Postfix) with ESMTP id 860615F16 for ; Fri, 26 Oct 2018 01:33:16 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=4W3H4z5qgeIGrAuH8dLKlEPKcSsZYOFiOrc0PRd7bUk=; b=i8L49fryTaYeSEWlT6GGxSjDhxjksrUcdRk4R5H3IFv2PxZXtJm+dqEplVQxxp8gUNf7JtsP5qjNqP++Mcd2CWlyUJq3l2Utwkuq7kVitGfg3Db8bZFCqsoVmUDfLY7R8iYkOZgj091+31z56NVqt6NNye7mXQAvGlJZ8oMsj4w= Received: from DB3PR0502MB3980.eurprd05.prod.outlook.com (52.134.72.27) by DB3PR0502MB4027.eurprd05.prod.outlook.com (52.134.66.142) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1250.30; Thu, 25 Oct 2018 23:33:14 +0000 Received: from DB3PR0502MB3980.eurprd05.prod.outlook.com ([fe80::f8a1:fcab:94f0:97cc]) by DB3PR0502MB3980.eurprd05.prod.outlook.com ([fe80::f8a1:fcab:94f0:97cc%4]) with mapi id 15.20.1273.021; Thu, 25 Oct 2018 23:33:14 +0000 From: Yongseok Koh To: Slava Ovsiienko CC: Shahaf Shuler , "dev@dpdk.org" Thread-Topic: [PATCH v2 1/7] net/mlx5: e-switch VXLAN configuration and definitions Thread-Index: AQHUZJFXAn6IFwvCd0O52pR+SaFJP6UsMDUAgAPJQgCAALOQgA== Date: Thu, 25 Oct 2018 23:33:14 +0000 Message-ID: <20181025233306.GA6434@mtidpdk.mti.labs.mlnx> References: <1538461807-37507-1-git-send-email-viacheslavo@mellanox.com> <1539612815-47199-1-git-send-email-viacheslavo@mellanox.com> <1539612815-47199-2-git-send-email-viacheslavo@mellanox.com> <20181023100109.GA14792@mtidpdk.mti.labs.mlnx> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-clientproxiedby: BYAPR04CA0031.namprd04.prod.outlook.com (2603:10b6:a03:40::44) To DB3PR0502MB3980.eurprd05.prod.outlook.com (2603:10a6:8:10::27) authentication-results: spf=none (sender IP is ) smtp.mailfrom=yskoh@mellanox.com; x-ms-exchange-messagesentrepresentingtype: 1 x-originating-ip: [209.116.155.178] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DB3PR0502MB4027; 6:CUw+83XiWTLrCdytgoyEI0OcrwmxauKOEMpiAvDEGA5z7FvWZS0SV43+FdfQkQeUreoFYmwMSOEthZ/47KD/1YaWQxW6gqTFw2NdEpJghc9trxivLVq3e1ANq0PKh0efBBeWlsbxlfRaZEwciNcdJsTPel63jQH02bhI7T47IM+l4ixc2Z9WPcsZ8PeuhjX1yHssijQvFpx0WHnb8NQ4DjED0WUAy+l2k8jpEQntpOyA6W79HgA6Q4+/g8A80uebEFjblzYE+5KD1At66TETHH61c0Rd6X3N+pNIHJmRY9JEcS+Mgwsa7zhJM9ksk18C4mIC2vrQ8TXihthj6vZUF5Fu/tIF9cXOd4iGh2x9VbddcmDkEx9n3c8bnzmEvWxo67AZY2ZVyDT1xfT54kRXBAwZMr/v9g67nD3QOLzExSmL9XEYgxufQO4haH741dDhE098fT8M2nLJRcgz2cHovA==; 5:m0179nFVsyS0xGnFkMTb6cQoP/Z/piNkIXDmMqE7ZSjZIOFKjVFh5HgW9NR1MnCir1eAXZPrgUvVGKTuD28EzqUODTrtEVpOYh8v0O1VGfg3FCXsEEhV2A5Y4+ik5WPjiwc4DIvTgHT627ahfpfBc7ukIL5T2Z7LZ5oJZTaksOI=; 7:YKho2TABxfBrqlxai6qSv6/t6m5J7tbYdAI4YyPlBanZlD8GLM++5xUVDirgZcP8yB8YO86SNelDv+wBWzKURObf0hgYaYsL57/6bAdwd1qgq+kBTk4aG3gtE15DOf/VW1ecsmOCFlWfyQZhJLKs1DKMtUDY7w0/upo408b+jJz9OImK0eSZs9VAu7dkB9uSpgdndvfJIBBgYHra8JM5pm7kIEzwaks5iRvJhqVeazb/H6GZK52cVTW8ILra2a+k x-ms-office365-filtering-correlation-id: e82ff949-fbed-49ff-1941-08d63ad23bbd x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600074)(711020)(4618075)(2017052603328)(7153060)(7193020); SRVR:DB3PR0502MB4027; x-ms-traffictypediagnostic: DB3PR0502MB4027: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(5005006)(8121501046)(93006095)(93001095)(10201501046)(3231355)(944501410)(4982022)(52105095)(3002001)(6055026)(148016)(149066)(150057)(6041310)(20161123560045)(20161123564045)(20161123558120)(20161123562045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(201708071742011)(7699051)(76991095); SRVR:DB3PR0502MB4027; BCL:0; PCL:0; RULEID:; SRVR:DB3PR0502MB4027; x-forefront-prvs: 083691450C x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(376002)(366004)(136003)(39860400002)(346002)(396003)(13464003)(57704003)(189003)(199004)(486006)(68736007)(6486002)(71200400001)(256004)(186003)(71190400001)(6306002)(53936002)(11346002)(9686003)(446003)(476003)(2906002)(25786009)(6246003)(966005)(6512007)(14454004)(14444005)(66066001)(6862004)(33656002)(6436002)(7736002)(86362001)(1076002)(305945005)(105586002)(26005)(3846002)(478600001)(229853002)(8936002)(53546011)(33896004)(6506007)(102836004)(386003)(76176011)(52116002)(81166006)(4326008)(106356001)(5250100002)(93886005)(6116002)(81156014)(97736004)(8676002)(6636002)(316002)(2900100001)(54906003)(99286004)(5660300001)(21314003); DIR:OUT; SFP:1101; SCL:1; SRVR:DB3PR0502MB4027; H:DB3PR0502MB3980.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-microsoft-antispam-message-info: Ef6PLRKRtj0AZSfvR+mTtQWZ4xntRRX95U1gIla3J2H9VthV9DYHmVZRUp7Fg4RT6nI6VPw9m0qu828hMkaWPyDo5CISw7IneeSayGIb5dDixBpOpX4yyQaYI75dLRfUtpxrgBUCR1VUTc7TKk/CkE+Rzq55FTvVN2Nl4a5sUfadLAaW2uflGcl9WR1J3QNkjKDTLXBdUEeIOX1qX4c2cbxpQhVGS6R54pvjSV5f1zjOVRQCi0zeQU0qMguzbwZDmsQ+/yicnNVD0rtCbOi1ZvIxX25SwlmzWEA98yXsjZNYXOAlSBMAM4Q4o89wHib0bAOeuVCMbWEmE8VcqJFVNJCviDGVU+phuu5rRyHHr9c= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-ID: <6EBFEC82E314614E9FDA5C471976D1E1@eurprd05.prod.outlook.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: e82ff949-fbed-49ff-1941-08d63ad23bbd X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Oct 2018 23:33:14.4249 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB3PR0502MB4027 Subject: Re: [dpdk-dev] [PATCH v2 1/7] net/mlx5: e-switch VXLAN configuration and definitions 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: , X-List-Received-Date: Thu, 25 Oct 2018 23:33:16 -0000 On Thu, Oct 25, 2018 at 05:50:26AM -0700, Slava Ovsiienko wrote: > > -----Original Message----- > > From: Yongseok Koh > > Sent: Tuesday, October 23, 2018 13:02 > > To: Slava Ovsiienko > > Cc: Shahaf Shuler ; dev@dpdk.org > > Subject: Re: [PATCH v2 1/7] net/mlx5: e-switch VXLAN configuration and > > definitions > >=20 > > On Mon, Oct 15, 2018 at 02:13:29PM +0000, Viacheslav Ovsiienko wrote: [...] > > > diff --git a/drivers/net/mlx5/mlx5_flow.h b/drivers/net/mlx5/mlx5_flo= w.h > > > index 840d645..b838ab0 100644 > > > --- a/drivers/net/mlx5/mlx5_flow.h > > > +++ b/drivers/net/mlx5/mlx5_flow.h > > > @@ -85,6 +85,8 @@ > > > #define MLX5_FLOW_ACTION_SET_TP_SRC (1u << 15) > > > #define MLX5_FLOW_ACTION_SET_TP_DST (1u << 16) > > > #define MLX5_FLOW_ACTION_JUMP (1u << 17) > > > +#define MLX5_ACTION_VXLAN_ENCAP (1u << 11) > > > +#define MLX5_ACTION_VXLAN_DECAP (1u << 12) > >=20 > > MLX5_ACTION_* has been changed to MLX5_FLOW_ACTION_* as you can > > see above. > OK. Miscopied from previous version of patch. >=20 > > And make it alphabetical order; decap first and encap later? Or, at lea= st make > > it consistent. The order (case clause) is different among validate, pre= pare and > > translate. > OK. Will reorder. >=20 > >=20 > > > #define MLX5_FLOW_FATE_ACTIONS \ > > > (MLX5_FLOW_ACTION_DROP | MLX5_FLOW_ACTION_QUEUE | > > MLX5_FLOW_ACTION_RSS) > > > @@ -182,8 +184,17 @@ struct mlx5_flow_dv { > > > struct mlx5_flow_tcf { > > > struct nlmsghdr *nlh; > > > struct tcmsg *tcm; > > > + uint32_t nlsize; /**< Size of NL message buffer. */ > >=20 > > It is used only for assert(), but if prepare() is trusted, why do we ne= ed to > > keep it? I don't it is needed. > >=20 > Q? Let's keep the nlsize under NDEBUG flag?=20 > It's extremely useful to have assert() > on allocated size for debugging purposes. Totally agree. Please do so. > > > + uint32_t applied:1; /**< Whether rule is currently applied. */ > > > + uint64_t item_flags; /**< Item flags. */ > >=20 > > This isn't used at all. > OK, now no dependencies on it, should be removed, good. >=20 > >=20 > > > + uint64_t action_flags; /**< Action flags. */ > >=20 > > I checked following patches and it doesn't seem necessary. Please refer= to > > the > > comment on the translation func. But if you think it is really needed, = you > > could've used actions field of struct rte_flow and layers field of stru= ct > > mlx5_flow in mlx5_flow.h >=20 > When translating item list into NL-message we have to know whether there = is=20 > some tunneling action in the actions list. This is due to possible=20 > changing of the item meanings if tunneling action is present. For example= , > usually the ipv4 item provides IPv4 addresses for matching and translated= to > TCA_FLOWER_KEY_IPV4_SRC (+ xxx_DST) Netlink attribute(s), but if there is > VXLAN decap action specified, this item becames outer tunnel source IPs > and should be translated to TCA_FLOWER_KEY_ENC_IPV4_SRC. The action > list is scanned in the preperd list, so we can save action flags and us= e these > gathered results in translation routine. As we can see from mlx5_flow_lis= t_create() source, > it does not save item/actions flags, gathered by flow_drv_prepare(). That= 's why > there are item_flags/action_flags in the struct mlx5_flow_tcf. item_flag= s is not > needed, should be removed. action_flags is in use. >=20 > BTW, do we need item_flags, action_flags params in flow_drv_prepare() ? > We would avoid the item_flags field if flags were transferred from > flow_drv_prepare() to flow_drv_translate() (as local variable of > mlx5_flow_list_create(). That was a bug. I found it while I was reviewing your patches. Thanks :) Refer to my patch which is already merged. Prepare should return flags so t= hat it can be used by translate and others. http://git.dpdk.org/next/dpdk-next-net-mlx/commit/?id=3D4fa7d5e88165745523b= 9b6682c4092fb943a7d49 So, you don't need to keep this field here. >=20 > > > uint64_t hits; > > > uint64_t bytes; > > > + union { /**< Tunnel encap/decap descriptor. */ > > > + struct mlx5_flow_tcf_tunnel_hdr *tunnel; > > > + struct mlx5_flow_tcf_vxlan_decap *vxlan_decap; > > > + struct mlx5_flow_tcf_vxlan_encap *vxlan_encap; > > > + }; > >=20 > > What is the reason for keeping pointer even though the actual structure > > follows > > after mlx5_flow_tcf? Maybe you don't want to waste memory, as the size = of > > encap/decap struct differs a lot? >=20 > Sizes differ, but not a lot (especially comparing with DV rule size).=20 > Would you prefer to simplify and just include the union? > On other hand we could declare something like that: > ... > uint8_t tunnel_type; > alignas(struct mlx5_flow_tcf_tunnel_hdr) uint8_t buf[]; >=20 > and eliminate the pointer at all. The buf beginning contains either tunne= l structure > or Netlink message (if no tunnel), depending on the tunnel_type field. I was just curious. Either way looks good to me. Will defer it to you. Thanks, Yongseok