net/mlx5: fix Rx descriptor status returned value
diff mbox series

Message ID 20200313095659.19000-1-didier.pallard@6wind.com
State Superseded, archived
Delegated to: Raslan Darawsheh
Headers show
Series
  • net/mlx5: fix Rx descriptor status returned value
Related show

Checks

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

Commit Message

Didier Pallard March 13, 2020, 9:56 a.m. UTC
Two bugs in rx_queue_count function:
- One entry may contain several segments, so 'used' must be multiplied
  by number of segments per entry to properly reflect the queue usage.
- rx_queue_count returns the number of entries used in queue, so it ranges
  from 0 to max number of entries in queue, not this number minus
  one.

Fixes: 8788fec1f269 ("net/mlx5: implement descriptor status API")
Cc: stable@dpdk.org
Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
---
 drivers/net/mlx5/mlx5_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Slava Ovsiienko March 16, 2020, 4:05 p.m. UTC | #1
Hi, Didier

First, thank you for the patch.

If we have a look at the description of rte_eth_rx_queue_count(): "Get the number of used descriptors of a rx queue".
It means the DPDK generic descriptors, not PMD specific ones. "DPDK descriptor" means the entity which can handle one packet.
rte_eth_rx_queue_count() should return the potential number of packets could be fetched from the Rx queue on the next rx_burst() call.
Application should know anything about PMD descriptors, it must be isolated. So, rx_queue_count() should return the number of expected
packets not hardware entries. That's why the value being returned is compared with elts, not with HW desctriptors.

As for -1 - I agree, should be fixed.

It seems we have another bug - the Rx queue is created with number of hardware descriptors which does not correspond the requested packets
in case of multi-segment packets  (requested desc is divided by (1<<sges_n), it seems it should not).
 
With best regards, Slava

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Didier Pallard
> Sent: Friday, March 13, 2020 11:57
> To: dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: [dpdk-dev] [PATCH] net/mlx5: fix Rx descriptor status returned value
> 
> Two bugs in rx_queue_count function:
> - One entry may contain several segments, so 'used' must be multiplied
>   by number of segments per entry to properly reflect the queue usage.
> - rx_queue_count returns the number of entries used in queue, so it ranges
>   from 0 to max number of entries in queue, not this number minus
>   one.
> 
> Fixes: 8788fec1f269 ("net/mlx5: implement descriptor status API")
> Cc: stable@dpdk.org
> Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> ---
>  drivers/net/mlx5/mlx5_rxtx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c index
> 5ac63da8039d..17f80c25443e 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -500,7 +500,7 @@ rx_queue_count(struct mlx5_rxq_data *rxq)
>  		used += n;
>  		cqe = &(*rxq->cqes)[cq_ci & cqe_cnt];
>  	}
> -	used = RTE_MIN(used, (1U << rxq->elts_n) - 1);
> +	used = RTE_MIN(used * (1 << rxq->sges_n), 1U << rxq->elts_n);
>  	return used;
>  }
> 
> --
> 2.24.1
Didier Pallard March 16, 2020, 5:24 p.m. UTC | #2
Well, you're right, another way to fix the problem could be to set up the
queue size assuming the provided number
is a number of packets in queue rather than a number of mbufs in queue.
But not sure it's better, it's also important for the application/user to
know the number of mbufs that could fit in a rx/tx queue,
whatever the number of packets that it covers, since it is very important
to size the memory pools correctly to avoid any
mbuf shortage during system life.
Thanks
Didier

On Mon, Mar 16, 2020 at 5:05 PM Slava Ovsiienko <viacheslavo@mellanox.com>
wrote:

> Hi, Didier
>
> First, thank you for the patch.
>
> If we have a look at the description of rte_eth_rx_queue_count(): "Get the
> number of used descriptors of a rx queue".
> It means the DPDK generic descriptors, not PMD specific ones. "DPDK
> descriptor" means the entity which can handle one packet.
> rte_eth_rx_queue_count() should return the potential number of packets
> could be fetched from the Rx queue on the next rx_burst() call.
> Application should know anything about PMD descriptors, it must be
> isolated. So, rx_queue_count() should return the number of expected
> packets not hardware entries. That's why the value being returned is
> compared with elts, not with HW desctriptors.
>
> As for -1 - I agree, should be fixed.
>
> It seems we have another bug - the Rx queue is created with number of
> hardware descriptors which does not correspond the requested packets
> in case of multi-segment packets  (requested desc is divided by
> (1<<sges_n), it seems it should not).
>
> With best regards, Slava
>
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Didier Pallard
> > Sent: Friday, March 13, 2020 11:57
> > To: dev@dpdk.org
> > Cc: stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH] net/mlx5: fix Rx descriptor status returned
> value
> >
> > Two bugs in rx_queue_count function:
> > - One entry may contain several segments, so 'used' must be multiplied
> >   by number of segments per entry to properly reflect the queue usage.
> > - rx_queue_count returns the number of entries used in queue, so it
> ranges
> >   from 0 to max number of entries in queue, not this number minus
> >   one.
> >
> > Fixes: 8788fec1f269 ("net/mlx5: implement descriptor status API")
> > Cc: stable@dpdk.org
> > Signed-off-by: Didier Pallard <didier.pallard@6wind.com>
> > ---
> >  drivers/net/mlx5/mlx5_rxtx.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> index
> > 5ac63da8039d..17f80c25443e 100644
> > --- a/drivers/net/mlx5/mlx5_rxtx.c
> > +++ b/drivers/net/mlx5/mlx5_rxtx.c
> > @@ -500,7 +500,7 @@ rx_queue_count(struct mlx5_rxq_data *rxq)
> >               used += n;
> >               cqe = &(*rxq->cqes)[cq_ci & cqe_cnt];
> >       }
> > -     used = RTE_MIN(used, (1U << rxq->elts_n) - 1);
> > +     used = RTE_MIN(used * (1 << rxq->sges_n), 1U << rxq->elts_n);
> >       return used;
> >  }
> >
> > --
> > 2.24.1
>
>
Slava Ovsiienko March 17, 2020, 8:33 a.m. UTC | #3
>> From: Didier Pallard <didier.pallard@6wind.com> 
>> Sent: Monday, March 16, 2020 19:24
>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
>> Cc: dev@dpdk.org; stable@dpdk.org; Matan Azrad <matan@mellanox.com>
>> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix Rx descriptor status returned value
>> Well, you're right, another way to fix the problem could be to set up the queue size assuming the provided number
>> is a number of packets in queue rather than a number of mbufs in queue.

Yes, it is intended in queue setup routine. But, for mlx5 we have a bug for regular mlx5_rx_burst if scattering is enabled,
the Rx queue is created with size wqe_n elements, should be wqe_n << sges_n instead, to be able to receive the requested
number of packets (wqe_n). I think we must fix. Would you like to update your patch, or should I provide mine? 

>> But not sure it's better, it's also important for the application/user to know the number of mbufs that could fit in a rx/tx queue,
>> whatever the number of packets that it covers, since it is very important to size the memory pools correctly to avoid any
>> mbuf shortage during system life.
>> Thanks
>> Didier
To estimate - the number of "DPDK descriptors" should be multiplied by the maximal length of scattered packet chain.

With best regards, Slava

> -----Original Message-----
> From: dev <mailto:dev-bounces@dpdk.org> On Behalf Of Didier Pallard
> Sent: Friday, March 13, 2020 11:57
> To: mailto:dev@dpdk.org
> Cc: mailto:stable@dpdk.org
> Subject: [dpdk-dev] [PATCH] net/mlx5: fix Rx descriptor status returned value
> 
> Two bugs in rx_queue_count function:
> - One entry may contain several segments, so 'used' must be multiplied
>   by number of segments per entry to properly reflect the queue usage.
> - rx_queue_count returns the number of entries used in queue, so it ranges
>   from 0 to max number of entries in queue, not this number minus
>   one.
> 
> Fixes: 8788fec1f269 ("net/mlx5: implement descriptor status API")
> Cc: mailto:stable@dpdk.org
> Signed-off-by: Didier Pallard <mailto:didier.pallard@6wind.com>
> ---
>  drivers/net/mlx5/mlx5_rxtx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c index
> 5ac63da8039d..17f80c25443e 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -500,7 +500,7 @@ rx_queue_count(struct mlx5_rxq_data *rxq)
>               used += n;
>               cqe = &(*rxq->cqes)[cq_ci & cqe_cnt];
>       }
> -     used = RTE_MIN(used, (1U << rxq->elts_n) - 1);
> +     used = RTE_MIN(used * (1 << rxq->sges_n), 1U << rxq->elts_n);
>       return used;
>  }
> 
> --
> 2.24.1
Didier Pallard March 17, 2020, 9:19 a.m. UTC | #4
well, please do if you don't mind,
You will validate it quicker, and I'm currently working on a
different topic.
Btw, do you think it's possible to have an implementation of
[r,t]x_status_descriptor functions for vectorized implementation?
thanks
didier

On Tue, Mar 17, 2020 at 9:33 AM Slava Ovsiienko <viacheslavo@mellanox.com>
wrote:

> >> From: Didier Pallard <didier.pallard@6wind.com>
> >> Sent: Monday, March 16, 2020 19:24
> >> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org; Matan Azrad <matan@mellanox.com>
> >> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix Rx descriptor status
> returned value
> >> Well, you're right, another way to fix the problem could be to set up
> the queue size assuming the provided number
> >> is a number of packets in queue rather than a number of mbufs in queue.
>
> Yes, it is intended in queue setup routine. But, for mlx5 we have a bug
> for regular mlx5_rx_burst if scattering is enabled,
> the Rx queue is created with size wqe_n elements, should be wqe_n <<
> sges_n instead, to be able to receive the requested
> number of packets (wqe_n). I think we must fix. Would you like to update
> your patch, or should I provide mine?
>
> >> But not sure it's better, it's also important for the application/user
> to know the number of mbufs that could fit in a rx/tx queue,
> >> whatever the number of packets that it covers, since it is very
> important to size the memory pools correctly to avoid any
> >> mbuf shortage during system life.
> >> Thanks
> >> Didier
> To estimate - the number of "DPDK descriptors" should be multiplied by the
> maximal length of scattered packet chain.
>
> With best regards, Slava
>
> > -----Original Message-----
> > From: dev <mailto:dev-bounces@dpdk.org> On Behalf Of Didier Pallard
> > Sent: Friday, March 13, 2020 11:57
> > To: mailto:dev@dpdk.org
> > Cc: mailto:stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH] net/mlx5: fix Rx descriptor status returned
> value
> >
> > Two bugs in rx_queue_count function:
> > - One entry may contain several segments, so 'used' must be multiplied
> >   by number of segments per entry to properly reflect the queue usage.
> > - rx_queue_count returns the number of entries used in queue, so it
> ranges
> >   from 0 to max number of entries in queue, not this number minus
> >   one.
> >
> > Fixes: 8788fec1f269 ("net/mlx5: implement descriptor status API")
> > Cc: mailto:stable@dpdk.org
> > Signed-off-by: Didier Pallard <mailto:didier.pallard@6wind.com>
> > ---
> >  drivers/net/mlx5/mlx5_rxtx.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> index
> > 5ac63da8039d..17f80c25443e 100644
> > --- a/drivers/net/mlx5/mlx5_rxtx.c
> > +++ b/drivers/net/mlx5/mlx5_rxtx.c
> > @@ -500,7 +500,7 @@ rx_queue_count(struct mlx5_rxq_data *rxq)
> >               used += n;
> >               cqe = &(*rxq->cqes)[cq_ci & cqe_cnt];
> >       }
> > -     used = RTE_MIN(used, (1U << rxq->elts_n) - 1);
> > +     used = RTE_MIN(used * (1 << rxq->sges_n), 1U << rxq->elts_n);
> >       return used;
> >  }
> >
> > --
> > 2.24.1
>
Slava Ovsiienko March 17, 2020, 9:25 a.m. UTC | #5
From: Didier Pallard <didier.pallard@6wind.com>
Sent: Tuesday, March 17, 2020 11:19
To: Slava Ovsiienko <viacheslavo@mellanox.com>
Cc: dev@dpdk.org; stable@dpdk.org; Matan Azrad <matan@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix Rx descriptor status returned value
>>>well, please do if you don't mind,
OK, I will.

>>>You will validate it quicker, and I'm currently working on a different topic.
>>>Btw, do you think it's possible to have an implementation of [r,t]x_status_descriptor functions for vectorized implementation?
>>>thanks
Yes, we are planning this update.

With best regards, Slava


didier

On Tue, Mar 17, 2020 at 9:33 AM Slava Ovsiienko <viacheslavo@mellanox.com<mailto:viacheslavo@mellanox.com>> wrote:
>> From: Didier Pallard <didier.pallard@6wind.com<mailto:didier.pallard@6wind.com>>
>> Sent: Monday, March 16, 2020 19:24
>> To: Slava Ovsiienko <viacheslavo@mellanox.com<mailto:viacheslavo@mellanox.com>>
>> Cc: dev@dpdk.org<mailto:dev@dpdk.org>; stable@dpdk.org<mailto:stable@dpdk.org>; Matan Azrad <matan@mellanox.com<mailto:matan@mellanox.com>>
>> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix Rx descriptor status returned value
>> Well, you're right, another way to fix the problem could be to set up the queue size assuming the provided number
>> is a number of packets in queue rather than a number of mbufs in queue.

Yes, it is intended in queue setup routine. But, for mlx5 we have a bug for regular mlx5_rx_burst if scattering is enabled,
the Rx queue is created with size wqe_n elements, should be wqe_n << sges_n instead, to be able to receive the requested
number of packets (wqe_n). I think we must fix. Would you like to update your patch, or should I provide mine?

>> But not sure it's better, it's also important for the application/user to know the number of mbufs that could fit in a rx/tx queue,
>> whatever the number of packets that it covers, since it is very important to size the memory pools correctly to avoid any
>> mbuf shortage during system life.
>> Thanks
>> Didier
To estimate - the number of "DPDK descriptors" should be multiplied by the maximal length of scattered packet chain.

With best regards, Slava

> -----Original Message-----
> From: dev <mailto:dev-bounces@dpdk.org<mailto:dev-bounces@dpdk.org>> On Behalf Of Didier Pallard
> Sent: Friday, March 13, 2020 11:57
> To: mailto:dev@dpdk.org<mailto:dev@dpdk.org>
> Cc: mailto:stable@dpdk.org<mailto:stable@dpdk.org>
> Subject: [dpdk-dev] [PATCH] net/mlx5: fix Rx descriptor status returned value
>
> Two bugs in rx_queue_count function:
> - One entry may contain several segments, so 'used' must be multiplied
>   by number of segments per entry to properly reflect the queue usage.
> - rx_queue_count returns the number of entries used in queue, so it ranges
>   from 0 to max number of entries in queue, not this number minus
>   one.
>
> Fixes: 8788fec1f269 ("net/mlx5: implement descriptor status API")
> Cc: mailto:stable@dpdk.org<mailto:stable@dpdk.org>
> Signed-off-by: Didier Pallard <mailto:didier.pallard@6wind.com<mailto:didier.pallard@6wind.com>>
> ---
>  drivers/net/mlx5/mlx5_rxtx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c index
> 5ac63da8039d..17f80c25443e 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -500,7 +500,7 @@ rx_queue_count(struct mlx5_rxq_data *rxq)
>               used += n;
>               cqe = &(*rxq->cqes)[cq_ci & cqe_cnt];
>       }
> -     used = RTE_MIN(used, (1U << rxq->elts_n) - 1);
> +     used = RTE_MIN(used * (1 << rxq->sges_n), 1U << rxq->elts_n);
>       return used;
>  }
>
> --
> 2.24.1
Didier Pallard March 17, 2020, 9:41 a.m. UTC | #6
OK, thank you
best regards
didier

On Tue, Mar 17, 2020 at 10:25 AM Slava Ovsiienko <viacheslavo@mellanox.com>
wrote:

> *From:* Didier Pallard <didier.pallard@6wind.com>
> *Sent:* Tuesday, March 17, 2020 11:19
> *To:* Slava Ovsiienko <viacheslavo@mellanox.com>
> *Cc:* dev@dpdk.org; stable@dpdk.org; Matan Azrad <matan@mellanox.com>
> *Subject:* Re: [dpdk-dev] [PATCH] net/mlx5: fix Rx descriptor status
> returned value
>
> >>>well, please do if you don't mind,
>
> OK, I will.
>
>
>
> >>>You will validate it quicker, and I'm currently working on a
> different topic.
>
> >>>Btw, do you think it's possible to have an implementation of
> [r,t]x_status_descriptor functions for vectorized implementation?
>
> >>>thanks
>
> Yes, we are planning this update.
>
>
>
> With best regards, Slava
>
>
>
>
>
> didier
>
>
>
> On Tue, Mar 17, 2020 at 9:33 AM Slava Ovsiienko <viacheslavo@mellanox.com>
> wrote:
>
> >> From: Didier Pallard <didier.pallard@6wind.com>
> >> Sent: Monday, March 16, 2020 19:24
> >> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> >> Cc: dev@dpdk.org; stable@dpdk.org; Matan Azrad <matan@mellanox.com>
> >> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix Rx descriptor status
> returned value
> >> Well, you're right, another way to fix the problem could be to set up
> the queue size assuming the provided number
> >> is a number of packets in queue rather than a number of mbufs in queue.
>
> Yes, it is intended in queue setup routine. But, for mlx5 we have a bug
> for regular mlx5_rx_burst if scattering is enabled,
> the Rx queue is created with size wqe_n elements, should be wqe_n <<
> sges_n instead, to be able to receive the requested
> number of packets (wqe_n). I think we must fix. Would you like to update
> your patch, or should I provide mine?
>
> >> But not sure it's better, it's also important for the application/user
> to know the number of mbufs that could fit in a rx/tx queue,
> >> whatever the number of packets that it covers, since it is very
> important to size the memory pools correctly to avoid any
> >> mbuf shortage during system life.
> >> Thanks
> >> Didier
> To estimate - the number of "DPDK descriptors" should be multiplied by the
> maximal length of scattered packet chain.
>
> With best regards, Slava
>
> > -----Original Message-----
> > From: dev <mailto:dev-bounces@dpdk.org> On Behalf Of Didier Pallard
> > Sent: Friday, March 13, 2020 11:57
> > To: mailto:dev@dpdk.org
> > Cc: mailto:stable@dpdk.org
> > Subject: [dpdk-dev] [PATCH] net/mlx5: fix Rx descriptor status returned
> value
> >
> > Two bugs in rx_queue_count function:
> > - One entry may contain several segments, so 'used' must be multiplied
> >   by number of segments per entry to properly reflect the queue usage.
> > - rx_queue_count returns the number of entries used in queue, so it
> ranges
> >   from 0 to max number of entries in queue, not this number minus
> >   one.
> >
> > Fixes: 8788fec1f269 ("net/mlx5: implement descriptor status API")
> > Cc: mailto:stable@dpdk.org
> > Signed-off-by: Didier Pallard <mailto:didier.pallard@6wind.com>
> > ---
> >  drivers/net/mlx5/mlx5_rxtx.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> index
> > 5ac63da8039d..17f80c25443e 100644
> > --- a/drivers/net/mlx5/mlx5_rxtx.c
> > +++ b/drivers/net/mlx5/mlx5_rxtx.c
> > @@ -500,7 +500,7 @@ rx_queue_count(struct mlx5_rxq_data *rxq)
> >               used += n;
> >               cqe = &(*rxq->cqes)[cq_ci & cqe_cnt];
> >       }
> > -     used = RTE_MIN(used, (1U << rxq->elts_n) - 1);
> > +     used = RTE_MIN(used * (1 << rxq->sges_n), 1U << rxq->elts_n);
> >       return used;
> >  }
> >
> > --
> > 2.24.1
>
>

Patch
diff mbox series

diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
index 5ac63da8039d..17f80c25443e 100644
--- a/drivers/net/mlx5/mlx5_rxtx.c
+++ b/drivers/net/mlx5/mlx5_rxtx.c
@@ -500,7 +500,7 @@  rx_queue_count(struct mlx5_rxq_data *rxq)
 		used += n;
 		cqe = &(*rxq->cqes)[cq_ci & cqe_cnt];
 	}
-	used = RTE_MIN(used, (1U << rxq->elts_n) - 1);
+	used = RTE_MIN(used * (1 << rxq->sges_n), 1U << rxq->elts_n);
 	return used;
 }