From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0048.outbound.protection.outlook.com [104.47.1.48]) by dpdk.org (Postfix) with ESMTP id 5BC865F17 for ; Tue, 9 Oct 2018 10:55:31 +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=fPoGGCiRsyrasB+1ZPopd0MX/qEHA+P/c8rsnlG68G0=; b=bXIAjJ95/Cp7LPaxm+kwJw/H1DdbpgKADEUUdBpyxofvkIt8vWYNTxC2bdwBBQGahTEodVUQScwFTzU2FXd6HIU65RGE0P9ft7iltISeEGz6Uef4Eihe0cHxnJPsR3c/SgX2WR1j6DVWjJTOrFrfPXT4SSRPXrpAeCjQiPsuLuo= Received: from DB3PR0502MB3980.eurprd05.prod.outlook.com (52.134.72.27) by DB3PR0502MB3946.eurprd05.prod.outlook.com (52.134.71.23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1228.21; Tue, 9 Oct 2018 08:55:29 +0000 Received: from DB3PR0502MB3980.eurprd05.prod.outlook.com ([fe80::1cb0:661b:ecab:6045]) by DB3PR0502MB3980.eurprd05.prod.outlook.com ([fe80::1cb0:661b:ecab:6045%2]) with mapi id 15.20.1207.024; Tue, 9 Oct 2018 08:55:29 +0000 From: Yongseok Koh To: Jack Min CC: Shahaf Shuler , "dev@dpdk.org" Thread-Topic: [PATCH] net/mlx5: eswitch-IP address UDP/TCP port rewrite Thread-Index: AQHUVMYzsnQsXgDklk+QshXlrRt6gqUF4ASAgAKSugCAAfZJgIAK37KAgAA/PACAAQrtAIAAHzeA Date: Tue, 9 Oct 2018 08:55:29 +0000 Message-ID: <7BBA09A8-00D5-43D4-9215-72DE4580493B@mellanox.com> References: <20180925115107.12242-1-jackmin@mellanox.com> <20180928230318.GA90324@minint-98vp2qg> <20180930072104.f7o5c6yazgznivzd@MTBC-JACKMIN.mtl.com> <20181001201752.GA21384@yongseok-MBP.local> <20181008112203.kxk7bk5jl4rega6h@MTBC-JACKMIN.mtl.com> <20181008220822.GC9031@mtidpdk.mti.labs.mlnx> <20181009070323.kdbje4eyg7dzmca6@MTBC-JACKMIN.mtl.com> In-Reply-To: <20181009070323.kdbje4eyg7dzmca6@MTBC-JACKMIN.mtl.com> 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=yskoh@mellanox.com; x-originating-ip: [69.181.245.183] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DB3PR0502MB3946; 6:K1ZldVLWLincerF+LWRKKo0EeNegSwe5wG3RHcbBUNRfp/ckymXao5meDtNVPT+VQUsYkVA2N3r2nWCrvskYQgAqEf9QUP19+UsLn/uRuxKXKE+pRRpuz1skr3Yp3qVFE21VjWEwTS7s8Ce/tMKtrDWEoBAHOTqb4f5hl8rc+kGP7fzcB+yCdYFM2mz8TEbV26ylep2aAZ+Gz9mQQPL+4TSbo8af0up5II6RbJ0AeYMrnEwZE1SiPkvGAMuEI3PNnbYVAPQ5xmjQuvPBuPHiLIwzT/jonCRk2MS9JWm/XNJCrWF1RyRTvRXm5s7AgI/jVN3uO5Ze0ttK4Saz9ENfwrYeq2U27uLSPPl0OlwHxVDmic8SYuqYAoyL0Xi2nUQ2MAjN2kVnZ7s0kxZ6CVj/4pmLOYNsuqJ6M/PU4ZN+KJKrAUujlUesgvewtdq+ekIvb8UCvk65Z7sOdwo7lNtLSg==; 5:0o86rPtlBWi5hNK3pYk4tCVZg5HRv6nb/hUjZi6VOJNIzzG30bkOEnH8RFYoqeG3uyeqXFL1OSr+krxZOYbmnhkbixe80NaN4+GVnGWxPImo0+/kC0vKynAM9sORlpw6vb2KkZgqq7JPold04ALKr2HuPxsZ9M1fAgbhj5XaayM=; 7:9ab3a9k29cyKBt2ZydQBAIi0zxt7SZLeXO6cPxEO7JvHStp1kLJljqyb/BbaKpEsgc4snmQgO8LxJ7rDDuD0XF4uBmfFjqsr3om7GDoBp42hTZleNp32OD2iP0pU9Mv2WeaNr/HY8UXmWbaUyuYge5zZNTELPSOkzWhz6H2lRU+uhdxzc7t2Q0eLW2Kl0+DimgF5Xgk0xs8p+iDDyAntWUWu1dVIk0P1EBBpBS36ihFVpas2/bu/ZH2agEfKtUoy x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-correlation-id: 11eadb35-6a44-4585-bf8c-08d62dc4f69d x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(7020095)(4652040)(8989299)(5600074)(711020)(4618075)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7153060)(7193020); SRVR:DB3PR0502MB3946; x-ms-traffictypediagnostic: DB3PR0502MB3946: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:(17755550239193); x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(5005006)(8121501046)(3002001)(3231355)(944501410)(52105095)(10201501046)(93006095)(93001095)(6055026)(149066)(150057)(6041310)(20161123564045)(20161123558120)(20161123560045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(201708071742011)(7699051); SRVR:DB3PR0502MB3946; BCL:0; PCL:0; RULEID:; SRVR:DB3PR0502MB3946; x-forefront-prvs: 08200063E9 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(396003)(136003)(366004)(346002)(376002)(39860400002)(199004)(189003)(8936002)(6486002)(106356001)(186003)(105586002)(2616005)(446003)(11346002)(486006)(476003)(6512007)(6306002)(8676002)(6436002)(81166006)(81156014)(102836004)(53936002)(26005)(82746002)(5660300001)(6636002)(305945005)(7736002)(97736004)(54906003)(93886005)(316002)(3846002)(71200400001)(71190400001)(6116002)(83716004)(37006003)(6246003)(6506007)(53546011)(2906002)(4326008)(6862004)(229853002)(25786009)(66066001)(36756003)(68736007)(478600001)(14454004)(5250100002)(966005)(99286004)(76176011)(2900100001)(256004)(4744004)(86362001)(33656002)(14444005); DIR:OUT; SFP:1101; SCL:1; SRVR:DB3PR0502MB3946; H:DB3PR0502MB3980.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: EmPT43yk9M/rRc2tye3/s98cy0lszWoVK6sbtd8y5P93P3uBn2qJrSibJ10yYFQ3b5LayrQUZwtcb7a/waD23EZkCLrlFt9R7077a2RXrXr7KFszz/VxpP3n940fETzzYHJ/s6SmHITglzwT6yzFhJUWsLrsXjoPsIxrkE1Mk+D8z2rqR6iahAh9p9LjBTCm62Tw8bNNLLn9rz54mr699r2BOFWqSx+ULO1L4r8bh7PyR8G4IaUVeAxdtlb4uFrK59S+JW45uCnvZUVYQEv/fUhTrSyC6DMYjt56HgtF32JOUubU1HiFgF8P2I6IZyYRli2rNfxU4lM8ycOHznUkSKhln1nnpyblBLtKNlRmdX0= 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: 11eadb35-6a44-4585-bf8c-08d62dc4f69d X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Oct 2018 08:55:29.2726 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB3PR0502MB3946 Subject: Re: [dpdk-dev] [PATCH] net/mlx5: eswitch-IP address UDP/TCP port rewrite 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: Tue, 09 Oct 2018 08:55:31 -0000 > On Oct 9, 2018, at 12:03 AM, Jack Min wrote: >=20 > On 18-10-09 06:08:32, Yongseok Koh wrote: >> On Mon, Oct 08, 2018 at 07:22:03PM +0800, Xiaoyu Min wrote: >>> On 18-10-02 04:19:00, Yongseok Koh wrote: >>>> On Sun, Sep 30, 2018 at 03:21:04PM +0800, Xiaoyu Min wrote: >>>>> On 18-09-29 07:03:33, Yongseok Koh wrote: >>>>>> On Tue, Sep 25, 2018 at 07:51:06PM +0800, Xiaoyu Min wrote: [...] >>>>>=20 >>>>>>> + p_parser->keys[idx].mask =3D 0xFFFF0000; >>>>>>> + p_parser->keys[idx].val =3D ((const struct rte_flow_action_set_tp= *) >>>>>>> + actions->conf)->port; >>>>>>=20 >>>>>> Assigning 2B to 4B big-endian stroage? Doesn't look consistent with = the mask >>>>>> above - 0xffff0000. >>>>>>=20 >>>>> So it should be as following ? >>>>> p_parser->keys[idx].val =3D (__u32)((const struct rte_flow_action_set= _tp *)) >>>>> actions->conf)->port; >>>>=20 >>>> You can figure it out by actual tests but I think the following would = be right. >>>> p_parser->keys[idx].val =3D >>>> rte_cpu_to_be_32(((const struct rte_flow_action_set_tp *) >>>> actions->conf)->port); >>>>=20 >>>> Please verify it by testing anyway. >>>>=20 >>> No, it doesn't work correctly if it's converted to BE. >>> As my understanding the Netlink message should be expressed in host-byt= e order (?) >>=20 >> As far as I know rte_flow takes network-byte order for args and tcf na m= sg takes >> host-byte order. Please refer to the "override_na_vlan_id:" in mlx5_flow= _tcf.c, >> rte_be_to_cpu_16() is used there. I'm still confusing why the mask is >> 0xffff0000 above. Please make sure your code works correctly by multiple= test >> cases and hopefully I can hear clear explanation what is right and why. >>=20 > Hey Koh, > 1. yes, rte_flow takes network-byte order. Here the 'port' is already con= verted to > BE by testpmd. > If the rte_be_to_cpu_16 is used just like "override_na_vlan_id" does the = result > is not correct. Things like TC-pedit takes network-byte order. > 2. For the mask, honestly, I'm not very clear. The only reference is from= [1] and=20 > my verification by using TC show command. > [1] https://www.netdevconf.org/2.1/slides/apr7/tc-workshop/vadai-tc-works= hop-update.pdf Interesting. Please go ahead with your code because you have already verified it. >>>>>>> + p_parser->sel.nkeys =3D (++idx); >>>>>>> +} >>>>>>> + >>>>>>> +static void >>>>>>> +flow_tcf_pedit_key_set_ipv6_addr(const struct rte_flow_action *act= ions, >>>>>>> + struct pedit_parser *p_parser) >>>>>>> +{ >>>>>>> + int idx =3D p_parser->sel.nkeys; >>>>>>> + int keys =3D flow_tcf_calc_pedit_keys(IPV6_ADDR_LEN); >>>>>>> + int off_base =3D >>>>>>> + actions->type =3D=3D RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC ? 8 : 24; >>>>>>=20 >>>>>> offsetof(struct ipv6_hdr, src_addr) : >>>>>> offsetof(struct ipv6_hdr, dst_addr); >>>>>>=20 >>>>> Got it! >>>>>>> + const struct rte_flow_action_set_ipv6 *conf =3D >>>>>>> + (const struct rte_flow_action_set_ipv6 *)actions->conf; >>>>>>> + >>>>>>> + for (int i =3D 0; i < keys; i++, idx++) { >>>>>>> + p_parser->keys_ex[idx].htype =3D TCA_PEDIT_KEY_EX_HDR_TYPE_IP6; >>>>>>> + p_parser->keys_ex[idx].cmd =3D TCA_PEDIT_KEY_EX_CMD_SET; >>>>>>> + p_parser->keys[idx].off =3D off_base + i * SZ_PEDIT_KEY_VAL; >>>>>>> + p_parser->keys[idx].mask =3D ~UINT32_MAX; >>>>>>> + memcpy(&p_parser->keys[idx].val, >>>>>>> + conf->ipv6_addr + i * SZ_PEDIT_KEY_VAL, >>>>>>> + SZ_PEDIT_KEY_VAL); >>>>>>> + } >>>>>>> + p_parser->sel.nkeys +=3D keys; >>>>>>> +} >>>>>>> + >>>>>>> +static void >>>>>>> +flow_tcf_pedit_key_set_ipv4_addr(const struct rte_flow_action *act= ions, >>>>>>=20 >>>>>> How about getting rte_flow_action_set_ipv4 instead of rte_flow_actio= n? >>>>>> Same comment for ipv6 and tp_port. >>>>>>=20 >>>>> What's the benefit by using rte_flow_action_set_ipv4 and how I know i= t's >>>>> for src or dst address ? >>>>=20 >>>> Just to make the function neat but I overlooked that you still need >>>> actions->type. Please disregard my previous comment. >>>>=20 >>> OK~ >>>>>>> + struct pedit_parser *p_parser) >>>>>>> +{ >>>>>>> + int idx =3D p_parser->sel.nkeys; >>>>>>> + >>>>>>> + p_parser->keys_ex[idx].htype =3D TCA_PEDIT_KEY_EX_HDR_TYPE_IP4; >>>>>>> + p_parser->keys_ex[idx].cmd =3D TCA_PEDIT_KEY_EX_CMD_SET; >>>>>>> + p_parser->keys[idx].off =3D >>>>>>> + (actions->type =3D=3D RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC ? 12 : 1= 6); >>>>>>=20 >>>>>> offsetof(struct ipv4_hdr, src_addr) : >>>>>> offsetof(struct ipv4_hdr, dst_addr); >>>>>>=20 >>>>> Got it! >>>>>>> + p_parser->keys[idx].mask =3D ~UINT32_MAX; >>>>>>> + p_parser->keys[idx].val =3D >>>>>>> + ((const struct rte_flow_action_set_ipv4 *) >>>>>>> + actions->conf)->ipv4_addr; >>>>>>> + p_parser->sel.nkeys =3D (++idx); >>>>>>> +} >>>>>>> + >>>>>>> +static int >>>>>>> +flow_tcf_create_pedit_mnl_msg(struct nlmsghdr *nl, >>>>>>> + const struct rte_flow_action **actions, >>>>>>> + uint64_t item_flags) >>>>>>> +{ >>>>>>> + struct pedit_parser p_parser; >>>>>>> + >>>>>>> + memset(&p_parser, 0, sizeof(p_parser)); >>>>>>> + mnl_attr_put_strz(nl, TCA_ACT_KIND, "pedit"); >>>>>>> + struct nlattr *na_act_options =3D mnl_attr_nest_start(nl, >>>>>>> + TCA_ACT_OPTIONS); >>>>>>> + /* all modify header actions should be in one tc-pedit action */ >>>>>>> + for (; (*actions)->type !=3D RTE_FLOW_ACTION_TYPE_END; (*actions)= ++) { >>>>>>=20 >>>>>> It seems that you want to aggregate all the pedit actions and form a= single >>>>>> na attr. But what if rte_flow_action_set_* are not contiguous? E.g. >>>>>>=20 >>>>>> flow create ... actions set1 / set2 / port_id / set3 / end >>>>>>=20 >>>>>> Then, it would have two pedit na attrs. Is that okay? >>>>>> Or, need to think about another way? >>>>>>=20 >>>>>> Same will happen in flow_tcf_get_pedit_actions_size(). >>>>>>=20 >>>>> It's OK if we have more than one pedit na attrs. >>>>> _BUT_ only last pedit take effect based on my experiment >>>>=20 >>>> Then, shouldn't we give some warning to user in validation? So that us= er can >>>> have right expectation and reorder the actions as their intention like= : >>>> flow create ... actions set1 / set2 / set3 / port_id / end >>>>=20 >>>> Otherwise set1 and set2 will be lost according to your comment. >>>>=20 >>> I prefer to give error to user in validation because this is simple. >>=20 >> Good. >>=20 >>>> Or, how about making PMD do the right thing. I mean, even if the set a= ctions are >>>> scattered, PMD can collect it and apply in a single na attr? >>>>=20 >>> My feeling is the above approach will be (become) complex. It looks lik= e we introduce >>> new functionality which re-order all actions, something like rss_expand= .=20 >>=20 >> +1 >>=20 >>>>>>> + switch ((*actions)->type) { >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC: >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST: >>>>>>> + flow_tcf_pedit_key_set_ipv4_addr(*actions, &p_parser); >>>>>>> + break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC: >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_IPV6_DST: >>>>>>> + flow_tcf_pedit_key_set_ipv6_addr(*actions, &p_parser); >>>>>>> + break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_TP_SRC: >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_TP_DST: >>>>>>> + flow_tcf_pedit_key_set_tp_port(*actions, >>>>>>> + &p_parser, item_flags); >>>>>>> + break; >>>>>>> + default: >>>>>>> + goto pedit_mnl_msg_done; >>>>>>> + } >>>>>>> + } >>>>>>> +pedit_mnl_msg_done: >>>>>>> + p_parser.sel.action =3D TC_ACT_PIPE; >>>>>>> + mnl_attr_put(nl, TCA_PEDIT_PARMS_EX, >>>>>>> + sizeof(p_parser.sel) + >>>>>>> + p_parser.sel.nkeys * sizeof(struct tc_pedit_key), >>>>>>> + &p_parser); >>>>>>> + struct nlattr *na_pedit_keys =3D mnl_attr_nest_start(nl, >>>>>>> + TCA_PEDIT_KEYS_EX | NLA_F_NESTED); >>>>>>> + for (int i =3D 0; i < p_parser.sel.nkeys; i++) { >>>>>>> + struct nlattr *na_pedit_key =3D mnl_attr_nest_start(nl, >>>>>>> + TCA_PEDIT_KEY_EX | NLA_F_NESTED); >>>>>>> + mnl_attr_put_u16(nl, TCA_PEDIT_KEY_EX_HTYPE, >>>>>>> + p_parser.keys_ex[i].htype); >>>>>>> + mnl_attr_put_u16(nl, TCA_PEDIT_KEY_EX_CMD, >>>>>>> + p_parser.keys_ex[i].cmd); >>>>>>> + mnl_attr_nest_end(nl, na_pedit_key); >>>>>>> + } >>>>>>> + mnl_attr_nest_end(nl, na_pedit_keys); >>>>>>> + mnl_attr_nest_end(nl, na_act_options); >>>>>>> + (*actions)--; >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> +/** >>>>>>> + * Calculate max memory size of one TC-pedit actions. >>>>>>> + * One TC-pedit action can contain set of keys each defining >>>>>>> + * a rewrite element (rte_flow action) >>>>>>> + * >>>>>>> + * @param[in] actions >>>>>>> + * actions specification. >>>>>>> + * @param[inout] action_flags >>>>>>> + * actions flags >>>>>>> + * @param[inout] size >>>>>>> + * accumulated size >>>>>>> + * @return >>>>>>> + * Max memory size of one TC-pedit action >>>>>>> + */ >>>>>>> +static int >>>>>>> +flow_tcf_get_pedit_actions_size(const struct rte_flow_action **act= ions, >>>>>>> + uint64_t *action_flags) >>>>>>> +{ >>>>>>> + int pedit_size =3D 0; >>>>>>> + int keys =3D 0; >>>>>>> + uint64_t flags =3D 0; >>>>>>> + >>>>>>> + pedit_size +=3D SZ_NLATTR_NEST + /* na_act_index. */ >>>>>>> + SZ_NLATTR_STRZ_OF("pedit") + >>>>>>> + SZ_NLATTR_NEST; /* TCA_ACT_OPTIONS. */ >>>>>>> + for (; (*actions)->type !=3D RTE_FLOW_ACTION_TYPE_END; (*actions)= ++) { >>>>>>> + switch ((*actions)->type) { >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC: >>>>>>> + keys +=3D flow_tcf_calc_pedit_keys(IPV4_ADDR_LEN); >>>>>>> + flags |=3D MLX5_ACTION_SET_IPV4_SRC; >>>>>>> + break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST: >>>>>>> + keys +=3D flow_tcf_calc_pedit_keys(IPV4_ADDR_LEN); >>>>>>> + flags |=3D MLX5_ACTION_SET_IPV4_DST; >>>>>>> + break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC: >>>>>>> + keys +=3D flow_tcf_calc_pedit_keys(IPV6_ADDR_LEN); >>>>>>> + flags |=3D MLX5_ACTION_SET_IPV6_SRC; >>>>>>> + break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_IPV6_DST: >>>>>>> + keys +=3D flow_tcf_calc_pedit_keys(IPV6_ADDR_LEN); >>>>>>> + flags |=3D MLX5_ACTION_SET_IPV6_DST; >>>>>>> + break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_TP_SRC: >>>>>>> + /* TCP is as same as UDP */ >>>>>>> + keys +=3D flow_tcf_calc_pedit_keys(TP_PORT_LEN); >>>>>>> + flags |=3D MLX5_ACTION_SET_TP_SRC; >>>>>>> + break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_TP_DST: >>>>>>> + /* TCP is as same as UDP */ >>>>>>> + keys +=3D flow_tcf_calc_pedit_keys(TP_PORT_LEN); >>>>>>> + flags |=3D MLX5_ACTION_SET_TP_DST; >>>>>>> + break; >>>>>>> + default: >>>>>>> + goto get_pedit_action_size_done; >>>>>>> + } >>>>>>> + } >>>>>>> +get_pedit_action_size_done: >>>>>>> + /* TCA_PEDIT_PARAMS_EX */ >>>>>>> + pedit_size +=3D SZ_NLATTR_DATA_OF(sizeof(struct tc_pedit_sel) + >>>>>>> + keys * sizeof(struct tc_pedit_key)); >>>>>>=20 >>>>>>> + pedit_size +=3D SZ_NLATTR_NEST; /* TCA_PEDIT_KEYS */ >>>>>>> + pedit_size +=3D keys * >>>>>>> + /* TCA_PEDIT_KEY_EX + HTYPE + CMD */ >>>>>>> + (SZ_NLATTR_NEST + SZ_NLATTR_DATA_OF(2) + SZ_NLATTR_DATA_OF(2)); >>>>>>> + (*action_flags) |=3D flags; >>>>>>> + (*actions)--; >>>>>>> + return pedit_size; >>>>>>> +} >>>>>>> + >>>>>>> /** >>>>>>> * Retrieve mask for pattern item. >>>>>>> * >>>>>>> @@ -430,6 +708,8 @@ flow_tcf_validate(struct rte_eth_dev *dev, >>>>>>> of_set_vlan_vid; >>>>>>> const struct rte_flow_action_of_set_vlan_pcp * >>>>>>> of_set_vlan_pcp; >>>>>>> + const struct rte_flow_action_set_ipv4 *set_ipv4; >>>>>>> + const struct rte_flow_action_set_ipv6 *set_ipv6; >>>>>>> } conf; >>>>>>> uint32_t item_flags =3D 0; >>>>>>> uint32_t action_flags =3D 0; >>>>>>> @@ -690,12 +970,64 @@ flow_tcf_validate(struct rte_eth_dev *dev, >>>>>>> case RTE_FLOW_ACTION_TYPE_OF_SET_VLAN_PCP: >>>>>>> action_flags |=3D MLX5_ACTION_OF_SET_VLAN_PCP; >>>>>>> break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC: >>>>>>> + action_flags |=3D MLX5_ACTION_SET_IPV4_SRC; >>>>>>> + break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_IPV4_DST: >>>>>>> + action_flags |=3D MLX5_ACTION_SET_IPV4_DST; >>>>>>> + break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_IPV6_SRC: >>>>>>> + action_flags |=3D MLX5_ACTION_SET_IPV6_SRC; >>>>>>> + break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_IPV6_DST: >>>>>>> + action_flags |=3D MLX5_ACTION_SET_IPV6_DST; >>>>>>> + break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_TP_SRC: >>>>>>> + action_flags |=3D MLX5_ACTION_SET_TP_SRC; >>>>>>> + break; >>>>>>> + case RTE_FLOW_ACTION_TYPE_SET_TP_DST: >>>>>>> + action_flags |=3D MLX5_ACTION_SET_TP_DST; >>>>>>> + break; >>>>>>> default: >>>>>>> return rte_flow_error_set(error, ENOTSUP, >>>>>>> RTE_FLOW_ERROR_TYPE_ACTION, >>>>>>> actions, >>>>>>> "action not supported"); >>>>>>> } >>>>>>> + if (IS_MODIFY_ACTION(actions->type)) { >>>>=20 >>>> This would be a redundant 'if' as classification is already done above= . So, how >>>> about adding a goto label at the end of this code - 'err_no_action_con= f:', and >>>> use goto above. E.g., >>>>=20 >>>> case RTE_FLOW_ACTION_TYPE_SET_TP_DST: >>>> action_flags |=3D MLX5_ACTION_SET_TP_DST; >>>> if (!actions->conf) >>>> goto err_no_action_conf; >>>> break; >>>>=20 >>> In some level I do agree with you it's redundant. But things like this = kind of >>> redundancy is not avoidable. I mean if we use "goto err_no_action_conf"= , the >>> "if (!actions->conf) goto err_no_action_conf" has to be repeated in eac= h "case" >>> which needs to check conf or you think it's acceptable ? >>=20 >> But in your approach, the redundant check (IS_MODIFY_ACTION) is inside a= loop >> and it has to be checked even if there's no 'set' action in the action l= ist. >>=20 > Yes, it will check very time. But I doubt it will impact performace signi= ficantly. Not a performance issue but I just wanted to make the code modular as much = as possible. > Another thing bother me is, if taking 'goto' approach, beside the 'if (!a= ctions->conf)...', > at this moment, we also need to add 'if (set action is discontinued)' for > each modification case. I'm not sure if this will keep code clear > considering many repeated code there... And like I asked, this sanity check is also needed for other actions (PORT_= ID and VLAN). That's why I just wanted to have more generic way. I'm not sure which way is clearer considering all that. I don't have a strong objection about your current code but please make sur= e you add the same sanity check for other existing acitons in a unified way. Also I hope = the macro (IS_MODIFY_ACTION) has better name. Thanks, Yongseok >>>>=20 >>>> And if I may, can I ask you to add the same to RTE_FLOW_ACTION_TYPE_PO= RT_ID? >>>>=20 >>> Yes, I will add. >>>>>>> + if (!actions->conf) >>>>>>> + return rte_flow_error_set(error, ENOTSUP, >>>>>>> + RTE_FLOW_ERROR_TYPE_ACTION_CONF, >>>>>>> + actions, >>>>>>> + "action configuration not set"); >>>>>>> + } >>>>>>> + }