net/ixgbe: put 5tuple check in front to jump over ntuple filter case
Checks
Commit Message
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.
Signed-off-by: faicker.mo <faicker.mo@ucloud.cn>
---
drivers/net/ixgbe/ixgbe_flow.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
Comments
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
>
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
>>
>
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>
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
>>
>
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
> >>
> >
> -----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
1. it is better be done by Intel people with enough validation
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.
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
> > >>
> > >
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
> > > >>
> > > >
Hi Wei:
> -----Original Message-----
> From: Zhao1, Wei
> Sent: Thursday, October 11, 2018 1:10 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, 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.
>
Just want to confirm with you , are you agree to merge this patch?
Or you think v2 with generic fix is necessary?
From my view, the patch can be accepted, since it just add more strict check in cons_parse_ntuple_filter, it does not break anything, and it fix the specific issue.
Thanks
Qi
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, October 15, 2018 11:31 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
>
> Hi Wei:
>
> > -----Original Message-----
> > From: Zhao1, Wei
> > Sent: Thursday, October 11, 2018 1:10 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, 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.
> >
>
> Just want to confirm with you , are you agree to merge this patch?
> Or you think v2 with generic fix is necessary?
> From my view, the patch can be accepted, since it just add more strict check
> in cons_parse_ntuple_filter, it does not break anything, and it fix the specific
> issue.
>
> Thanks
> Qi
>
>
Of course, it can be merge, but a more generic still need.
Acked-by: Wei Zhao <wei.zhao1@intel.com>
> -----Original Message-----
> From: Zhao1, Wei
> Sent: Tuesday, October 16, 2018 10:57 PM
> 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
>
>
>
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Monday, October 15, 2018 11:31 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
> >
> > Hi Wei:
> >
> > > -----Original Message-----
> > > From: Zhao1, Wei
> > > Sent: Thursday, October 11, 2018 1:10 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, 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.
> > >
> >
> > Just want to confirm with you , are you agree to merge this patch?
> > Or you think v2 with generic fix is necessary?
> > From my view, the patch can be accepted, since it just add more strict
> > check in cons_parse_ntuple_filter, it does not break anything, and it
> > fix the specific issue.
> >
> > Thanks
> > Qi
> >
> >
>
> Of course, it can be merge, but a more generic still need.
> Acked-by: Wei Zhao <wei.zhao1@intel.com>
Applied to dpdk-next-net-intel with below changes
1) change the title to "fix to reject a valid flow during ntuple check"
2) more detail commit log base on auther's comment in mail list
3) add fix line and Cc stable@dpdk.org
Thanks
Qi
@@ -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;