DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Yan, Zhirun" <zhirun.yan@intel.com>
To: "Cao, Yahui" <yahui.cao@intel.com>, "dev@dpdk.org" <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: [dpdk-dev] [PATCH v1 2/5] net/ice: refactor flow pattern parser
Date: Thu, 7 Jan 2021 03:07:20 +0000
Message-ID: <18cbad7220074b50a244ee66920f516e@intel.com> (raw)
In-Reply-To: <2e7933eca3084b798cffe4a992be1246@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


  reply	other threads:[~2021-01-07  3:07 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-21  6:51 [dpdk-dev] [PATCH v1 0/5] Refactor FDIR " Zhirun Yan
2020-12-21  6:51 ` [dpdk-dev] [PATCH v1 1/5] net/ice: clean input set macro definition Zhirun Yan
2020-12-21  6:51 ` [dpdk-dev] [PATCH v1 2/5] net/ice: refactor flow pattern parser Zhirun Yan
2020-12-25  5:21   ` Cao, Yahui
2021-01-07  3:07     ` Yan, Zhirun [this message]
2020-12-21  6:51 ` [dpdk-dev] [PATCH v1 3/5] net/ice: add outer input set mask to distinguish outer fields Zhirun Yan
2020-12-25  5:27   ` Cao, Yahui
2021-01-07  3:11     ` Yan, Zhirun
2020-12-21  6:51 ` [dpdk-dev] [PATCH v1 4/5] net/ice: add outer input set mask check Zhirun Yan
2020-12-25  5:28   ` Cao, Yahui
2021-01-07  3:14     ` Yan, Zhirun
2020-12-21  6:51 ` [dpdk-dev] [PATCH v1 5/5] net/ice: enable FDIR outer/inner fields for VXLAN Zhirun Yan
2021-01-27  5:29 ` [dpdk-dev] [PATCH v2 0/3] Refactor FDIR pattern parser Zhirun Yan
2021-01-27  5:29   ` [dpdk-dev] [PATCH v2 1/3] net/ice: clean input set macro definition Zhirun Yan
2021-01-27  5:29   ` [dpdk-dev] [PATCH v2 2/3] net/ice: refactor flow pattern parser Zhirun Yan
2021-01-27  5:29   ` [dpdk-dev] [PATCH v2 3/3] net/ice: add outer input set mask to distinguish outer fields Zhirun Yan
2021-03-02  2:54   ` [dpdk-dev] [PATCH v3 0/6] Refactor FDIR pattern parser Zhirun Yan
2021-03-02  2:54     ` [dpdk-dev] [PATCH v3 1/6] net/ice: clean input set macro definition Zhirun Yan
2021-03-02  2:54     ` [dpdk-dev] [PATCH v3 2/6] net/ice: refactor structure field Zhirun Yan
2021-03-02  2:54     ` [dpdk-dev] [PATCH v3 3/6] net/ice: refactor flow pattern parser Zhirun Yan
2021-03-02  2:54     ` [dpdk-dev] [PATCH v3 4/6] net/ice: refactor input set conf Zhirun Yan
2021-03-02  2:54     ` [dpdk-dev] [PATCH v3 5/6] net/ice: add outer input set mask to distinguish outer fields Zhirun Yan
2021-03-02  2:54     ` [dpdk-dev] [PATCH v3 6/6] net/ice: clean GTPU flow_type for FDIR Zhirun Yan
2021-03-05  8:46     ` [dpdk-dev] [PATCH v3 0/6] Refactor FDIR pattern parser Zhang, Qi Z

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=18cbad7220074b50a244ee66920f516e@intel.com \
    --to=zhirun.yan@intel.com \
    --cc=dev@dpdk.org \
    --cc=junfeng.guo@intel.com \
    --cc=qi.z.zhang@intel.com \
    --cc=simei.su@intel.com \
    --cc=ting.xu@intel.com \
    --cc=xiao.w.wang@intel.com \
    --cc=yahui.cao@intel.com \
    --cc=yuying.zhang@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git