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 7F3B0A04E6; Wed, 18 Nov 2020 04:15:35 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2499F58C4; Wed, 18 Nov 2020 04:15:33 +0100 (CET) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 264544C90 for ; Wed, 18 Nov 2020 04:15:29 +0100 (CET) IronPort-SDR: LjorKEl97RmnCO+YSQ+BzjYJqdVo4ggse5XdJr8p1jXhP86lBqGR3u3y2zko++4mnUK3bgYGFk gz/2ifhCoqYw== X-IronPort-AV: E=McAfee;i="6000,8403,9808"; a="158089959" X-IronPort-AV: E=Sophos;i="5.77,486,1596524400"; d="scan'208";a="158089959" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 17 Nov 2020 19:15:27 -0800 IronPort-SDR: D0LjeKBsb+N2T3HPy6Vuy2LK9C8db/rrVySr7YAU7LVZ9X2R9tsVChRmJeSVzDty+/U6ZvfrJO dAG6qvN+i/aw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,486,1596524400"; d="scan'208";a="368314196" Received: from fmsmsx606.amr.corp.intel.com ([10.18.126.86]) by orsmga007.jf.intel.com with ESMTP; 17 Nov 2020 19:15:27 -0800 Received: from shsmsx601.ccr.corp.intel.com (10.109.6.141) by fmsmsx606.amr.corp.intel.com (10.18.126.86) 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 19:15:26 -0800 Received: from shsmsx605.ccr.corp.intel.com (10.109.6.215) by SHSMSX601.ccr.corp.intel.com (10.109.6.141) 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 11:15:24 +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 11:15:24 +0800 From: "Yan, Zhirun" To: "Cao, Yahui" CC: "dev@dpdk.org" , "Zhang, Qi Z" , "Wang, Xiao W" , "Su, Simei" Thread-Topic: [PATCH v1] net/ice: refactor flow pattern parser Thread-Index: AQHWvL7y6e0lGtQm6ESFbbHnGjUYX6nMbyPggAFHowA= Date: Wed, 18 Nov 2020 03:15:24 +0000 Message-ID: <3cb3569f17574168accda2bd6cdde6a1@intel.com> References: <20201117084524.3610038-1-zhirun.yan@intel.com> <567972babaca49939efeebe98859eb50@intel.com> In-Reply-To: <567972babaca49939efeebe98859eb50@intel.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-imapappendstamp: SHSMSX605.ccr.corp.intel.com (15.01.1713.001) x-ms-exchange-messagesentrepresentingtype: 1 x-originating-ip: [10.239.127.36] Content-Type: text/plain; charset="us-ascii" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: [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" * Cao, Yahui [2020-11-17 23:31:40 +0800]: > Hi Zhirun, >=20 > I think it is great to refactor ice fdir to differentiate inner = and outer field vector in a more clear way. Thanks for your commit. > It seems there still needs some effort to complete this patch. >=20 > > -----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 > >=20 > > Distinguish inner/outer fields. And avoid too many nested conditionals > > in each type's parser. > [Cao, Yahui]=20 > Since this is quite a huge refactor, could you give a more detailed descr= iption in the commit message? Thanks. > It would be better if you can make them a patchset. >=20 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(-) > >=20 > > diff --git a/drivers/net/ice/ice_fdir_filter.c b/drivers/net/ice/ice_fd= ir_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_ad= apter *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 ic= e_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; > >=20 > > + 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]=20 > You should take both of VXLAN and GTP cases into consideration Yes, will add GTP in v2. > > + } > > + > > + /* This loop parse flow pattern and distinguish Non-tunnel and tunnel > > + * flow. For tunnel flow, reuse non-tunnel structure to track inner > > + * part. > > + * > > + * is_outer tunnel_type p_input_set input_set_bit data_struct > > + * 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; > >=20 > > + 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]=20 > Why do you put p_v4 value assignment out of switch case RTE_FLOW_ITEM_TYP= E_IPV4? > Why do you put p_v6 value assignment out of switch case RTE_FLOW_ITEM_TYP= E_IPV6? >=20 Yes, for RTE_FLOW_ITEM_TYPE_IPV4/6, p_v4/6 assigned in each case as you say. But this is only for L4 layer parser. For L4 layer, the p_v4/6 is the share= d pointer will be used after L3 loop. > > 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; > >=20 > > - 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; > >=20 > > - 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_INNER; > [Cao, Yahui]=20 > ICE_PROT_XXX is internally used. You should use ICE_INSET_XXX version. > The same comment applies for similar case elsewhere. 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. 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. > > + if (!rte_is_zero_ether_addr(ð_mask->dst)) > > + *input_set |=3D ICE_DMAC; > [Cao, Yahui]=20 > You should not use ICE_DMAC here. You should use ICE_INSET_XXX version. > The same comment applies for similar case elsewhere. I guess this part should re-design if we want to use. So I prefer not to us= e it. > > + if (!rte_is_zero_ether_addr(ð_mask->src)) > > + *input_set |=3D ICE_SMAC; > >=20 > > - /* 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 --=20 Best regards, Zhirun Yan