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 84365A04E6; Wed, 18 Nov 2020 05:17:01 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6AACE58C4; Wed, 18 Nov 2020 05:17:00 +0100 (CET) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id DF0D04C90 for ; Wed, 18 Nov 2020 05:16:57 +0100 (CET) IronPort-SDR: fRTfmOx+9/1bux/0F3ih0ufWhFO8RGnM2E3NaMCxGyo6oh9BW38PTlR98Rfay8pGO0N2gRFAZe F1bzjqenVZ+Q== X-IronPort-AV: E=McAfee;i="6000,8403,9808"; a="255772883" X-IronPort-AV: E=Sophos;i="5.77,486,1596524400"; d="scan'208";a="255772883" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Nov 2020 20:16:55 -0800 IronPort-SDR: z4Y6drKJ9ZB7JqOHhzvBBwD6OK0TTAh2A+T70VNAEFBinLS5oGHsnr8qTiIcL5ixQiHApiaa61 qnvCI9Qs98qA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,486,1596524400"; d="scan'208";a="359167618" Received: from fmsmsx603.amr.corp.intel.com ([10.18.126.83]) by fmsmga004.fm.intel.com with ESMTP; 17 Nov 2020 20:16:55 -0800 Received: from shsmsx606.ccr.corp.intel.com (10.109.6.216) by fmsmsx603.amr.corp.intel.com (10.18.126.83) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Tue, 17 Nov 2020 20:16:54 -0800 Received: from shsmsx605.ccr.corp.intel.com (10.109.6.215) by SHSMSX606.ccr.corp.intel.com (10.109.6.216) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Wed, 18 Nov 2020 12:16:52 +0800 Received: from shsmsx605.ccr.corp.intel.com ([10.109.6.215]) by SHSMSX605.ccr.corp.intel.com ([10.109.6.215]) with mapi id 15.01.1713.004; Wed, 18 Nov 2020 12:16:52 +0800 From: "Cao, Yahui" To: "Yan, Zhirun" CC: "dev@dpdk.org" , "Zhang, Qi Z" , "Wang, Xiao W" , "Su, Simei" Thread-Topic: [PATCH v1] net/ice: refactor flow pattern parser Thread-Index: AQHWvL7y6e0lGtQm6ESFbbHnGjUYX6nMbyPggAFHowD//48d4A== Date: Wed, 18 Nov 2020 04:16:52 +0000 Message-ID: References: <20201117084524.3610038-1-zhirun.yan@intel.com> <567972babaca49939efeebe98859eb50@intel.com> <3cb3569f17574168accda2bd6cdde6a1@intel.com> In-Reply-To: <3cb3569f17574168accda2bd6cdde6a1@intel.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-reaction: no-action dlp-version: 11.5.1.3 x-originating-ip: [10.239.127.36] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v1] net/ice: refactor flow pattern parser 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" > -----Original Message----- > From: Yan, Zhirun > Sent: Wednesday, November 18, 2020 11:15 AM > To: Cao, Yahui > Cc: dev@dpdk.org; Zhang, Qi Z ; Wang, Xiao W ; Su, Simei > Subject: [PATCH v1] net/ice: refactor flow pattern parser >=20 > * Cao, Yahui [2020-11-17 23:31:40 +0800]: >=20 > > Hi Zhirun, > > > > I think it is great to refactor ice fdir to differentiate inne= r and outer field vector in a more clear way. Thanks for your commit. > > It seems there still needs some effort to complete this patch. > > > > > -----Original Message----- > > > From: Yan, Zhirun > > > Sent: Tuesday, November 17, 2020 4:45 PM > > > To: dev@dpdk.org; Zhang, Qi Z ; Cao, Yahui ; Wang, Xiao W > ; > > > Su, Simei > > > Cc: Yan, Zhirun > > > Subject: [PATCH v1] net/ice: refactor flow pattern parser > > > > > > Distinguish inner/outer fields. And avoid too many nested conditional= s > > > in each type's parser. > > [Cao, Yahui] > > Since this is quite a huge refactor, could you give a more detailed des= cription in the commit message? Thanks. > > It would be better if you can make them a patchset. > > > Thanks Yahui, will add more details. All changes in one function. >=20 > > > > > > Signed-off-by: Zhirun Yan > > > --- > > > drivers/net/ice/ice_fdir_filter.c | 504 ++++++++++++++++------------= -- > > > 1 file changed, 269 insertions(+), 235 deletions(-) > > > > > > diff --git a/drivers/net/ice/ice_fdir_filter.c b/drivers/net/ice/ice_= fdir_filter.c > > > index 175abcdd5c..b53ed30b1c 100644 > > > --- a/drivers/net/ice/ice_fdir_filter.c > > > +++ b/drivers/net/ice/ice_fdir_filter.c > > > @@ -1646,7 +1646,9 @@ ice_fdir_parse_pattern(__rte_unused struct ice_= adapter *ad, > > > const struct rte_flow_item_vxlan *vxlan_spec, *vxlan_mask; > > > const struct rte_flow_item_gtp *gtp_spec, *gtp_mask; > > > const struct rte_flow_item_gtp_psc *gtp_psc_spec, *gtp_psc_mask; > > > - uint64_t input_set =3D ICE_INSET_NONE; > > > + uint64_t inner_input_set =3D ICE_INSET_NONE; > > > + uint64_t outer_input_set =3D ICE_INSET_NONE; > > > + uint64_t *input_set; > > > uint8_t flow_type =3D ICE_FLTR_PTYPE_NONF_NONE; > > > uint8_t ipv6_addr_mask[16] =3D { > > > 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, > > > @@ -1655,289 +1657,315 @@ ice_fdir_parse_pattern(__rte_unused struct = ice_adapter *ad, > > > uint32_t vtc_flow_cpu; > > > uint16_t ether_type; > > > enum rte_flow_item_type next_type; > > > + bool is_outer =3D true; > > > + struct ice_fdir_extra *p_ext_data; > > > + struct ice_fdir_v4 *p_v4; > > > + struct ice_fdir_v6 *p_v6; > > > > > > + for (item =3D pattern; item->type !=3D RTE_FLOW_ITEM_TYPE_END; item= ++) { > > > + if (item->type =3D=3D RTE_FLOW_ITEM_TYPE_VXLAN) { > > > + tunnel_type =3D ICE_FDIR_TUNNEL_TYPE_VXLAN; > > > + break; > > > + } > > [Cao, Yahui] > > You should take both of VXLAN and GTP cases into consideration >=20 > Yes, will add GTP in v2. >=20 > > > + } > > > + > > > + /* This loop parse flow pattern and distinguish Non-tunnel and tunn= el > > > + * flow. For tunnel flow, reuse non-tunnel structure to track inner > > > + * part. > > > + * > > > + * is_outer tunnel_type p_input_set input_set_bit data_struc= t > > > + * Non-Tun Y N inner outer origin > > > + * Tun-out Y Y outer outer outer > > > + * Tun-in N Y inner inner origin > > > + */ > > > for (item =3D pattern; item->type !=3D RTE_FLOW_ITEM_TYPE_END; item= ++) { > > > if (item->last) { > > > rte_flow_error_set(error, EINVAL, > > > - RTE_FLOW_ERROR_TYPE_ITEM, > > > - item, > > > - "Not support range"); > > > + RTE_FLOW_ERROR_TYPE_ITEM, > > > + item, > > > + "Not support range"); > > > return -rte_errno; > > > } > > > item_type =3D item->type; > > > > > > + input_set =3D (tunnel_type && is_outer) ? > > > + &outer_input_set : > > > + &inner_input_set; > > > + > > > + if (l3 =3D=3D RTE_FLOW_ITEM_TYPE_IPV4) > > > + p_v4 =3D (tunnel_type && is_outer) ? > > > + &filter->input.ip_outer.v4 : > > > + &filter->input.ip.v4; > > > + if (l3 =3D=3D RTE_FLOW_ITEM_TYPE_IPV6) > > > + p_v6 =3D (tunnel_type && is_outer) ? > > > + &filter->input.ip_outer.v6 : > > > + &filter->input.ip.v6; > > > + > > [Cao, Yahui] > > Why do you put p_v4 value assignment out of switch case RTE_FLOW_ITEM_T= YPE_IPV4? > > Why do you put p_v6 value assignment out of switch case RTE_FLOW_ITEM_T= YPE_IPV6? > > >=20 > Yes, for RTE_FLOW_ITEM_TYPE_IPV4/6, p_v4/6 assigned in each case as you > say. >=20 > But this is only for L4 layer parser. For L4 layer, the p_v4/6 is the sha= red pointer > will be used after L3 loop. >=20 >=20 > > > switch (item_type) { > > > case RTE_FLOW_ITEM_TYPE_ETH: > > > + flow_type =3D ICE_FLTR_PTYPE_NON_IP_L2; > > > eth_spec =3D item->spec; > > > eth_mask =3D item->mask; > > > - next_type =3D (item + 1)->type; > > > > > > - if (eth_spec && eth_mask) { > > > - if (!rte_is_zero_ether_addr(ð_mask->dst)) { > > > - input_set |=3D ICE_INSET_DMAC; > > > - rte_memcpy(&filter->input.ext_data.dst_mac, > > > - ð_spec->dst, > > > - RTE_ETHER_ADDR_LEN); > > > - } > > > + if (!(eth_spec && eth_mask)) > > > + break; > > > > > > - if (!rte_is_zero_ether_addr(ð_mask->src)) { > > > - input_set |=3D ICE_INSET_SMAC; > > > - rte_memcpy(&filter->input.ext_data.src_mac, > > > - ð_spec->src, > > > - RTE_ETHER_ADDR_LEN); > > > - } > > > + *input_set |=3D is_outer ? ICE_PROT_MAC_OUTER : ICE_PROT_MAC_INNE= R; > > [Cao, Yahui] > > ICE_PROT_XXX is internally used. You should use ICE_INSET_XXX version. > > The same comment applies for similar case elsewhere. >=20 > How to distinguish internally used? ICE_PROT_XXX and ICE_INSET_XXX are > defined in same file ice_generic_flow.h. And this kind of macro tries > to introduce inner/outer bit to distinguish inner/outer, but actually > it will mix field with its location. For FDIR, it seems not a good way. >=20 [Cao, Yahui]=20 The distinguish rule is that only ICE_INSET_XXX is allowed to be used for i= nput_set value.=20 Otherwise, it may cause malfunction. > There is much more cases for inner/outer and src/dst. So I try to > use this line to set outer/inner bit first, and src/dst bit can be reused= . >=20 [Cao, Yahui]=20 It is a historical reason that input_set have both inner and outer field de= fined. To not break the original design and also compatible with current design, I suggest we can just deprecate the tunnel version field ICE_INSET_TUN_XXX = and instead use the non-tunnel version like ICE_INSET_XXX. For example, if you specify a vxlan filter rule with outter src ip and inne= r dst udp, The value is like: outer_input_set =3D ICE_INSET_IPV4_SRC inner_input_set =3D ICE_INSET_UDP_DST_PORT > > > + if (!rte_is_zero_ether_addr(ð_mask->dst)) > > > + *input_set |=3D ICE_DMAC; > > [Cao, Yahui] > > You should not use ICE_DMAC here. You should use ICE_INSET_XXX version. > > The same comment applies for similar case elsewhere. >=20 > I guess this part should re-design if we want to use. So I prefer not to = use > it. >=20 > > > + if (!rte_is_zero_ether_addr(ð_mask->src)) > > > + *input_set |=3D ICE_SMAC; > > > > > > - /* Ignore this field except for ICE_FLTR_PTYPE_NON_IP_L2 */ > > > - if (eth_mask->type =3D=3D RTE_BE16(0xffff) && > > > - next_type =3D=3D RTE_FLOW_ITEM_TYPE_END) { > > > - input_set |=3D ICE_INSET_ETHERTYPE; > > > - ether_type =3D rte_be_to_cpu_16(eth_spec->type); > > > - > > > - if (ether_type =3D=3D RTE_ETHER_TYPE_IPV4 || > > > - ether_type =3D=3D RTE_ETHER_TYPE_IPV6) { > > > - rte_flow_error_set(error, EINVAL, > > > - RTE_FLOW_ERROR_TYPE_ITEM, > > > - item, > > > - "Unsupported ether_type."); > > > - return -rte_errno; > > > - } > > > - > > > - rte_memcpy(&filter->input.ext_data.ether_type, > > .... > > > } > > > -- > > > 2.25.1 > > >=20 > -- > Best regards, > Zhirun Yan