diff mbox series

[v2] net/i40e: fix flow FDIR enable issue

Message ID 20200521080306.17404-1-wei.zhao1@intel.com (mailing list archive)
State Accepted, archived
Delegated to: xiaolong ye
Headers
Series [v2] net/i40e: fix flow FDIR enable issue |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-nxp-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-testing fail Testing issues
ci/Intel-compilation success Compilation OK

Commit Message

Zhao1, Wei May 21, 2020, 8:03 a.m. UTC 
  When we flush flow FDIR, all queues are disabled for FDIR.
If FDIR rule is created again, then the flow list is empty,
as it is the first time to create rule after flush fdir filter,
so we need to enable FDIR for all queues. And also, disable FDIR
for queues should be done in function i40e_flow_flush_fdir_filter().

Fixes: 1491f63c7559 ("net/i40e: fix flush of flow director filter")
Fixes: 6ae9b2b5e8c2 ("net/i40e: cache flow director enable value in Rx queue")
Cc: stable@dpdk.org

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>

---

v2:
update log info
---
 drivers/net/i40e/i40e_flow.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Guo, Jia May 21, 2020, 2:27 p.m. UTC | #1 
hi, zhaowei

On 5/21/2020 4:03 PM, Wei Zhao wrote:
> When we flush flow FDIR, all queues are disabled for FDIR.
> If FDIR rule is created again, then the flow list is empty,
> as it is the first time to create rule after flush fdir filter,
> so we need to enable FDIR for all queues. And also, disable FDIR
> for queues should be done in function i40e_flow_flush_fdir_filter().
>
> Fixes: 1491f63c7559 ("net/i40e: fix flush of flow director filter")
> Fixes: 6ae9b2b5e8c2 ("net/i40e: cache flow director enable value in Rx queue")
> Cc: stable@dpdk.org
>
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>
> ---
>
> v2:
> update log info
> ---
>   drivers/net/i40e/i40e_flow.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
> index 25c77e7aa..8f8df6fae 100644
> --- a/drivers/net/i40e/i40e_flow.c
> +++ b/drivers/net/i40e/i40e_flow.c
> @@ -3462,6 +3462,10 @@ i40e_flow_parse_fdir_filter(struct rte_eth_dev *dev,
>   		}
>   	}
>   
> +	/* If create the first fdir rule, enable fdir check for rx queues */
> +	if (TAILQ_EMPTY(&pf->fdir.fdir_list))
> +		i40e_fdir_rx_proc_enable(dev, 1);
> +


Do you think it is make sense that move the configure out of the priors 
condition? If so the list empty check is no need here.

     if (pf->fdir.fdir_vsi == NULL) {
.....
     }

     ret = i40e_fdir_configure(dev);


>   	return 0;
>   err:
>   	i40e_fdir_teardown(pf);
> @@ -5330,9 +5334,6 @@ i40e_flow_flush(struct rte_eth_dev *dev, struct rte_flow_error *error)
>   		return -rte_errno;
>   	}
>   
> -	/* Disable FDIR processing as all FDIR rules are now flushed */
> -	i40e_fdir_rx_proc_enable(dev, 0);
> -
>   	return ret;
>   }
>   
> @@ -5368,6 +5369,9 @@ i40e_flow_flush_fdir_filter(struct i40e_pf *pf)
>   		for (pctype = I40E_FILTER_PCTYPE_NONF_IPV4_UDP;
>   		     pctype <= I40E_FILTER_PCTYPE_L2_PAYLOAD; pctype++)
>   			pf->fdir.inset_flag[pctype] = 0;
> +
> +		/* Disable FDIR processing as all FDIR rules are now flushed */
> +		i40e_fdir_rx_proc_enable(dev, 0);
>   	}
>   
>   	return ret;
Zhao1, Wei May 22, 2020, 12:59 a.m. UTC | #2 
Hi, jia

> -----Original Message-----
> From: Guo, Jia <jia.guo@intel.com>
> Sent: Thursday, May 21, 2020 10:27 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Ye, Xiaolong
> <xiaolong.ye@intel.com>
> Subject: Re: [PATCH v2] net/i40e: fix flow FDIR enable issue
> 
> hi, zhaowei
> 
> On 5/21/2020 4:03 PM, Wei Zhao wrote:
> > When we flush flow FDIR, all queues are disabled for FDIR.
> > If FDIR rule is created again, then the flow list is empty, as it is
> > the first time to create rule after flush fdir filter, so we need to
> > enable FDIR for all queues. And also, disable FDIR for queues should
> > be done in function i40e_flow_flush_fdir_filter().
> >
> > Fixes: 1491f63c7559 ("net/i40e: fix flush of flow director filter")
> > Fixes: 6ae9b2b5e8c2 ("net/i40e: cache flow director enable value in Rx
> > queue")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >
> > ---
> >
> > v2:
> > update log info
> > ---
> >   drivers/net/i40e/i40e_flow.c | 10 +++++++---
> >   1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/i40e/i40e_flow.c
> > b/drivers/net/i40e/i40e_flow.c index 25c77e7aa..8f8df6fae 100644
> > --- a/drivers/net/i40e/i40e_flow.c
> > +++ b/drivers/net/i40e/i40e_flow.c
> > @@ -3462,6 +3462,10 @@ i40e_flow_parse_fdir_filter(struct rte_eth_dev
> *dev,
> >   		}
> >   	}
> >
> > +	/* If create the first fdir rule, enable fdir check for rx queues */
> > +	if (TAILQ_EMPTY(&pf->fdir.fdir_list))
> > +		i40e_fdir_rx_proc_enable(dev, 1);
> > +
> 
> 
> Do you think it is make sense that move the configure out of the priors
> condition? If so the list empty check is no need here.
> 
>      if (pf->fdir.fdir_vsi == NULL) {
> .....
>      }
> 
>      ret = i40e_fdir_configure(dev);
> 

That seems more reasonable, but I do not know what will happen for this , because
That will cause i40e_fdir_configure() be called every time when download rule. 
I dare not make this change for fear of introducing more problems.

> 
> >   	return 0;
> >   err:
> >   	i40e_fdir_teardown(pf);
> > @@ -5330,9 +5334,6 @@ i40e_flow_flush(struct rte_eth_dev *dev, struct
> rte_flow_error *error)
> >   		return -rte_errno;
> >   	}
> >
> > -	/* Disable FDIR processing as all FDIR rules are now flushed */
> > -	i40e_fdir_rx_proc_enable(dev, 0);
> > -
> >   	return ret;
> >   }
> >
> > @@ -5368,6 +5369,9 @@ i40e_flow_flush_fdir_filter(struct i40e_pf *pf)
> >   		for (pctype = I40E_FILTER_PCTYPE_NONF_IPV4_UDP;
> >   		     pctype <= I40E_FILTER_PCTYPE_L2_PAYLOAD; pctype++)
> >   			pf->fdir.inset_flag[pctype] = 0;
> > +
> > +		/* Disable FDIR processing as all FDIR rules are now flushed */
> > +		i40e_fdir_rx_proc_enable(dev, 0);
> >   	}
> >
> >   	return ret;
Guo, Jia May 22, 2020, 2:16 a.m. UTC | #3 
hi, zhaowei

On 5/22/2020 8:59 AM, Zhao1, Wei wrote:
> Hi, jia
>
>> -----Original Message-----
>> From: Guo, Jia <jia.guo@intel.com>
>> Sent: Thursday, May 21, 2020 10:27 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Ye, Xiaolong
>> <xiaolong.ye@intel.com>
>> Subject: Re: [PATCH v2] net/i40e: fix flow FDIR enable issue
>>
>> hi, zhaowei
>>
>> On 5/21/2020 4:03 PM, Wei Zhao wrote:
>>> When we flush flow FDIR, all queues are disabled for FDIR.
>>> If FDIR rule is created again, then the flow list is empty, as it is
>>> the first time to create rule after flush fdir filter, so we need to
>>> enable FDIR for all queues. And also, disable FDIR for queues should
>>> be done in function i40e_flow_flush_fdir_filter().
>>>
>>> Fixes: 1491f63c7559 ("net/i40e: fix flush of flow director filter")
>>> Fixes: 6ae9b2b5e8c2 ("net/i40e: cache flow director enable value in Rx
>>> queue")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>>
>>> ---
>>>
>>> v2:
>>> update log info
>>> ---
>>>    drivers/net/i40e/i40e_flow.c | 10 +++++++---
>>>    1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/i40e/i40e_flow.c
>>> b/drivers/net/i40e/i40e_flow.c index 25c77e7aa..8f8df6fae 100644
>>> --- a/drivers/net/i40e/i40e_flow.c
>>> +++ b/drivers/net/i40e/i40e_flow.c
>>> @@ -3462,6 +3462,10 @@ i40e_flow_parse_fdir_filter(struct rte_eth_dev
>> *dev,
>>>    }
>>>    }
>>>
>>> +/* If create the first fdir rule, enable fdir check for rx queues */
>>> +if (TAILQ_EMPTY(&pf->fdir.fdir_list))
>>> +i40e_fdir_rx_proc_enable(dev, 1);
>>> +
>>
>> Do you think it is make sense that move the configure out of the priors
>> condition? If so the list empty check is no need here.
>>
>>       if (pf->fdir.fdir_vsi == NULL) {
>> .....
>>       }
>>
>>       ret = i40e_fdir_configure(dev);
>>
> That seems more reasonable, but I do not know what will happen for this , because
> That will cause i40e_fdir_configure() be called every time when download rule.
> I dare not make this change for fear of introducing more problems.


make sense.


>>>    return 0;
>>>    err:
>>>    i40e_fdir_teardown(pf);
>>> @@ -5330,9 +5334,6 @@ i40e_flow_flush(struct rte_eth_dev *dev, struct
>> rte_flow_error *error)
>>>    return -rte_errno;
>>>    }
>>>
>>> -/* Disable FDIR processing as all FDIR rules are now flushed */
>>> -i40e_fdir_rx_proc_enable(dev, 0);
>>> -
>>>    return ret;
>>>    }
>>>
>>> @@ -5368,6 +5369,9 @@ i40e_flow_flush_fdir_filter(struct i40e_pf *pf)
>>>    for (pctype = I40E_FILTER_PCTYPE_NONF_IPV4_UDP;
>>>         pctype <= I40E_FILTER_PCTYPE_L2_PAYLOAD; pctype++)
>>>    pf->fdir.inset_flag[pctype] = 0;
>>> +
>>> +/* Disable FDIR processing as all FDIR rules are now flushed */
>>> +i40e_fdir_rx_proc_enable(dev, 0);
>>>    }
>>>
>>>    return ret;


Reviewed-by: Jeff Guo <jia.guo@intel.com>
Xiaolong Ye May 22, 2020, 9:18 a.m. UTC | #4 
On 05/21, Wei Zhao wrote:
>When we flush flow FDIR, all queues are disabled for FDIR.
>If FDIR rule is created again, then the flow list is empty,
>as it is the first time to create rule after flush fdir filter,
>so we need to enable FDIR for all queues. And also, disable FDIR
>for queues should be done in function i40e_flow_flush_fdir_filter().
>
>Fixes: 1491f63c7559 ("net/i40e: fix flush of flow director filter")
>Fixes: 6ae9b2b5e8c2 ("net/i40e: cache flow director enable value in Rx queue")
>Cc: stable@dpdk.org
>
>Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>
>---
>
>v2:
>update log info
>---
> drivers/net/i40e/i40e_flow.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
>index 25c77e7aa..8f8df6fae 100644
>--- a/drivers/net/i40e/i40e_flow.c
>+++ b/drivers/net/i40e/i40e_flow.c
>@@ -3462,6 +3462,10 @@ i40e_flow_parse_fdir_filter(struct rte_eth_dev *dev,
> 		}
> 	}
> 
>+	/* If create the first fdir rule, enable fdir check for rx queues */
>+	if (TAILQ_EMPTY(&pf->fdir.fdir_list))
>+		i40e_fdir_rx_proc_enable(dev, 1);
>+
> 	return 0;
> err:
> 	i40e_fdir_teardown(pf);
>@@ -5330,9 +5334,6 @@ i40e_flow_flush(struct rte_eth_dev *dev, struct rte_flow_error *error)
> 		return -rte_errno;
> 	}
> 
>-	/* Disable FDIR processing as all FDIR rules are now flushed */
>-	i40e_fdir_rx_proc_enable(dev, 0);
>-
> 	return ret;
> }
> 
>@@ -5368,6 +5369,9 @@ i40e_flow_flush_fdir_filter(struct i40e_pf *pf)
> 		for (pctype = I40E_FILTER_PCTYPE_NONF_IPV4_UDP;
> 		     pctype <= I40E_FILTER_PCTYPE_L2_PAYLOAD; pctype++)
> 			pf->fdir.inset_flag[pctype] = 0;
>+
>+		/* Disable FDIR processing as all FDIR rules are now flushed */
>+		i40e_fdir_rx_proc_enable(dev, 0);
> 	}
> 
> 	return ret;
>-- 
>2.19.1
>

Applied to dpdk-next-net-intel, Thanks.
diff mbox series

Patch

diff --git a/drivers/net/i40e/i40e_flow.c b/drivers/net/i40e/i40e_flow.c
index 25c77e7aa..8f8df6fae 100644
--- a/drivers/net/i40e/i40e_flow.c
+++ b/drivers/net/i40e/i40e_flow.c
@@ -3462,6 +3462,10 @@  i40e_flow_parse_fdir_filter(struct rte_eth_dev *dev,
 		}
 	}
 
+	/* If create the first fdir rule, enable fdir check for rx queues */
+	if (TAILQ_EMPTY(&pf->fdir.fdir_list))
+		i40e_fdir_rx_proc_enable(dev, 1);
+
 	return 0;
 err:
 	i40e_fdir_teardown(pf);
@@ -5330,9 +5334,6 @@  i40e_flow_flush(struct rte_eth_dev *dev, struct rte_flow_error *error)
 		return -rte_errno;
 	}
 
-	/* Disable FDIR processing as all FDIR rules are now flushed */
-	i40e_fdir_rx_proc_enable(dev, 0);
-
 	return ret;
 }
 
@@ -5368,6 +5369,9 @@  i40e_flow_flush_fdir_filter(struct i40e_pf *pf)
 		for (pctype = I40E_FILTER_PCTYPE_NONF_IPV4_UDP;
 		     pctype <= I40E_FILTER_PCTYPE_L2_PAYLOAD; pctype++)
 			pf->fdir.inset_flag[pctype] = 0;
+
+		/* Disable FDIR processing as all FDIR rules are now flushed */
+		i40e_fdir_rx_proc_enable(dev, 0);
 	}
 
 	return ret;