Re: [dpdk-dev] [PATCH] net/ixgbe: put 5tuple check in front to jump over ntuple filter case

["Zhao1, Wei" <wei.zhao1@intel.com>]

Hi, qi

> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Thursday, October 11, 2018 2:36 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; mocan <faicker.mo@ucloud.cn>
> Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> Subject: RE: Re:RE: [dpdk-dev] [PATCH] net/ixgbe: put 5tuple check in front
> to jump over ntuple filter case
> 
> 
> 
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Monday, October 8, 2018 2:46 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>; mocan <faicker.mo@ucloud.cn>
> > Cc: dev@dpdk.org; Lu, Wenzhuo <wenzhuo.lu@intel.com>
> > Subject: RE: Re:RE: [dpdk-dev] [PATCH] net/ixgbe: put 5tuple check in
> > front to jump over ntuple filter case
> >
> > Hi,
> >
> >
> > > -----Original Message-----
> > > From: Zhang, Qi Z
> > > Sent: Wednesday, September 26, 2018 7:14 PM
> > > To: mocan <faicker.mo@ucloud.cn>; Zhao1, Wei <wei.zhao1@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: RE: Re:RE: [dpdk-dev] [PATCH] net/ixgbe: put 5tuple check
> > > in front to jump over ntuple filter case
> > >
> > > OK, got your point. We should not reject a possible valid fdir flow
> > > at n-tuple flow check stage.
> > >
> > > Review-by: Qi Zhang <qi.z.zhang@intel.com>
> >
> >
> > I agree with the point of " We should not reject a possible valid fdir
> > flow at n-tuple flow check stage".
> > But, I think the fix patch should be more generic for all types filter
> > of this problem.
> > Maybe, we should delete all " goto out"  in function ixgbe_flow_create().
> > Then, it will go to ntuple filter and  ethertype filter, syn filter
> > and fdir filter ,l2_tn_filter one by one.
> > And aslo, we should code as
> >
> > {
> >
> > Ntuple:
> > ..........
> > if(ret)
> >     Goto ethertype
> > ..........
> >
> > Ethertype:
> >
> > ..........
> > if(ret)
> >     Goto fdir filter
> > .........
> >
> > fdir filter:
> >
> > if(ret)
> >   Goto l2_tn_filter
> >
> > l2_tn_filter:
> >
> > .............
> >
> > }
> >
> > This fix patch only solve the problem of  ntuple and fdir.
> > Qi, What do you think of this?
> 
> I'm not the author of this part of code, so my understanding of current
> implementation is:
> It assume a flow will not be ambiguous which means if it match to some filter
> parser (ixgbe_parse_xxx_filter), it is not necessary to match on a different
> filter.
> But I'm not sure if the assumption is correct or not, (this depends on the
> knowledge of the device capability), So do you mean the assumption is not
> correct? If you think a generic fix is necessary, I have below comments

Yes, the assumption is may cause bug, this patch is an evidence, maybe this user has encountered this problem.

> 
> 1. it is better be done by Intel people with enough validation 

I agree with you, I will commit a generic fix patch later.

>2. two options  for patch submit.
> 	Submit a v2 with the generic fix, and it will be captured in this release.
> 	If it is not urgent, we can just accept current one first, then have a
> separate patch in next release.

Ok, If someone supply a v2 with the generic fix, I will ack.

> 
> Thanks
> Qi
> 
> 
> >
> > >
> > > Thanks
> > > Qi
> > >
> > > From: mocan [mailto:faicker.mo@ucloud.cn]
> > > Sent: Wednesday, September 26, 2018 4:16 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org
> > > Subject: Re:RE: [dpdk-dev] [PATCH] net/ixgbe: put 5tuple check in
> > > front to jump over ntuple filter case
> > >
> > > Hi Qi,
> > > In ixgbe_flow_create function, ntuple filter is parsed first. If the
> > > flow is considered to be ntuple filter, it will not go on to judge
> > > ethertype filter, syn filter and fdir filter.
> > > In the function ntuple_filter_to_5tuple, 5 tuple info is checked,
> > > but it's too late to jump over the ntuple filter if it's a fdir filter.
> > >
> > >
> > >
> > >
> > >
> > >
> > > At 2018-09-21 23:48:37, "Zhang, Qi Z" <qi.z.zhang@intel.com> wrote:
> > > >Hi Faicker:
> > > >
> > > >> -----Original Message-----
> > > >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of faicker.mo
> > > >> Sent: Tuesday, September 18, 2018 1:49 PM
> > > >> To: dev@dpdk.org
> > > >> Cc: faicker.mo <faicker.mo@ucloud.cn>
> > > >> Subject: [dpdk-dev] [PATCH] net/ixgbe: put 5tuple check in front
> > > >> to jump
> > > over
> > > >> ntuple filter case
> > > >>
> > > >> From: "faicker.mo" <faicker.mo@ucloud.cn>
> > > >>
> > > >> Check in func ntuple_filter_to_5tuple is too late for fdir filter
> > > >> rule, add
> > > check
> > > >> in func cons_parse_ntuple_filter.
> > > >
> > > >Would you explain more about the intention for this patch?
> > > >Though it can be more fast to reject an invalid flow, but why it is
> > > >too late in
> > > your case?
> > > >
> > > >Thanks
> > > >Qi
> > > >
> > > >
> > > >>
> > > >> Signed-off-by: faicker.mo <faicker.mo@ucloud.cn>
> > > >> ---
> > > >>  drivers/net/ixgbe/ixgbe_flow.c | 29
> > > +++++++++++++++++++++++++++++
> > > >>  1 file changed, 29 insertions(+)
> > > >>
> > > >> diff --git a/drivers/net/ixgbe/ixgbe_flow.c
> > > b/drivers/net/ixgbe/ixgbe_flow.c
> > > >> index 1adf1b8..f0fafeb 100644
> > > >> --- a/drivers/net/ixgbe/ixgbe_flow.c
> > > >> +++ b/drivers/net/ixgbe/ixgbe_flow.c
> > > >> @@ -363,6 +363,17 @@ const struct rte_flow_action
> > > *next_no_void_action(
> > > >>  				item, "Not supported by ntuple filter");
> > > >>  			return -rte_errno;
> > > >>  		}
> > > >> +		if ((ipv4_mask->hdr.src_addr != 0 &&
> > > >> +			ipv4_mask->hdr.src_addr != UINT32_MAX)
> ||
> > > >> +			(ipv4_mask->hdr.dst_addr != 0 &&
> > > >> +			ipv4_mask->hdr.dst_addr != UINT32_MAX)
> ||
> > > >> +			(ipv4_mask->hdr.next_proto_id !=
> UINT8_MAX &&
> > > >> +			ipv4_mask->hdr.next_proto_id != 0)) {
> > > >> +			rte_flow_error_set(error,
> > > >> +				EINVAL,
> RTE_FLOW_ERROR_TYPE_ITEM,
> > > >> +				item, "Not supported by ntuple
> filter");
> > > >> +			return -rte_errno;
> > > >> +		}
> > > >>
> > > >>  		filter->dst_ip_mask = ipv4_mask->hdr.dst_addr;
> > > >>  		filter->src_ip_mask = ipv4_mask->hdr.src_addr; @@ -432,6
> > > +443,15
> > > >> @@ const struct rte_flow_action *next_no_void_action(
> > > >>  				item, "Not supported by ntuple filter");
> > > >>  			return -rte_errno;
> > > >>  		}
> > > >> +		if ((tcp_mask->hdr.src_port != 0 &&
> > > >> +			tcp_mask->hdr.src_port != UINT16_MAX) ||
> > > >> +			(tcp_mask->hdr.dst_port != 0 &&
> > > >> +			tcp_mask->hdr.dst_port != UINT16_MAX)) {
> > > >> +			rte_flow_error_set(error,
> > > >> +				EINVAL,
> RTE_FLOW_ERROR_TYPE_ITEM,
> > > >> +				item, "Not supported by ntuple
> filter");
> > > >> +			return -rte_errno;
> > > >> +		}
> > > >>
> > > >>  		filter->dst_port_mask  = tcp_mask->hdr.dst_port;
> > > >>  		filter->src_port_mask  = tcp_mask->hdr.src_port; @@ -467,6
> > > >> +487,15 @@ const struct rte_flow_action *next_no_void_action(
> > > >>  				item, "Not supported by ntuple filter");
> > > >>  			return -rte_errno;
> > > >>  		}
> > > >> +		if ((udp_mask->hdr.src_port != 0 &&
> > > >> +			udp_mask->hdr.src_port != UINT16_MAX) ||
> > > >> +			(udp_mask->hdr.dst_port != 0 &&
> > > >> +			udp_mask->hdr.dst_port != UINT16_MAX)) {
> > > >> +			rte_flow_error_set(error,
> > > >> +				EINVAL,
> RTE_FLOW_ERROR_TYPE_ITEM,
> > > >> +				item, "Not supported by ntuple
> filter");
> > > >> +			return -rte_errno;
> > > >> +		}
> > > >>
> > > >>  		filter->dst_port_mask = udp_mask->hdr.dst_port;
> > > >>  		filter->src_port_mask = udp_mask->hdr.src_port;
> > > >> --
> > > >> 1.8.3.1
> > > >>
> > > >