From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR03-VE1-obe.outbound.protection.outlook.com (mail-eopbgr50044.outbound.protection.outlook.com [40.107.5.44]) by dpdk.org (Postfix) with ESMTP id 4E00B2B8C for ; Sat, 27 Oct 2018 00:11:00 +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=WiL73kjaII0kQc0y2sYVgfcU/HqL8eMqkce9zSlii+U=; b=DB1jQOew13CxmH1gzbifoNBp5ttYe0wE7YMAentedmocrn3OToUvBqQPWOqMQ1Q3kILtAq60vx+jsj1N7LCju7tAEHU6lQfGNY2aU9cit3HuS1Gck2BYeiC1YiF3NElm6LqgrFAeD74Fr2RnwI427vX9UPHun9EDEBC79v4f+os= Received: from DB3PR0502MB3980.eurprd05.prod.outlook.com (52.134.72.27) by DB3PR0502MB4041.eurprd05.prod.outlook.com (52.134.66.31) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1273.25; Fri, 26 Oct 2018 22:10:58 +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.025; Fri, 26 Oct 2018 22:10:58 +0000 From: Yongseok Koh To: Slava Ovsiienko CC: Shahaf Shuler , "dev@dpdk.org" Thread-Topic: [PATCH v2 3/7] net/mlx5: e-switch VXLAN flow translation routine Thread-Index: AQHUZJFZmoxzpCFrNk+Iuvz4Vcwsm6UsMZyAgAPl5ACAAHDagIAAxPyAgADbCQA= Date: Fri, 26 Oct 2018 22:10:58 +0000 Message-ID: <20181026221049.GD13615@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-4-git-send-email-viacheslavo@mellanox.com> <20181023100621.GC14792@mtidpdk.mti.labs.mlnx> <20181026042151.GC6434@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: BYAPR08CA0013.namprd08.prod.outlook.com (2603:10b6:a03:100::26) 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; DB3PR0502MB4041; 6:FYZx3YYcokapd3WLwkEjpSEAPZ3ThBR+WMK8gX3742THQ7TZp57qEM/fWW+wCAqYf4zxQcUJw9pxi3u+dt64hagtHVVuxvzdollqfxzX7h3AjOkMF0AOBd/Qd0qAoc1meyEbh/FDQ8nQXlGg2GLDvhn2DYaMFpYVgQ5OGkkTfQvMEa16rARRUTdVUvt+/DCvtVbKEJK87OEaJgC7XCA9FqmtNXpAKkcWEvkJrrUdlPGKx/ArdPsgQXYop7mtQzzMcmeboDoql8f1JoOedk0i7gPk2AgO7WAx4gW+RBR82n76nA7Oe/qx5WWHCtFFCwrH4s950y1OVXRIHWIaKABRwuea7/bdY06fVm95Z+u4onCD2cRA8mtTen6u3NLFxnRKpsLGXI8n9QwoB1bQ+nwZNLf/w+Mc5Ssemvul3l2wX0Y+Gu5oC6OqdqoyzP8r+r/WAx2nK6A7SZKYi4lJlR8NoA==; 5:d2hZMDA6KmyJOscBP+RikKLIaNMlc9pdmpNx3Iast1MGOj4JCMBoQrWVqgojBEMveaRTg+MsHaJ5+AKXS5KR6YR1I8rwD/uQTDEGWrovTQG9YaHxZyI3D8pUJpDCWGPpzHrtHs+BrrcitaTRMy4YaSBxMA4WvOKtiEW6WQW/h2A=; 7:ig7Qws8+ew1bsv5W414PgJxh747wZmsSneLZQt4GnlXA+iRdOdM52l8fP5WJ7ChZUW4iyxbmbYnY59OKDdOI/yAOxGO4u52o165Rwg5fJBY0x5ubvAsZJli+4nxmXzFqV60QSVpBEDwDcxoGnDcd3g== x-ms-office365-filtering-correlation-id: ff8c81e9-c02e-41d5-109d-08d63b8fe803 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:DB3PR0502MB4041; x-ms-traffictypediagnostic: DB3PR0502MB4041: 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)(8121501046)(5005006)(10201501046)(93006095)(93001095)(3231355)(944501410)(52105095)(3002001)(6055026)(148016)(149066)(150057)(6041310)(20161123562045)(20161123558120)(20161123560045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123564045)(201708071742011)(7699051)(76991095); SRVR:DB3PR0502MB4041; BCL:0; PCL:0; RULEID:; SRVR:DB3PR0502MB4041; x-forefront-prvs: 083751FCA6 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(376002)(136003)(39860400002)(366004)(346002)(396003)(189003)(199004)(52314003)(13464003)(6636002)(2906002)(76176011)(97736004)(1076002)(102836004)(53546011)(52116002)(6486002)(478600001)(186003)(6436002)(33656002)(93886005)(7736002)(305945005)(229853002)(4744004)(446003)(71200400001)(26005)(71190400001)(386003)(6506007)(25786009)(105586002)(66066001)(53946003)(99286004)(6246003)(54906003)(4326008)(53936002)(106356001)(6512007)(486006)(6862004)(316002)(9686003)(2900100001)(6116002)(14454004)(476003)(3846002)(8936002)(11346002)(5660300001)(33896004)(86362001)(81166006)(81156014)(8676002)(68736007)(256004)(5250100002)(14444005); DIR:OUT; SFP:1101; SCL:1; SRVR:DB3PR0502MB4041; 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: 1tc8qWTvBUtoVmL5AEmdjwUzqrLHxXx0i+GtEswWte+FbVduMRM2pQQ1vVWfbKFwI4xsA3wHfzIidME4dK2EcH2Vm3hOquNf4xyNRRrVIACy+7Ko8XHztvaYYdc/Xh6bGx4Tv+96v7opgBN8ENxNmSuzl8EcyQBcM5K8xRDQE/cQKC3/tgplPWHBONuSASj5nPcItSkStHB6QHYITS4/db0o0u0iFOhTuPR7v3LlripC4iR/33LJiDhGzK4sNOuKFM9UvqjQgPRudE2mBYOO5Al8Ec8GR1+tzU9NER9FxAh+reZQuwcZF9BkBD9c/rLaUhy1LYRsNMEn8febPGJuCHgISUuo9Xkb1DiMz3ZFtyo= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: ff8c81e9-c02e-41d5-109d-08d63b8fe803 X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Oct 2018 22:10:58.4785 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB3PR0502MB4041 Subject: Re: [dpdk-dev] [PATCH v2 3/7] net/mlx5: e-switch VXLAN flow translation routine 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: Fri, 26 Oct 2018 22:11:00 -0000 On Fri, Oct 26, 2018 at 02:06:53AM -0700, Slava Ovsiienko wrote: > > -----Original Message----- > > From: Yongseok Koh > > Sent: Friday, October 26, 2018 7:22 > > To: Slava Ovsiienko > > Cc: Shahaf Shuler ; dev@dpdk.org > > Subject: Re: [PATCH v2 3/7] net/mlx5: e-switch VXLAN flow translation > > routine > >=20 > > On Thu, Oct 25, 2018 at 07:37:56AM -0700, Slava Ovsiienko wrote: > > > > -----Original Message----- > > > > From: Yongseok Koh > > > > Sent: Tuesday, October 23, 2018 13:06 > > > > To: Slava Ovsiienko > > > > Cc: Shahaf Shuler ; dev@dpdk.org > > > > Subject: Re: [PATCH v2 3/7] net/mlx5: e-switch VXLAN flow > > > > translation routine > > > > > > > > On Mon, Oct 15, 2018 at 02:13:31PM +0000, Viacheslav Ovsiienko wrot= e: > > [...] > > > > > @@ -2184,13 +2447,16 @@ struct pedit_parser { > > > > > * Pointer to the list of actions. > > > > > * @param[out] action_flags > > > > > * Pointer to the detected actions. > > > > > + * @param[out] tunnel > > > > > + * Pointer to tunnel encapsulation parameters structure to fil= l. > > > > > * > > > > > * @return > > > > > * Maximum size of memory for actions. > > > > > */ > > > > > static int > > > > > flow_tcf_get_actions_and_size(const struct rte_flow_action actio= ns[], > > > > > - uint64_t *action_flags) > > > > > + uint64_t *action_flags, > > > > > + void *tunnel) > > > > > > > > This func is to get actions and size but you are parsing and fillin= g > > > > tunnel info here. It would be better to move parsing to translate() > > > > because it anyway has multiple if conditions (same as switch/case) > > > > to set TCA_TUNNEL_KEY_ENC_* there. > > > Do you mean call of flow_tcf_vxlan_encap_parse(actions, tunnel)? > >=20 > > Yes. > >=20 > > > OK, let's move it to translate stage. Anyway, we need to keep encap > > > structure for local/neigh rules. > > > > > > > > > > > > { > > > > > int size =3D 0; > > > > > uint64_t flags =3D 0; > > > > > @@ -2246,6 +2512,29 @@ struct pedit_parser { > > > > > SZ_NLATTR_TYPE_OF(uint16_t) + /* VLAN ID. > > > > */ > > > > > SZ_NLATTR_TYPE_OF(uint8_t); /* VLAN prio. > > > > */ > > > > > break; > > > > > + case RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP: > > > > > + size +=3D SZ_NLATTR_NEST + /* na_act_index. */ > > > > > + SZ_NLATTR_STRZ_OF("tunnel_key") + > > > > > + SZ_NLATTR_NEST + /* TCA_ACT_OPTIONS. > > > > */ > > > > > + SZ_NLATTR_TYPE_OF(uint8_t); > > > > > + size +=3D SZ_NLATTR_TYPE_OF(struct tc_tunnel_key); > > > > > + size +=3D flow_tcf_vxlan_encap_parse(actions, tunnel) > > > > + > > > > > + RTE_ALIGN_CEIL /* preceding encap params. > > > > */ > > > > > + (sizeof(struct mlx5_flow_tcf_vxlan_encap), > > > > > + MNL_ALIGNTO); > > > > > > > > Is it different from SZ_NLATTR_TYPE_OF(struct > > > > mlx5_flow_tcf_vxlan_encap)? Or, use __rte_aligned(MNL_ALIGNTO) > > instead. > > > > > > It is written intentionally in this form. It means that there is > > > struct mlx5_flow_tcf_vxlan_encap at the beginning of buffer. This is > > > not the NL attribute, usage of SZ_NLATTR_TYPE_OF is not relevant here= . > > Alignment is needed for the following Netlink message. > >=20 > > Good point. Understood. > >=20 > > > > > > > > > + flags |=3D MLX5_ACTION_VXLAN_ENCAP; > > > > > + break; > > > > > + case RTE_FLOW_ACTION_TYPE_VXLAN_DECAP: > > > > > + size +=3D SZ_NLATTR_NEST + /* na_act_index. */ > > > > > + SZ_NLATTR_STRZ_OF("tunnel_key") + > > > > > + SZ_NLATTR_NEST + /* TCA_ACT_OPTIONS. > > > > */ > > > > > + SZ_NLATTR_TYPE_OF(uint8_t); > > > > > + size +=3D SZ_NLATTR_TYPE_OF(struct tc_tunnel_key); > > > > > + size +=3D RTE_ALIGN_CEIL /* preceding decap params. > > > > */ > > > > > + (sizeof(struct mlx5_flow_tcf_vxlan_decap), > > > > > + MNL_ALIGNTO); > > > > > > > > Same here. > > > > > > > > > + flags |=3D MLX5_ACTION_VXLAN_DECAP; > > > > > + break; > > > > > case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC: > > > > > case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST: > > > > > case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC: > > > > > @@ -2289,6 +2578,26 @@ struct pedit_parser { } > > > > > > > > > > /** > > > > > + * Convert VXLAN VNI to 32-bit integer. > > > > > + * > > > > > + * @param[in] vni > > > > > + * VXLAN VNI in 24-bit wire format. > > > > > + * > > > > > + * @return > > > > > + * VXLAN VNI as a 32-bit integer value in network endian. > > > > > + */ > > > > > +static rte_be32_t > > > > > > > > make it inline. > > > OK. Missed point. > > > > > > > > > > > > +vxlan_vni_as_be32(const uint8_t vni[3]) { > > > > > + rte_be32_t ret; > > > > > > > > Defining ret as rte_be32_t? The return value of this func which is > > > > bswap(ret) is also rte_be32_t?? > > > Yes. And it is directly stored in the net-endian NL attribute. > > > I've compiled and checked the listing of the function you proposed. I= t > > seems to be best, I'll take it. > > > > > > > > > > > > + > > > > > + ret =3D vni[0]; > > > > > + ret =3D (ret << 8) | vni[1]; > > > > > + ret =3D (ret << 8) | vni[2]; > > > > > + return RTE_BE32(ret); > > > > > > > > Use rte_cpu_to_be_*() instead. But I still don't understand why you > > > > shuffle bytes twice. One with shift and or and other by bswap(). > > > And it works. There are three bytes in very bizarre order (in NL attr= ibute) - > > 0, vni[0], vni[1], vni[2]. > > > > > > > > > > > { > > > > union { > > > > uint8_t vni[4]; > > > > rte_be32_t dword; > > > > } ret =3D { > > > > .vni =3D { 0, vni[0], vni[1], vni[2] }, > > > > }; > > > > return ret.dword; > > > > } > > > > > > > > This will have the same result without extra cost. > > > > > > OK. Investigated, it is the best for x86-64. Also I'm going to test i= t > > > on the ARM 32, with various compilers, just curious. > > > > > > > > > > > > +} > > > > > + > > > > > +/** > > > > > * Prepare a flow object for Linux TC flower. It calculates the > > > > > maximum > > > > size of > > > > > * memory required, allocates the memory, initializes Netlink > > > > > message > > > > headers > > > > > * and set unique TC message handle. > > > > > @@ -2323,22 +2632,54 @@ struct pedit_parser { > > > > > struct mlx5_flow *dev_flow; > > > > > struct nlmsghdr *nlh; > > > > > struct tcmsg *tcm; > > > > > + struct mlx5_flow_tcf_vxlan_encap encap =3D {.mask =3D 0}; > > > > > + uint8_t *sp, *tun =3D NULL; > > > > > > > > > > size +=3D flow_tcf_get_items_and_size(attr, items, item_flags); > > > > > - size +=3D flow_tcf_get_actions_and_size(actions, action_flags); > > > > > - dev_flow =3D rte_zmalloc(__func__, size, MNL_ALIGNTO); > > > > > + size +=3D flow_tcf_get_actions_and_size(actions, action_flags, > > > > &encap); > > > > > + dev_flow =3D rte_zmalloc(__func__, size, > > > > > + RTE_MAX(alignof(struct mlx5_flow_tcf_tunnel_hdr), > > > > > + (size_t)MNL_ALIGNTO)); > > > > > > > > Why RTE_MAX between the two? Note that it is alignment for start > > > > address of the memory and the minimum alignment is cacheline size. > > > > On x86, non- zero value less than 64 will have same result as 64. > > > > > > OK. Thanks for note. > > > It is not expected the structure alignments exceed the cache line siz= e. > > > So? Just specify zero? > > > > > > > > > if (!dev_flow) { > > > > > rte_flow_error_set(error, ENOMEM, > > > > > RTE_FLOW_ERROR_TYPE_UNSPECIFIED, > > > > NULL, > > > > > "not enough memory to create E-Switch > > > > flow"); > > > > > return NULL; > > > > > } > > > > > - nlh =3D mnl_nlmsg_put_header((void *)(dev_flow + 1)); > > > > > + sp =3D (uint8_t *)(dev_flow + 1); > > > > > + if (*action_flags & MLX5_ACTION_VXLAN_ENCAP) { > > > > > + tun =3D sp; > > > > > + sp +=3D RTE_ALIGN_CEIL > > > > > + (sizeof(struct mlx5_flow_tcf_vxlan_encap), > > > > > + MNL_ALIGNTO); > > > > > > > > And why should it be aligned? > > > > > > Netlink message should be aligned. It follows the > > > mlx5_flow_tcf_vxlan_encap, that's why the pointer is aligned. > >=20 > > Not true. There's no requirement for nl msg buffer alignment. > > MNL_ALIGNTO is for mainly size alignment. For example, checkout the > > source code of mnl_nlmsg_put_header(void *buf). There's no requirement = of > > aligning the start address of buf. But, size of any entries (hdr, attr = ...) should > > be aligned to MNL_ALIGNTO(4). >=20 > Formally speaking, yes. There is no explicit requirement for the header > alignment. And the entire message goes to the send(), it does not care ab= out alignment. > But not aligning the entire structure does not look as a good practice. > ( I had been living for a long time on embedded systems with activated > Alignment Check feature and off unaligned access compiler flags.=20 > There was not very long waiting time to get punishing exception. ) Like mentioned before, I don't have any objection for the alignment. > > > As the size of dev_flow might not be aligned, it > > > > is meaningless, isn't it? If you think it must be aligned for bette= r > > > > performance (not much anyway), you can use > > > > __rte_aligned(MNL_ALIGNTO) on the struct > > > Hm. Where we can use __rte_aligned? Could you clarify, please. > >=20 > > For example, > >=20 > > struct mlx5_flow_tcf_tunnel_hdr { > > uint32_t type; /**< Tunnel action type. */ > > unsigned int ifindex_tun; /**< Tunnel endpoint interface. */ > > unsigned int ifindex_org; /**< Original dst/src interface */ > > unsigned int *ifindex_ptr; /**< Interface ptr in message. */ } > > __rte_aligned(MNL_ALIGNTO); >=20 > No. tunnel_hdr should not know anything about NL message. > It happens, we have the NL message follows the tunnel_hdr=20 > in some our memory buf. What if we would like to add some other > object after tunnel_hdr in buffer? Not NL message?=20 > The aligment of objects is duty of code which places objects into buffer= , > Objects can be very different, with different alignment requirements, > and, generally speaking, placed in arbitrary order. Why, while > declaring the tunnel_hdr structure, we should make an assumption > it is always followed by NL message? _rte_aligned(MNL_ALIGNTO) at the end > of tunnel_hdr - is exactly an example of that unapropriate assumption. Yeah, I agree. That was just an example. And my point was the original code isn't enough to achieve the alignment as= the size of dev_flow isn't aligned. You should've done something like: tun =3D RTE_PTR_ALIGN(dev_flow + 1, MNL_ALIGNTO); In summary, if you want to make it aligned, please fix the issue I raised a= nd improve readability of the code. > > A good example is the struct rte_mbuf. If this attribute is used, the s= ize of the > > struct will be aligned to the value. > >=20 > > If you still want to make the nl msg aligned, > >=20 > > dev_flow =3D rte_zmalloc(..., MNL_ALIGNTO); /* anyway cacheline > > aligned. */ > > tun =3D RTE_PTR_ALIGN(dev_flow + 1, MNL_ALIGNTO); > > nlh =3D mnl_nlmsg_put_header(tun); > >=20 > > with adding '__rte_aligned(MNL_ALIGNTO)' to struct > > mlx5_flow_tcf_vxlan_encap/decap. > >=20 > > Then, nlh will be aligned. You should make sure size is correctly calcu= lated. > >=20 > > > > > > > definition but not for mlx5_flow (it's not only for tcf, have to do= it > > manually). > > > > > > > > > + size -=3D RTE_ALIGN_CEIL > > > > > + (sizeof(struct mlx5_flow_tcf_vxlan_encap), > > > > > + MNL_ALIGNTO); > > > > > > > > Don't you have to subtract sizeof(struct mlx5_flow) as well? But > > > > like I mentioned, if '.nlsize' below isn't needed, you don't need t= o > > > > have this calculation either. > > > Yes, it is a bug. Should be fixed. Thank you. > > > Let's discuss whether we can keep the nlsize under NDEBUG switch. > >=20 > > I agreed on using NDEBUG for it. > >=20 > > > > > > > > > > > > + encap.hdr.type =3D > > > > MLX5_FLOW_TCF_TUNACT_VXLAN_ENCAP; > > > > > + memcpy(tun, &encap, > > > > > + sizeof(struct mlx5_flow_tcf_vxlan_encap)); > > > > > + } else if (*action_flags & MLX5_ACTION_VXLAN_DECAP) { > > > > > + tun =3D sp; > > > > > + sp +=3D RTE_ALIGN_CEIL > > > > > + (sizeof(struct mlx5_flow_tcf_vxlan_decap), > > > > > + MNL_ALIGNTO); > > > > > + size -=3D RTE_ALIGN_CEIL > > > > > + (sizeof(struct mlx5_flow_tcf_vxlan_decap), > > > > > + MNL_ALIGNTO); > > > > > + encap.hdr.type =3D > > > > MLX5_FLOW_TCF_TUNACT_VXLAN_DECAP; > > > > > + memcpy(tun, &encap, > > > > > + sizeof(struct mlx5_flow_tcf_vxlan_decap)); > > > > > + } > > > > > + nlh =3D mnl_nlmsg_put_header(sp); > > > > > tcm =3D mnl_nlmsg_put_extra_header(nlh, sizeof(*tcm)); > > > > > *dev_flow =3D (struct mlx5_flow){ > > > > > .tcf =3D (struct mlx5_flow_tcf){ > > > > > + .nlsize =3D size, > > > > > .nlh =3D nlh, > > > > > .tcm =3D tcm, > > > > > + .tunnel =3D (struct mlx5_flow_tcf_tunnel_hdr *)tun, > > > > > + .item_flags =3D *item_flags, > > > > > + .action_flags =3D *action_flags, > > > > > }, > > > > > }; > > > > > /* > > [...] > > > > > @@ -2827,6 +3268,76 @@ struct pedit_parser { > > > > > (na_vlan_priority) =3D > > > > > conf.of_set_vlan_pcp->vlan_pcp; > > > > > } > > > > > + assert(dev_flow->tcf.nlsize >=3D nlh->nlmsg_len); > > > > > + break; > > > > > + case RTE_FLOW_ACTION_TYPE_VXLAN_DECAP: > > > > > + assert(decap.vxlan); > > > > > + assert(dev_flow->tcf.tunnel); > > > > > + dev_flow->tcf.tunnel->ifindex_ptr > > > > > + =3D (unsigned int *)&tcm->tcm_ifindex; > > > > > + na_act_index =3D > > > > > + mnl_attr_nest_start(nlh, > > > > na_act_index_cur++); > > > > > + assert(na_act_index); > > > > > + mnl_attr_put_strz(nlh, TCA_ACT_KIND, > > > > "tunnel_key"); > > > > > + na_act =3D mnl_attr_nest_start(nlh, > > > > TCA_ACT_OPTIONS); > > > > > + assert(na_act); > > > > > + mnl_attr_put(nlh, TCA_TUNNEL_KEY_PARMS, > > > > > + sizeof(struct tc_tunnel_key), > > > > > + &(struct tc_tunnel_key){ > > > > > + .action =3D TC_ACT_PIPE, > > > > > + .t_action =3D > > > > TCA_TUNNEL_KEY_ACT_RELEASE, > > > > > + }); > > > > > + mnl_attr_nest_end(nlh, na_act); > > > > > + mnl_attr_nest_end(nlh, na_act_index); > > > > > + assert(dev_flow->tcf.nlsize >=3D nlh->nlmsg_len); > > > > > + break; > > > > > + case RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP: > > > > > + assert(encap.vxlan); > > > > > + na_act_index =3D > > > > > + mnl_attr_nest_start(nlh, > > > > na_act_index_cur++); > > > > > + assert(na_act_index); > > > > > + mnl_attr_put_strz(nlh, TCA_ACT_KIND, > > > > "tunnel_key"); > > > > > + na_act =3D mnl_attr_nest_start(nlh, > > > > TCA_ACT_OPTIONS); > > > > > + assert(na_act); > > > > > + mnl_attr_put(nlh, TCA_TUNNEL_KEY_PARMS, > > > > > + sizeof(struct tc_tunnel_key), > > > > > + &(struct tc_tunnel_key){ > > > > > + .action =3D TC_ACT_PIPE, > > > > > + .t_action =3D > > > > TCA_TUNNEL_KEY_ACT_SET, > > > > > + }); > > > > > + if (encap.vxlan->mask & > > > > MLX5_FLOW_TCF_ENCAP_UDP_DST) > > > > > + mnl_attr_put_u16(nlh, > > > > > + TCA_TUNNEL_KEY_ENC_DST_PORT, > > > > > + encap.vxlan->udp.dst); > > > > > + if (encap.vxlan->mask & > > > > MLX5_FLOW_TCF_ENCAP_IPV4_SRC) > > > > > + mnl_attr_put_u32(nlh, > > > > > + TCA_TUNNEL_KEY_ENC_IPV4_SRC, > > > > > + encap.vxlan->ipv4.src); > > > > > + if (encap.vxlan->mask & > > > > MLX5_FLOW_TCF_ENCAP_IPV4_DST) > > > > > + mnl_attr_put_u32(nlh, > > > > > + TCA_TUNNEL_KEY_ENC_IPV4_DST, > > > > > + encap.vxlan->ipv4.dst); > > > > > + if (encap.vxlan->mask & > > > > MLX5_FLOW_TCF_ENCAP_IPV6_SRC) > > > > > + mnl_attr_put(nlh, > > > > > + TCA_TUNNEL_KEY_ENC_IPV6_SRC, > > > > > + sizeof(encap.vxlan->ipv6.src), > > > > > + &encap.vxlan->ipv6.src); > > > > > + if (encap.vxlan->mask & > > > > MLX5_FLOW_TCF_ENCAP_IPV6_DST) > > > > > + mnl_attr_put(nlh, > > > > > + TCA_TUNNEL_KEY_ENC_IPV6_DST, > > > > > + sizeof(encap.vxlan->ipv6.dst), > > > > > + &encap.vxlan->ipv6.dst); > > > > > + if (encap.vxlan->mask & > > > > MLX5_FLOW_TCF_ENCAP_VXLAN_VNI) > > > > > + mnl_attr_put_u32(nlh, > > > > > + TCA_TUNNEL_KEY_ENC_KEY_ID, > > > > > + vxlan_vni_as_be32 > > > > > + (encap.vxlan->vxlan.vni)); > > > > > +#ifdef TCA_TUNNEL_KEY_NO_CSUM > > > > > + mnl_attr_put_u8(nlh, TCA_TUNNEL_KEY_NO_CSUM, > > > > 0); #endif > > > > > > > > TCA_TUNNEL_KEY_NO_CSUM is anyway defined like others, then why do > > > > you treat it differently with #ifdef/#endif? > > > > > > As it was found it is not defined on old kernels, on some our CI > > > machines compilation errors occurred. > >=20 > > In your first patch, TCA_TUNNEL_KEY_NO_CSUM is defined if there isn't > > HAVE_TC_ACT_TUNNEL_KEY. Actually I'm wondering why it is different from > > HAVE_TCA_TUNNEL_KEY_ENC_DST_PORT. It looks like the following is > > needed - HAVE_TCA_TUNNEL_KEY_NO_CSUM ?? > >=20 > >=20 > > #ifdef HAVE_TC_ACT_TUNNEL_KEY > >=20 > > #include > >=20 > > #ifndef HAVE_TCA_TUNNEL_KEY_ENC_DST_PORT > > #define TCA_TUNNEL_KEY_ENC_DST_PORT 9 > > #endif > >=20 > > #ifndef HAVE_TCA_TUNNEL_KEY_NO_CSUM > > #define TCA_TUNNEL_KEY_NO_CSUM 10 > > #endif >=20 > I think it is subject to check. Yes, we can define the "missing" > macros, but it seems the old kernel just does not know these > keys. Whether the rule with these keys is accepted by kernel? > I did not check (have no host with old setup to check), > I'd prefer to exclude not very significant key to lower the > rule rejection risk.=20 My question is that why the two missing macros (TCA_TUNNEL_KEY_ENC_DST_PORT= and TCA_TUNNEL_KEY_NO_CSUM) are treated differently? AFAIK, the reason for defi= ning it manually for old kernel is that it can be run on a different host from t= he compile host. Even though the compile machine doesn't have the feature, but= it can be supported on the machine it runs on. If kernel doesn't understand it= on an old machine, an error will be returned, which is fine. Thanks, Yongseok > >=20 > > #else /* HAVE_TC_ACT_TUNNEL_KEY */ > >=20 > >=20