Re: [dpdk-dev] [PATCH v1 2/5] net/ice: refactor flow pattern parser

["Yan, Zhirun" <zhirun.yan@intel.com>]

> -----Original Message-----
> From: Cao, Yahui
> Sent: Friday, December 25, 2020 1:22 PM
> To: Yan, Zhirun <zhirun.yan@intel.com>; dev@dpdk.org; Zhang, Qi Z
> <qi.z.zhang@intel.com>; Wang, Xiao W <xiao.w.wang@intel.com>; Guo,
> Junfeng <junfeng.guo@intel.com>
> Cc: Su, Simei <simei.su@intel.com>; Xu, Ting <ting.xu@intel.com>; Zhang,
> Yuying <yuying.zhang@intel.com>
> Subject: RE: [PATCH v1 2/5] net/ice: refactor flow pattern parser
> 
> 
> 
> > -----Original Message-----
> > From: Yan, Zhirun <zhirun.yan@intel.com>
> > Sent: Monday, December 21, 2020 2:52 PM
> > To: dev@dpdk.org; Zhang, Qi Z <qi.z.zhang@intel.com>; Cao, Yahui
> > <yahui.cao@intel.com>; Wang, Xiao W <xiao.w.wang@intel.com>; Guo,
> > Junfeng <junfeng.guo@intel.com>
> > Cc: Su, Simei <simei.su@intel.com>; Xu, Ting <ting.xu@intel.com>;
> > Zhang, Yuying <yuying.zhang@intel.com>; Yan, Zhirun
> > <zhirun.yan@intel.com>
> > Subject: [PATCH v1 2/5] net/ice: refactor flow pattern parser
> >
> > Distinguish inner/outer input set. And avoid too many nested
> > conditionals in each type's parser. input_set_f is used for outer
> > fields, input_set_l is used for inner or non-tunnel fields.
> >
> > Signed-off-by: Zhirun Yan <zhirun.yan@intel.com>
> > ---
> >  drivers/net/ice/ice_fdir_filter.c | 467
> > +++++++++++++++---------------
> >  1 file changed, 229 insertions(+), 238 deletions(-)
> >
> > diff --git a/drivers/net/ice/ice_fdir_filter.c
> > b/drivers/net/ice/ice_fdir_filter.c
> > index 92b8a7e6ad..1f2576a444 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 = ICE_INSET_NONE;
> > +	uint64_t input_set_l = ICE_INSET_NONE;
> > +	uint64_t input_set_f = ICE_INSET_NONE;
> > +	uint64_t *input_set;
> >  	uint8_t flow_type = ICE_FLTR_PTYPE_NONF_NONE;
> >  	uint8_t  ipv6_addr_mask[16] = {
> >  		0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, @@ -1655,289
> > +1657,276 @@ 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 = true;
> > +	struct ice_fdir_extra *p_ext_data;
> > +	struct ice_fdir_v4 *p_v4;
> > +	struct ice_fdir_v6 *p_v6;
> >
> > +	for (item = pattern; item->type != RTE_FLOW_ITEM_TYPE_END;
> item++) {
> > +		if (item->type == RTE_FLOW_ITEM_TYPE_VXLAN)
> > +			tunnel_type = ICE_FDIR_TUNNEL_TYPE_VXLAN;
> > +		if (item->type == RTE_FLOW_ITEM_TYPE_GTPU)
> > +			tunnel_type = ICE_FDIR_TUNNEL_TYPE_GTPU;
> > +		if (item->type == RTE_FLOW_ITEM_TYPE_GTP_PSC)
> > +			tunnel_type = ICE_FDIR_TUNNEL_TYPE_GTPU_EH;
> > +	}
> > +
> > +	/* This loop parse flow pattern and distinguish Non-tunnel and
> tunnel
> > +	 * flow. input_set_l is used for non-tunnel and tunnel inner part.
> > +	 */
> 
> ...
> >
> > +		input_set = (tunnel_type && is_outer) ?
> > +			    &input_set_f : &input_set_l;
> [Cao, Yahui] input_set_l used for inner filed or non-tunnel filed looks
> confusing, can we just use input_set_l as non-tunnel filed or tunnel outer
> field ?

Yes, both are OK.
In my view, Non-tunnel and inner part have no tunnel layer info, they can be the same definition.
But for tunnel type, the outer part is different. It may contain tunnel layer info like VNI in VXLAN. 
Only this part is specific with tunnel type.

> > +
> > +		if (l3 == RTE_FLOW_ITEM_TYPE_IPV4)
> > +			p_v4 = (tunnel_type && is_outer) ?
> > +			       &filter->input.ip_outer.v4 :
> > +			       &filter->input.ip.v4;
> > +		if (l3 == RTE_FLOW_ITEM_TYPE_IPV6)
> > +			p_v6 = (tunnel_type && is_outer) ?
> > +			       &filter->input.ip_outer.v6 :
> > +			       &filter->input.ip.v6;
> > +
> [Cao, Yahui] p_v4 and p_v6 pointer assignment looks redundant since it will
> be assigned in IPV4 and IPV6 switch case statement.

If L3 has no fields, then we could not know whether it is ipv4 or ipv6 for L4 case.
You are right. The p_v4/6 assignment can be set earlier in L3 layer parse.

> >  		switch (item_type) {
> 
> ...
> > +
> > +			p_v4 = (tunnel_type && is_outer) ?
> > +			       &filter->input.ip_outer.v4 :
> > +			       &filter->input.ip.v4;
> [Cao, Yahui]  it is assigned here again

Yes, I will move this earlier when set flow item type.


> > +			p_v4->dst_ip = ipv4_spec->hdr.dst_addr;
> > +			p_v4->src_ip = ipv4_spec->hdr.src_addr;
> > +			p_v4->tos = ipv4_spec->hdr.type_of_service;
> >  			break;
> 
> ......
> > +
> > +			p_v6 = (tunnel_type && is_outer) ?
> > +			       &filter->input.ip_outer.v6 :
> > +			       &filter->input.ip.v6;
> [Cao, Yahui]  it is assigned here again

Yes, I will fix it.

> > +			rte_memcpy(&p_v6->dst_ip, ipv6_spec-
> >hdr.dst_addr, 16);
> > +			rte_memcpy(&p_v6->src_ip, ipv6_spec-
> >hdr.src_addr, 16);
> > +			vtc_flow_cpu = rte_be_to_cpu_32(ipv6_spec-
> >hdr.vtc_flow);
> > +			p_v6->tc = (uint8_t)(vtc_flow_cpu >>
> ICE_FDIR_IPV6_TC_OFFSET);
> > +			p_v6->proto = ipv6_spec->hdr.proto;
> > +			p_v6->hlim = ipv6_spec->hdr.hop_limits;
> >  			break;
> ......
> >  	filter->tunnel_type = tunnel_type;
> >  	filter->input.flow_type = flow_type;
> > -	filter->input_set = input_set;
> > +	filter->input_set = input_set_l;
> > +	filter->outer_input_set = input_set_f;
> [Cao, Yahui] Correspondingly here, input_set and outer_input_set naming is
> also confusing.

input_set is used for tunnel inner part and non-tunnel type.
outer_input_set is only for tunnel outer part.
Thanks.

> >
> >  	return 0;
> >  }
> > --
> > 2.25.1