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

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

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?

> 
> 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
> >>
> >