[dpdk-dev,v7] net/i40e: improved FDIR programming times
Checks
Commit Message
Previously, the FDIR programming time is +11ms on i40e.
This patch will result in an average programming time of
22usec with a max of 60usec .
Signed-off-by: Michael Lilja <ml@napatech.com>
---
v7:
* Code style changes
v6:
* Fixed code style issues
v5:
* Reinitialization of "i" inconsistent with original intent
v4:
* Code style fix
v3:
* Replaced commit message
v2:
* Code style fix
v1:
* Initial version
---
---
drivers/net/i40e/i40e_fdir.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
Comments
On 5/17/2017 3:31 PM, Michael Lilja wrote:
> Previously, the FDIR programming time is +11ms on i40e.
> This patch will result in an average programming time of
> 22usec with a max of 60usec .
>
> Signed-off-by: Michael Lilja <ml@napatech.com>
Sorry for multiple, minor change requests ...
>
> ---
> v7:
> * Code style changes
>
> v6:
> * Fixed code style issues
>
> v5:
> * Reinitialization of "i" inconsistent with original intent
>
> v4:
> * Code style fix
>
> v3:
> * Replaced commit message
>
> v2:
> * Code style fix
>
> v1:
> * Initial version
> ---
> ---
> drivers/net/i40e/i40e_fdir.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_fdir.c b/drivers/net/i40e/i40e_fdir.c
> index 28cc554f5..1192d5831 100644
> --- a/drivers/net/i40e/i40e_fdir.c
> +++ b/drivers/net/i40e/i40e_fdir.c
> @@ -76,6 +76,7 @@
> /* Wait count and interval for fdir filter programming */
> #define I40E_FDIR_WAIT_COUNT 10
> #define I40E_FDIR_WAIT_INTERVAL_US 1000
> +#define I40E_FDIR_MAX_WAIT (I40E_FDIR_WAIT_COUNT * I40E_FDIR_WAIT_INTERVAL_US)
It looks like I40E_FDIR_WAIT_COUNT and I40E_FDIR_WAIT_INTERVAL_US not
used anywhere else, is there any value to keep them?
why not:
#define I40E_FDIR_MAX_WAIT_US 10000 /* 10 ms */
>
> /* Wait count and interval for fdir filter flush */
> #define I40E_FDIR_FLUSH_RETRY 50
> @@ -1295,28 +1296,27 @@ i40e_fdir_filter_programming(struct i40e_pf *pf,
> /* Update the tx tail register */
> rte_wmb();
> I40E_PCI_REG_WRITE(txq->qtx_tail, txq->tx_tail);
> -
> - for (i = 0; i < I40E_FDIR_WAIT_COUNT; i++) {
> - rte_delay_us(I40E_FDIR_WAIT_INTERVAL_US);
> + for (i = 0; i < I40E_FDIR_MAX_WAIT; i++) {
> if ((txdp->cmd_type_offset_bsz &
> rte_cpu_to_le_64(I40E_TXD_QW1_DTYPE_MASK)) ==
> rte_cpu_to_le_64(I40E_TX_DESC_DTYPE_DESC_DONE))
> break;
> + rte_delay_us(1);
> }
> - if (i >= I40E_FDIR_WAIT_COUNT) {
> + if (i >= I40E_FDIR_MAX_WAIT) {
> PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
> " time out to get DD on tx queue.");
> return -ETIMEDOUT;
> }
> /* totally delay 10 ms to check programming status*/
> - rte_delay_us((I40E_FDIR_WAIT_COUNT - i) * I40E_FDIR_WAIT_INTERVAL_US);
> - if (i40e_check_fdir_programming_status(rxq) < 0) {
> - PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
> - " programming status reported.");
> - return -ENOSYS;
> + for (; i < I40E_FDIR_MAX_WAIT; i++) {
> + if (i40e_check_fdir_programming_status(rxq) >= 0)
> + return 0;
> + rte_delay_us(1);
> }
> -
> - return 0;
> + PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
> + " programming status reported.");
> + return -ETIMEDOUT;
> }
>
> /*
>
It's ok. I didn't write the original code so I cannot tell why the two defines were made in the initial case. It make sense to remove them, but the maintainers must have had a reason, maybe they are needed in a future version of the code?
/Michael
-----Original Message-----
From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
Sent: 17 May 2017 16:44
To: Michael Lilja <ml@napatech.com>; helin.zhang@intel.com; jingjing.wu@intel.com
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v7] net/i40e: improved FDIR programming times
On 5/17/2017 3:31 PM, Michael Lilja wrote:
> Previously, the FDIR programming time is +11ms on i40e.
> This patch will result in an average programming time of 22usec with a
> max of 60usec .
>
> Signed-off-by: Michael Lilja <ml@napatech.com>
Sorry for multiple, minor change requests ...
>
> ---
> v7:
> * Code style changes
>
> v6:
> * Fixed code style issues
>
> v5:
> * Reinitialization of "i" inconsistent with original intent
>
> v4:
> * Code style fix
>
> v3:
> * Replaced commit message
>
> v2:
> * Code style fix
>
> v1:
> * Initial version
> ---
> ---
> drivers/net/i40e/i40e_fdir.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/i40e/i40e_fdir.c
> b/drivers/net/i40e/i40e_fdir.c index 28cc554f5..1192d5831 100644
> --- a/drivers/net/i40e/i40e_fdir.c
> +++ b/drivers/net/i40e/i40e_fdir.c
> @@ -76,6 +76,7 @@
> /* Wait count and interval for fdir filter programming */
> #define I40E_FDIR_WAIT_COUNT 10
> #define I40E_FDIR_WAIT_INTERVAL_US 1000
> +#define I40E_FDIR_MAX_WAIT (I40E_FDIR_WAIT_COUNT *
> +I40E_FDIR_WAIT_INTERVAL_US)
It looks like I40E_FDIR_WAIT_COUNT and I40E_FDIR_WAIT_INTERVAL_US not used anywhere else, is there any value to keep them?
why not:
#define I40E_FDIR_MAX_WAIT_US 10000 /* 10 ms */
>
> /* Wait count and interval for fdir filter flush */
> #define I40E_FDIR_FLUSH_RETRY 50
> @@ -1295,28 +1296,27 @@ i40e_fdir_filter_programming(struct i40e_pf *pf,
> /* Update the tx tail register */
> rte_wmb();
> I40E_PCI_REG_WRITE(txq->qtx_tail, txq->tx_tail);
> -
> - for (i = 0; i < I40E_FDIR_WAIT_COUNT; i++) {
> - rte_delay_us(I40E_FDIR_WAIT_INTERVAL_US);
> + for (i = 0; i < I40E_FDIR_MAX_WAIT; i++) {
> if ((txdp->cmd_type_offset_bsz &
> rte_cpu_to_le_64(I40E_TXD_QW1_DTYPE_MASK)) ==
> rte_cpu_to_le_64(I40E_TX_DESC_DTYPE_DESC_DONE))
> break;
> + rte_delay_us(1);
> }
> - if (i >= I40E_FDIR_WAIT_COUNT) {
> + if (i >= I40E_FDIR_MAX_WAIT) {
> PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
> " time out to get DD on tx queue.");
> return -ETIMEDOUT;
> }
> /* totally delay 10 ms to check programming status*/
> - rte_delay_us((I40E_FDIR_WAIT_COUNT - i) * I40E_FDIR_WAIT_INTERVAL_US);
> - if (i40e_check_fdir_programming_status(rxq) < 0) {
> - PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
> - " programming status reported.");
> - return -ENOSYS;
> + for (; i < I40E_FDIR_MAX_WAIT; i++) {
> + if (i40e_check_fdir_programming_status(rxq) >= 0)
> + return 0;
> + rte_delay_us(1);
> }
> -
> - return 0;
> + PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
> + " programming status reported.");
> + return -ETIMEDOUT;
> }
>
> /*
>
On 5/17/2017 3:46 PM, Michael Lilja wrote:
> It's ok. I didn't write the original code so I cannot tell why the two defines were made in the initial case. It make sense to remove them, but the maintainers must have had a reason, maybe they are needed in a future version of the code?
In original code, they have a meaning:
for (i = 0; i < I40E_FDIR_WAIT_COUNT; i++)
rte_delay_us(I40E_FDIR_WAIT_INTERVAL_US);
wait step is I40E_FDIR_WAIT_INTERVAL_US.
But you changed to fixes 1us stepping. So WAIT_COUNT and
WAIT_INTERVAL_US are no more meaningful. And since they are not used
anywhere else, I think they can go away.
And we can wait from maintainers ack for any "plan to use in the future"
case.
Thanks,
ferruh
>
> /Michael
>
> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: 17 May 2017 16:44
> To: Michael Lilja <ml@napatech.com>; helin.zhang@intel.com; jingjing.wu@intel.com
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v7] net/i40e: improved FDIR programming times
>
> On 5/17/2017 3:31 PM, Michael Lilja wrote:
>> Previously, the FDIR programming time is +11ms on i40e.
>> This patch will result in an average programming time of 22usec with a
>> max of 60usec .
>>
>> Signed-off-by: Michael Lilja <ml@napatech.com>
>
> Sorry for multiple, minor change requests ...
>
>>
>> ---
>> v7:
>> * Code style changes
>>
>> v6:
>> * Fixed code style issues
>>
>> v5:
>> * Reinitialization of "i" inconsistent with original intent
>>
>> v4:
>> * Code style fix
>>
>> v3:
>> * Replaced commit message
>>
>> v2:
>> * Code style fix
>>
>> v1:
>> * Initial version
>> ---
>> ---
>> drivers/net/i40e/i40e_fdir.c | 22 +++++++++++-----------
>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/i40e/i40e_fdir.c
>> b/drivers/net/i40e/i40e_fdir.c index 28cc554f5..1192d5831 100644
>> --- a/drivers/net/i40e/i40e_fdir.c
>> +++ b/drivers/net/i40e/i40e_fdir.c
>> @@ -76,6 +76,7 @@
>> /* Wait count and interval for fdir filter programming */
>> #define I40E_FDIR_WAIT_COUNT 10
>> #define I40E_FDIR_WAIT_INTERVAL_US 1000
>> +#define I40E_FDIR_MAX_WAIT (I40E_FDIR_WAIT_COUNT *
>> +I40E_FDIR_WAIT_INTERVAL_US)
>
> It looks like I40E_FDIR_WAIT_COUNT and I40E_FDIR_WAIT_INTERVAL_US not used anywhere else, is there any value to keep them?
>
> why not:
> #define I40E_FDIR_MAX_WAIT_US 10000 /* 10 ms */
>
>>
>> /* Wait count and interval for fdir filter flush */
>> #define I40E_FDIR_FLUSH_RETRY 50
>> @@ -1295,28 +1296,27 @@ i40e_fdir_filter_programming(struct i40e_pf *pf,
>> /* Update the tx tail register */
>> rte_wmb();
>> I40E_PCI_REG_WRITE(txq->qtx_tail, txq->tx_tail);
>> -
>> -for (i = 0; i < I40E_FDIR_WAIT_COUNT; i++) {
>> -rte_delay_us(I40E_FDIR_WAIT_INTERVAL_US);
>> +for (i = 0; i < I40E_FDIR_MAX_WAIT; i++) {
>> if ((txdp->cmd_type_offset_bsz &
>> rte_cpu_to_le_64(I40E_TXD_QW1_DTYPE_MASK)) ==
>> rte_cpu_to_le_64(I40E_TX_DESC_DTYPE_DESC_DONE))
>> break;
>> +rte_delay_us(1);
>> }
>> -if (i >= I40E_FDIR_WAIT_COUNT) {
>> +if (i >= I40E_FDIR_MAX_WAIT) {
>> PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
>> " time out to get DD on tx queue.");
>> return -ETIMEDOUT;
>> }
>> /* totally delay 10 ms to check programming status*/
>> -rte_delay_us((I40E_FDIR_WAIT_COUNT - i) * I40E_FDIR_WAIT_INTERVAL_US);
>> -if (i40e_check_fdir_programming_status(rxq) < 0) {
>> -PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
>> - " programming status reported.");
>> -return -ENOSYS;
>> +for (; i < I40E_FDIR_MAX_WAIT; i++) {
>> +if (i40e_check_fdir_programming_status(rxq) >= 0)
>> +return 0;
>> +rte_delay_us(1);
>> }
>> -
>> -return 0;
>> +PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
>> +" programming status reported.");
>> +return -ETIMEDOUT;
>> }
>>
>> /*
>>
>
> Disclaimer: This email and any files transmitted with it may contain confidential information intended for the addressee(s) only. The information is not to be surrendered or copied to unauthorized persons. If you have received this communication in error, please notify the sender immediately and delete this e-mail from your system.
>
Ok, I'll make a v8 removing the define.
/Michael
-----Original Message-----
From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
Sent: 17 May 2017 16:50
To: Michael Lilja <ml@napatech.com>; helin.zhang@intel.com; jingjing.wu@intel.com
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v7] net/i40e: improved FDIR programming times
On 5/17/2017 3:46 PM, Michael Lilja wrote:
> It's ok. I didn't write the original code so I cannot tell why the two defines were made in the initial case. It make sense to remove them, but the maintainers must have had a reason, maybe they are needed in a future version of the code?
In original code, they have a meaning:
for (i = 0; i < I40E_FDIR_WAIT_COUNT; i++)
rte_delay_us(I40E_FDIR_WAIT_INTERVAL_US);
wait step is I40E_FDIR_WAIT_INTERVAL_US.
But you changed to fixes 1us stepping. So WAIT_COUNT and WAIT_INTERVAL_US are no more meaningful. And since they are not used anywhere else, I think they can go away.
And we can wait from maintainers ack for any "plan to use in the future"
case.
Thanks,
ferruh
>
> /Michael
>
> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: 17 May 2017 16:44
> To: Michael Lilja <ml@napatech.com>; helin.zhang@intel.com;
> jingjing.wu@intel.com
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v7] net/i40e: improved FDIR programming
> times
>
> On 5/17/2017 3:31 PM, Michael Lilja wrote:
>> Previously, the FDIR programming time is +11ms on i40e.
>> This patch will result in an average programming time of 22usec with
>> a max of 60usec .
>>
>> Signed-off-by: Michael Lilja <ml@napatech.com>
>
> Sorry for multiple, minor change requests ...
>
>>
>> ---
>> v7:
>> * Code style changes
>>
>> v6:
>> * Fixed code style issues
>>
>> v5:
>> * Reinitialization of "i" inconsistent with original intent
>>
>> v4:
>> * Code style fix
>>
>> v3:
>> * Replaced commit message
>>
>> v2:
>> * Code style fix
>>
>> v1:
>> * Initial version
>> ---
>> ---
>> drivers/net/i40e/i40e_fdir.c | 22 +++++++++++-----------
>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/i40e/i40e_fdir.c
>> b/drivers/net/i40e/i40e_fdir.c index 28cc554f5..1192d5831 100644
>> --- a/drivers/net/i40e/i40e_fdir.c
>> +++ b/drivers/net/i40e/i40e_fdir.c
>> @@ -76,6 +76,7 @@
>> /* Wait count and interval for fdir filter programming */
>> #define I40E_FDIR_WAIT_COUNT 10
>> #define I40E_FDIR_WAIT_INTERVAL_US 1000
>> +#define I40E_FDIR_MAX_WAIT (I40E_FDIR_WAIT_COUNT *
>> +I40E_FDIR_WAIT_INTERVAL_US)
>
> It looks like I40E_FDIR_WAIT_COUNT and I40E_FDIR_WAIT_INTERVAL_US not used anywhere else, is there any value to keep them?
>
> why not:
> #define I40E_FDIR_MAX_WAIT_US 10000 /* 10 ms */
>
>>
>> /* Wait count and interval for fdir filter flush */
>> #define I40E_FDIR_FLUSH_RETRY 50
>> @@ -1295,28 +1296,27 @@ i40e_fdir_filter_programming(struct i40e_pf
>> *pf,
>> /* Update the tx tail register */
>> rte_wmb();
>> I40E_PCI_REG_WRITE(txq->qtx_tail, txq->tx_tail);
>> -
>> -for (i = 0; i < I40E_FDIR_WAIT_COUNT; i++) {
>> -rte_delay_us(I40E_FDIR_WAIT_INTERVAL_US);
>> +for (i = 0; i < I40E_FDIR_MAX_WAIT; i++) {
>> if ((txdp->cmd_type_offset_bsz &
>> rte_cpu_to_le_64(I40E_TXD_QW1_DTYPE_MASK)) ==
>> rte_cpu_to_le_64(I40E_TX_DESC_DTYPE_DESC_DONE))
>> break;
>> +rte_delay_us(1);
>> }
>> -if (i >= I40E_FDIR_WAIT_COUNT) {
>> +if (i >= I40E_FDIR_MAX_WAIT) {
>> PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
>> " time out to get DD on tx queue."); return -ETIMEDOUT; }
>> /* totally delay 10 ms to check programming status*/
>> -rte_delay_us((I40E_FDIR_WAIT_COUNT - i) *
>> I40E_FDIR_WAIT_INTERVAL_US); -if
>> (i40e_check_fdir_programming_status(rxq) < 0) { -PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
>> - " programming status reported.");
>> -return -ENOSYS;
>> +for (; i < I40E_FDIR_MAX_WAIT; i++) { if
>> +(i40e_check_fdir_programming_status(rxq) >= 0) return 0;
>> +rte_delay_us(1);
>> }
>> -
>> -return 0;
>> +PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
>> +" programming status reported.");
>> +return -ETIMEDOUT;
>> }
>>
>> /*
>>
>
> Disclaimer: This email and any files transmitted with it may contain confidential information intended for the addressee(s) only. The information is not to be surrendered or copied to unauthorized persons. If you have received this communication in error, please notify the sender immediately and delete this e-mail from your system.
>
@@ -76,6 +76,7 @@
/* Wait count and interval for fdir filter programming */
#define I40E_FDIR_WAIT_COUNT 10
#define I40E_FDIR_WAIT_INTERVAL_US 1000
+#define I40E_FDIR_MAX_WAIT (I40E_FDIR_WAIT_COUNT * I40E_FDIR_WAIT_INTERVAL_US)
/* Wait count and interval for fdir filter flush */
#define I40E_FDIR_FLUSH_RETRY 50
@@ -1295,28 +1296,27 @@ i40e_fdir_filter_programming(struct i40e_pf *pf,
/* Update the tx tail register */
rte_wmb();
I40E_PCI_REG_WRITE(txq->qtx_tail, txq->tx_tail);
-
- for (i = 0; i < I40E_FDIR_WAIT_COUNT; i++) {
- rte_delay_us(I40E_FDIR_WAIT_INTERVAL_US);
+ for (i = 0; i < I40E_FDIR_MAX_WAIT; i++) {
if ((txdp->cmd_type_offset_bsz &
rte_cpu_to_le_64(I40E_TXD_QW1_DTYPE_MASK)) ==
rte_cpu_to_le_64(I40E_TX_DESC_DTYPE_DESC_DONE))
break;
+ rte_delay_us(1);
}
- if (i >= I40E_FDIR_WAIT_COUNT) {
+ if (i >= I40E_FDIR_MAX_WAIT) {
PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
" time out to get DD on tx queue.");
return -ETIMEDOUT;
}
/* totally delay 10 ms to check programming status*/
- rte_delay_us((I40E_FDIR_WAIT_COUNT - i) * I40E_FDIR_WAIT_INTERVAL_US);
- if (i40e_check_fdir_programming_status(rxq) < 0) {
- PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
- " programming status reported.");
- return -ENOSYS;
+ for (; i < I40E_FDIR_MAX_WAIT; i++) {
+ if (i40e_check_fdir_programming_status(rxq) >= 0)
+ return 0;
+ rte_delay_us(1);
}
-
- return 0;
+ PMD_DRV_LOG(ERR, "Failed to program FDIR filter:"
+ " programming status reported.");
+ return -ETIMEDOUT;
}
/*