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); -- 2.30.2
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
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.
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. >
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. >>
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