diff mbox series

net/virtio: remove handling of zero desc number on RxQ setup

Message ID 20210820124741.3522576-1-andrew.rybchenko@oktetlabs.ru (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: remove handling of zero desc number on RxQ setup |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot: build success github build: passed
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Andrew Rybchenko Aug. 20, 2021, 12:47 p.m. UTC 
  From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>

Rx queue setup callback allows to use the whole ring when
descriptor number argument equals zero. There's no point to
handle zero in any way since RTE Rx queue setup function
rte_eth_rx_queue_setup() doesn't pass zero using fallback
values.

Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
Cc: stable@dpdk.org

Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
 drivers/net/virtio/virtio_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Maxime Coquelin Sept. 13, 2021, 7:25 p.m. UTC | #1 
On 8/20/21 2:47 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Rx queue setup callback allows to use the whole ring when
> descriptor number argument equals zero. There's no point to
> handle zero in any way since RTE Rx queue setup function
> rte_eth_rx_queue_setup() doesn't pass zero using fallback
> values.
> 
> Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
>  drivers/net/virtio/virtio_rxtx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 8a48fba5cc..18f03c9fc9 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -706,7 +706,7 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
>  	}
>  	vq->vq_free_thresh = rx_free_thresh;
>  
> -	if (nb_desc == 0 || nb_desc > vq->vq_nentries)
> +	if (nb_desc > vq->vq_nentries)
>  		nb_desc = vq->vq_nentries;
>  	vq->vq_free_cnt = RTE_MIN(vq->vq_free_cnt, nb_desc);
>  
> 

Is that really a fix?
I see it more like an optimization in a cold path, so maybe it is not
worth backporting?

Other than that:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime
Andrew Rybchenko Sept. 14, 2021, 6:40 a.m. UTC | #2 
On 9/13/21 10:25 PM, Maxime Coquelin wrote:
> 
> 
> On 8/20/21 2:47 PM, Andrew Rybchenko wrote:
>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>
>> Rx queue setup callback allows to use the whole ring when
>> descriptor number argument equals zero. There's no point to
>> handle zero in any way since RTE Rx queue setup function
>> rte_eth_rx_queue_setup() doesn't pass zero using fallback
>> values.
>>
>> Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> ---
>>  drivers/net/virtio/virtio_rxtx.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>> index 8a48fba5cc..18f03c9fc9 100644
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -706,7 +706,7 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
>>  	}
>>  	vq->vq_free_thresh = rx_free_thresh;
>>  
>> -	if (nb_desc == 0 || nb_desc > vq->vq_nentries)
>> +	if (nb_desc > vq->vq_nentries)
>>  		nb_desc = vq->vq_nentries;
>>  	vq->vq_free_cnt = RTE_MIN(vq->vq_free_cnt, nb_desc);
>>  
>>
> 
> Is that really a fix?
> I see it more like an optimization in a cold path, so maybe it is not
> worth backporting?

The main idea is not an optimization, but simplification of
the code to make it easier to understand. Less special
cases is better.

I agree that it does not make sense to backport it.

> Other than that:
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Andrew.
Maxime Coquelin Sept. 14, 2021, 7:26 a.m. UTC | #3 
On 9/14/21 8:40 AM, Andrew Rybchenko wrote:
> On 9/13/21 10:25 PM, Maxime Coquelin wrote:
>>
>>
>> On 8/20/21 2:47 PM, Andrew Rybchenko wrote:
>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>
>>> Rx queue setup callback allows to use the whole ring when
>>> descriptor number argument equals zero. There's no point to
>>> handle zero in any way since RTE Rx queue setup function
>>> rte_eth_rx_queue_setup() doesn't pass zero using fallback
>>> values.
>>>
>>> Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>> ---
>>>  drivers/net/virtio/virtio_rxtx.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>>> index 8a48fba5cc..18f03c9fc9 100644
>>> --- a/drivers/net/virtio/virtio_rxtx.c
>>> +++ b/drivers/net/virtio/virtio_rxtx.c
>>> @@ -706,7 +706,7 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
>>>  	}
>>>  	vq->vq_free_thresh = rx_free_thresh;
>>>  
>>> -	if (nb_desc == 0 || nb_desc > vq->vq_nentries)
>>> +	if (nb_desc > vq->vq_nentries)
>>>  		nb_desc = vq->vq_nentries;
>>>  	vq->vq_free_cnt = RTE_MIN(vq->vq_free_cnt, nb_desc);
>>>  
>>>
>>
>> Is that really a fix?
>> I see it more like an optimization in a cold path, so maybe it is not
>> worth backporting?
> 
> The main idea is not an optimization, but simplification of
> the code to make it easier to understand. Less special
> cases is better.
> 
> I agree that it does not make sense to backport it.

Ok, thanks. I'll will remove the Fixes tag while applying, no need to
resubmit.

Maxime
> 
>> Other than that:
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Thanks,
> Andrew.
>
Andrew Rybchenko Sept. 14, 2021, 7:33 a.m. UTC | #4 
On 9/14/21 10:26 AM, Maxime Coquelin wrote:
> 
> 
> On 9/14/21 8:40 AM, Andrew Rybchenko wrote:
>> On 9/13/21 10:25 PM, Maxime Coquelin wrote:
>>>
>>>
>>> On 8/20/21 2:47 PM, Andrew Rybchenko wrote:
>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>
>>>> Rx queue setup callback allows to use the whole ring when
>>>> descriptor number argument equals zero. There's no point to
>>>> handle zero in any way since RTE Rx queue setup function
>>>> rte_eth_rx_queue_setup() doesn't pass zero using fallback
>>>> values.
>>>>
>>>> Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> ---
>>>>  drivers/net/virtio/virtio_rxtx.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>>>> index 8a48fba5cc..18f03c9fc9 100644
>>>> --- a/drivers/net/virtio/virtio_rxtx.c
>>>> +++ b/drivers/net/virtio/virtio_rxtx.c
>>>> @@ -706,7 +706,7 @@ virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
>>>>  	}
>>>>  	vq->vq_free_thresh = rx_free_thresh;
>>>>  
>>>> -	if (nb_desc == 0 || nb_desc > vq->vq_nentries)
>>>> +	if (nb_desc > vq->vq_nentries)
>>>>  		nb_desc = vq->vq_nentries;
>>>>  	vq->vq_free_cnt = RTE_MIN(vq->vq_free_cnt, nb_desc);
>>>>  
>>>>
>>>
>>> Is that really a fix?
>>> I see it more like an optimization in a cold path, so maybe it is not
>>> worth backporting?
>>
>> The main idea is not an optimization, but simplification of
>> the code to make it easier to understand. Less special
>> cases is better.
>>
>> I agree that it does not make sense to backport it.
> 
> Ok, thanks. I'll will remove the Fixes tag while applying, no need to
> resubmit.

Thanks,
Andrew.

> Maxime
>>
>>> Other than that:
>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> Thanks,
>> Andrew.
>>
Maxime Coquelin Sept. 14, 2021, 11:24 a.m. UTC | #5 
On 8/20/21 2:47 PM, Andrew Rybchenko wrote:
> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> 
> Rx queue setup callback allows to use the whole ring when
> descriptor number argument equals zero. There's no point to
> handle zero in any way since RTE Rx queue setup function
> rte_eth_rx_queue_setup() doesn't pass zero using fallback
> values.
> 
> Fixes: 3be82f5cc5e3 ("ethdev: support PMD-tuned Tx/Rx parameters")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
>  drivers/net/virtio/virtio_rxtx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Applied to dpdk-next-virtio/main.

Thanks,
Maxime
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 8a48fba5cc..18f03c9fc9 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -706,7 +706,7 @@  virtio_dev_rx_queue_setup(struct rte_eth_dev *dev,
 	}
 	vq->vq_free_thresh = rx_free_thresh;
 
-	if (nb_desc == 0 || nb_desc > vq->vq_nentries)
+	if (nb_desc > vq->vq_nentries)
 		nb_desc = vq->vq_nentries;
 	vq->vq_free_cnt = RTE_MIN(vq->vq_free_cnt, nb_desc);