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.yi...@intel.com]
> Sent: 17 May 2017 16:44
> To: Michael Lilja <m...@napatech.com>; helin.zh...@intel.com;
> jingjing...@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 <m...@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.
>
