[v2,4/4] net/mlx5: engage free on completion queue
Checks
Commit Message
The free on completion queue keeps the indices of elts array,
all mbuf stored below this index should be freed on arrival
of normal send completion. In debug version it also contains
an index of completed transmitting descriptor (WQE) to check
queues synchronization.
Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>
---
drivers/net/mlx5/mlx5_rxtx.c | 33 +++++++++++++++++----------------
drivers/net/mlx5/mlx5_rxtx.h | 6 ++----
drivers/net/mlx5/mlx5_txq.c | 2 --
3 files changed, 19 insertions(+), 22 deletions(-)
Comments
On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
> The free on completion queue keeps the indices of elts array,
> all mbuf stored below this index should be freed on arrival
> of normal send completion. In debug version it also contains
> an index of completed transmitting descriptor (WQE) to check
> queues synchronization.
>
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> Acked-by: Matan Azrad <matan@mellanox.com>
<...>
> @@ -2108,17 +2108,18 @@ enum mlx5_txcmp_code {
> /*
> * We are going to fetch all entries with
> * MLX5_CQE_SYNDROME_WR_FLUSH_ERR status.
> + * The send queue is supposed to be empty.
> */
> ++ci;
> + txq->cq_pi = ci;
> + last_cqe = NULL;
> continue;
> }
> /* Normal transmit completion. */
> + assert(ci != txq->cq_pi);
> + assert((txq->fcqs[ci & txq->cqe_m] >> 16) == cqe->wqe_counter);
And same comments on these as previous patches, we spend some effort to remove
the 'rte_panic' from drivers, this is almost same thing.
I think a driver shouldn't decide to exit whole application, it's effect should
be limited to the driver.
Assert is useful for debug and during development, but not sure having them in
the production code.
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, January 9, 2020 17:19
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>; Thomas
> Monjalon <thomas@monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on completion
> queue
>
> On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
> > The free on completion queue keeps the indices of elts array, all mbuf
> > stored below this index should be freed on arrival of normal send
> > completion. In debug version it also contains an index of completed
> > transmitting descriptor (WQE) to check queues synchronization.
> >
> > Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > Acked-by: Matan Azrad <matan@mellanox.com>
>
> <...>
>
> > @@ -2108,17 +2108,18 @@ enum mlx5_txcmp_code {
> > /*
> > * We are going to fetch all entries with
> > * MLX5_CQE_SYNDROME_WR_FLUSH_ERR status.
> > + * The send queue is supposed to be empty.
> > */
> > ++ci;
> > + txq->cq_pi = ci;
> > + last_cqe = NULL;
> > continue;
> > }
> > /* Normal transmit completion. */
> > + assert(ci != txq->cq_pi);
> > + assert((txq->fcqs[ci & txq->cqe_m] >> 16) == cqe-
> >wqe_counter);
>
> And same comments on these as previous patches, we spend some effort to
> remove the 'rte_panic' from drivers, this is almost same thing.
>
> I think a driver shouldn't decide to exit whole application, it's effect should be
> limited to the driver.
>
> Assert is useful for debug and during development, but not sure having them
> in the production code.
IIRC, "assert" is standard C function. Compiled only if there is no NDEBUG defined.
So, assert does exactly what you are saying - provide the debug break
not allowing the bug to evolve. And no this break in production code.
With best regards, Slava
On 1/9/2020 3:27 PM, Slava Ovsiienko wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, January 9, 2020 17:19
>> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
>> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
>> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>; Thomas
>> Monjalon <thomas@monjalon.net>
>> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on completion
>> queue
>>
>> On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
>>> The free on completion queue keeps the indices of elts array, all mbuf
>>> stored below this index should be freed on arrival of normal send
>>> completion. In debug version it also contains an index of completed
>>> transmitting descriptor (WQE) to check queues synchronization.
>>>
>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
>>> Acked-by: Matan Azrad <matan@mellanox.com>
>>
>> <...>
>>
>>> @@ -2108,17 +2108,18 @@ enum mlx5_txcmp_code {
>>> /*
>>> * We are going to fetch all entries with
>>> * MLX5_CQE_SYNDROME_WR_FLUSH_ERR status.
>>> + * The send queue is supposed to be empty.
>>> */
>>> ++ci;
>>> + txq->cq_pi = ci;
>>> + last_cqe = NULL;
>>> continue;
>>> }
>>> /* Normal transmit completion. */
>>> + assert(ci != txq->cq_pi);
>>> + assert((txq->fcqs[ci & txq->cqe_m] >> 16) == cqe-
>>> wqe_counter);
>>
>> And same comments on these as previous patches, we spend some effort to
>> remove the 'rte_panic' from drivers, this is almost same thing.
>>
>> I think a driver shouldn't decide to exit whole application, it's effect should be
>> limited to the driver.
>>
>> Assert is useful for debug and during development, but not sure having them
>> in the production code.
>
> IIRC, "assert" is standard C function. Compiled only if there is no NDEBUG defined.
> So, assert does exactly what you are saying - provide the debug break
> not allowing the bug to evolve. And no this break in production code.
>
Since mlx driver is using NDEBUG defined, what you said is right indeed. But why
not using RTE_ASSERT to be consistent with rest. There is a specific config
option to control assert (RTE_ENABLE_ASSERT) and anyone using it will get
different behavior with mlx5.
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, January 9, 2020 17:44
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>; Thomas
> Monjalon <thomas@monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on completion
> queue
>
> On 1/9/2020 3:27 PM, Slava Ovsiienko wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Thursday, January 9, 2020 17:19
> >> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> >> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> >> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>; Thomas
> Monjalon
> >> <thomas@monjalon.net>
> >> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on
> >> completion queue
> >>
> >> On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
> >>> The free on completion queue keeps the indices of elts array, all
> >>> mbuf stored below this index should be freed on arrival of normal
> >>> send completion. In debug version it also contains an index of
> >>> completed transmitting descriptor (WQE) to check queues
> synchronization.
> >>>
> >>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> >>> Acked-by: Matan Azrad <matan@mellanox.com>
> >>
> >> <...>
> >>
> >>> @@ -2108,17 +2108,18 @@ enum mlx5_txcmp_code {
> >>> /*
> >>> * We are going to fetch all entries with
> >>> * MLX5_CQE_SYNDROME_WR_FLUSH_ERR status.
> >>> + * The send queue is supposed to be empty.
> >>> */
> >>> ++ci;
> >>> + txq->cq_pi = ci;
> >>> + last_cqe = NULL;
> >>> continue;
> >>> }
> >>> /* Normal transmit completion. */
> >>> + assert(ci != txq->cq_pi);
> >>> + assert((txq->fcqs[ci & txq->cqe_m] >> 16) == cqe-
> >>> wqe_counter);
> >>
> >> And same comments on these as previous patches, we spend some effort
> >> to remove the 'rte_panic' from drivers, this is almost same thing.
> >>
> >> I think a driver shouldn't decide to exit whole application, it's
> >> effect should be limited to the driver.
> >>
> >> Assert is useful for debug and during development, but not sure
> >> having them in the production code.
> >
> > IIRC, "assert" is standard C function. Compiled only if there is no NDEBUG
> defined.
> > So, assert does exactly what you are saying - provide the debug break
> > not allowing the bug to evolve. And no this break in production code.
> >
>
> Since mlx driver is using NDEBUG defined, what you said is right indeed. But
> why not using RTE_ASSERT to be consistent with rest. There is a specific config
> option to control assert (RTE_ENABLE_ASSERT) and anyone using it will get
> different behavior with mlx5.
We have the dedicated option to control mlx5 debug:
CONFIG_RTE_ENABLE_ASSERT controls the whole DPDK.
CONFIG_RTE_LIBRTE_MLX5_DEBUG controls NDEBUG for mlx5
From my practice - I switch the mlx5 debug option (in the process of the debugging/testing
datapath and checking the resulting performance, by directly defining NDEBUG in mlx5.h and
not reconfiguring/rebuilding the entire DPDK), this fine grained option seems to be useful.
With best regards, Slava
09/01/2020 17:22, Slava Ovsiienko:
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > On 1/9/2020 3:27 PM, Slava Ovsiienko wrote:
> > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > >> On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
> > >>> + assert(ci != txq->cq_pi);
> > >>> + assert((txq->fcqs[ci & txq->cqe_m] >> 16) == cqe-
> > >>> wqe_counter);
> > >>
> > >> And same comments on these as previous patches, we spend some effort
> > >> to remove the 'rte_panic' from drivers, this is almost same thing.
> > >>
> > >> I think a driver shouldn't decide to exit whole application, it's
> > >> effect should be limited to the driver.
> > >>
> > >> Assert is useful for debug and during development, but not sure
> > >> having them in the production code.
> > >
> > > IIRC, "assert" is standard C function. Compiled only if there is no NDEBUG
> > defined.
> > > So, assert does exactly what you are saying - provide the debug break
> > > not allowing the bug to evolve. And no this break in production code.
> > >
> >
> > Since mlx driver is using NDEBUG defined, what you said is right indeed. But
> > why not using RTE_ASSERT to be consistent with rest. There is a specific config
> > option to control assert (RTE_ENABLE_ASSERT) and anyone using it will get
> > different behavior with mlx5.
>
> We have the dedicated option to control mlx5 debug:
> CONFIG_RTE_ENABLE_ASSERT controls the whole DPDK.
No, it controls the whole DPDK except mlx PMDs.
> CONFIG_RTE_LIBRTE_MLX5_DEBUG controls NDEBUG for mlx5
>
> From my practice - I switch the mlx5 debug option (in the process of the debugging/testing
> datapath and checking the resulting performance, by directly defining NDEBUG in mlx5.h and
> not reconfiguring/rebuilding the entire DPDK), this fine grained option seems to be useful.
I don't like having mlx PMDs behave differently.
It make things difficult for newcomers.
And with meson, such options are cleaned up.
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, January 10, 2020 11:07
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Matan Azrad
> <matan@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>; Ori
> Kam <orika@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on completion
> queue
>
> 09/01/2020 17:22, Slava Ovsiienko:
> > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > On 1/9/2020 3:27 PM, Slava Ovsiienko wrote:
> > > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > >> On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
> > > >>> + assert(ci != txq->cq_pi);
> > > >>> + assert((txq->fcqs[ci & txq->cqe_m] >> 16) == cqe-
> > > >>> wqe_counter);
> > > >>
> > > >> And same comments on these as previous patches, we spend some
> > > >> effort to remove the 'rte_panic' from drivers, this is almost same thing.
> > > >>
> > > >> I think a driver shouldn't decide to exit whole application, it's
> > > >> effect should be limited to the driver.
> > > >>
> > > >> Assert is useful for debug and during development, but not sure
> > > >> having them in the production code.
> > > >
> > > > IIRC, "assert" is standard C function. Compiled only if there is
> > > > no NDEBUG
> > > defined.
> > > > So, assert does exactly what you are saying - provide the debug
> > > > break not allowing the bug to evolve. And no this break in production
> code.
> > > >
> > >
> > > Since mlx driver is using NDEBUG defined, what you said is right
> > > indeed. But why not using RTE_ASSERT to be consistent with rest.
> > > There is a specific config option to control assert
> > > (RTE_ENABLE_ASSERT) and anyone using it will get different behavior with
> mlx5.
> >
> > We have the dedicated option to control mlx5 debug:
> > CONFIG_RTE_ENABLE_ASSERT controls the whole DPDK.
>
> No, it controls the whole DPDK except mlx PMDs.
>
> > CONFIG_RTE_LIBRTE_MLX5_DEBUG controls NDEBUG for mlx5
> >
> > From my practice - I switch the mlx5 debug option (in the process of
> > the debugging/testing datapath and checking the resulting performance,
> > by directly defining NDEBUG in mlx5.h and not reconfiguring/rebuilding the
> entire DPDK), this fine grained option seems to be useful.
>
> I don't like having mlx PMDs behave differently.
> It make things difficult for newcomers.
> And with meson, such options are cleaned up.
Do you mean we should eliminate NDEBUG usage and convert it to some explicit "MLX5_NDEBUG"
(and convert "assert" to "MLX5_ASSERT") ?
With best regards, Slava
10/01/2020 10:28, Slava Ovsiienko:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 09/01/2020 17:22, Slava Ovsiienko:
> > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > On 1/9/2020 3:27 PM, Slava Ovsiienko wrote:
> > > > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > >> On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
> > > > >>> + assert(ci != txq->cq_pi);
> > > > >>> + assert((txq->fcqs[ci & txq->cqe_m] >> 16) == cqe-
> > > > >>> wqe_counter);
> > > > >>
> > > > >> And same comments on these as previous patches, we spend some
> > > > >> effort to remove the 'rte_panic' from drivers, this is almost same thing.
> > > > >>
> > > > >> I think a driver shouldn't decide to exit whole application, it's
> > > > >> effect should be limited to the driver.
> > > > >>
> > > > >> Assert is useful for debug and during development, but not sure
> > > > >> having them in the production code.
> > > > >
> > > > > IIRC, "assert" is standard C function. Compiled only if there is
> > > > > no NDEBUG
> > > > defined.
> > > > > So, assert does exactly what you are saying - provide the debug
> > > > > break not allowing the bug to evolve. And no this break in production
> > code.
> > > > >
> > > >
> > > > Since mlx driver is using NDEBUG defined, what you said is right
> > > > indeed. But why not using RTE_ASSERT to be consistent with rest.
> > > > There is a specific config option to control assert
> > > > (RTE_ENABLE_ASSERT) and anyone using it will get different behavior with
> > mlx5.
> > >
> > > We have the dedicated option to control mlx5 debug:
> > > CONFIG_RTE_ENABLE_ASSERT controls the whole DPDK.
> >
> > No, it controls the whole DPDK except mlx PMDs.
> >
> > > CONFIG_RTE_LIBRTE_MLX5_DEBUG controls NDEBUG for mlx5
> > >
> > > From my practice - I switch the mlx5 debug option (in the process of
> > > the debugging/testing datapath and checking the resulting performance,
> > > by directly defining NDEBUG in mlx5.h and not reconfiguring/rebuilding the
> > entire DPDK), this fine grained option seems to be useful.
> >
> > I don't like having mlx PMDs behave differently.
> > It make things difficult for newcomers.
> > And with meson, such options are cleaned up.
>
> Do you mean we should eliminate NDEBUG usage and convert it to some explicit "MLX5_NDEBUG"
> (and convert "assert" to "MLX5_ASSERT") ?
I mean we should use RTE_ASSERT in mlx5, as it is already done in some files.
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, January 10, 2020 11:34
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Matan Azrad
> <matan@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>; Ori
> Kam <orika@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on completion
> queue
>
> 10/01/2020 10:28, Slava Ovsiienko:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 09/01/2020 17:22, Slava Ovsiienko:
> > > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > > On 1/9/2020 3:27 PM, Slava Ovsiienko wrote:
> > > > > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > > >> On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
> > > > > >>> + assert(ci != txq->cq_pi);
> > > > > >>> + assert((txq->fcqs[ci & txq->cqe_m] >> 16) == cqe-
> > > > > >>> wqe_counter);
> > > > > >>
> > > > > >> And same comments on these as previous patches, we spend some
> > > > > >> effort to remove the 'rte_panic' from drivers, this is almost same
> thing.
> > > > > >>
> > > > > >> I think a driver shouldn't decide to exit whole application,
> > > > > >> it's effect should be limited to the driver.
> > > > > >>
> > > > > >> Assert is useful for debug and during development, but not
> > > > > >> sure having them in the production code.
> > > > > >
> > > > > > IIRC, "assert" is standard C function. Compiled only if there
> > > > > > is no NDEBUG
> > > > > defined.
> > > > > > So, assert does exactly what you are saying - provide the
> > > > > > debug break not allowing the bug to evolve. And no this break
> > > > > > in production
> > > code.
> > > > > >
> > > > >
> > > > > Since mlx driver is using NDEBUG defined, what you said is right
> > > > > indeed. But why not using RTE_ASSERT to be consistent with rest.
> > > > > There is a specific config option to control assert
> > > > > (RTE_ENABLE_ASSERT) and anyone using it will get different
> > > > > behavior with
> > > mlx5.
> > > >
> > > > We have the dedicated option to control mlx5 debug:
> > > > CONFIG_RTE_ENABLE_ASSERT controls the whole DPDK.
> > >
> > > No, it controls the whole DPDK except mlx PMDs.
> > >
> > > > CONFIG_RTE_LIBRTE_MLX5_DEBUG controls NDEBUG for mlx5
> > > >
> > > > From my practice - I switch the mlx5 debug option (in the process
> > > > of the debugging/testing datapath and checking the resulting
> > > > performance, by directly defining NDEBUG in mlx5.h and not
> > > > reconfiguring/rebuilding the
> > > entire DPDK), this fine grained option seems to be useful.
> > >
> > > I don't like having mlx PMDs behave differently.
> > > It make things difficult for newcomers.
> > > And with meson, such options are cleaned up.
> >
> > Do you mean we should eliminate NDEBUG usage and convert it to some
> explicit "MLX5_NDEBUG"
> > (and convert "assert" to "MLX5_ASSERT") ?
>
> I mean we should use RTE_ASSERT in mlx5, as it is already done in some files.
>
This would make not possible to engage asserts in mlx5 module only.
It is a question of structuring/layering, not "different behavior".
As for me - it is very nice to have fine grained debug control option,
and I use this feature actively, it just saves my time. Also, it seems
these options are implemented in many other PMDs
(with its own xxx_ASSERTs).
With best regards, Slava
10/01/2020 10:55, Slava Ovsiienko:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 10/01/2020 10:28, Slava Ovsiienko:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 09/01/2020 17:22, Slava Ovsiienko:
> > > > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > > > On 1/9/2020 3:27 PM, Slava Ovsiienko wrote:
> > > > > > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > > > >> On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
> > > > > > >>> + assert(ci != txq->cq_pi);
> > > > > > >>> + assert((txq->fcqs[ci & txq->cqe_m] >> 16) == cqe-
> > > > > > >>> wqe_counter);
> > > > > > >>
> > > > > > >> And same comments on these as previous patches, we spend some
> > > > > > >> effort to remove the 'rte_panic' from drivers, this is almost same
> > thing.
> > > > > > >>
> > > > > > >> I think a driver shouldn't decide to exit whole application,
> > > > > > >> it's effect should be limited to the driver.
> > > > > > >>
> > > > > > >> Assert is useful for debug and during development, but not
> > > > > > >> sure having them in the production code.
> > > > > > >
> > > > > > > IIRC, "assert" is standard C function. Compiled only if there
> > > > > > > is no NDEBUG
> > > > > > defined.
> > > > > > > So, assert does exactly what you are saying - provide the
> > > > > > > debug break not allowing the bug to evolve. And no this break
> > > > > > > in production
> > > > code.
> > > > > > >
> > > > > >
> > > > > > Since mlx driver is using NDEBUG defined, what you said is right
> > > > > > indeed. But why not using RTE_ASSERT to be consistent with rest.
> > > > > > There is a specific config option to control assert
> > > > > > (RTE_ENABLE_ASSERT) and anyone using it will get different
> > > > > > behavior with
> > > > mlx5.
> > > > >
> > > > > We have the dedicated option to control mlx5 debug:
> > > > > CONFIG_RTE_ENABLE_ASSERT controls the whole DPDK.
> > > >
> > > > No, it controls the whole DPDK except mlx PMDs.
> > > >
> > > > > CONFIG_RTE_LIBRTE_MLX5_DEBUG controls NDEBUG for mlx5
> > > > >
> > > > > From my practice - I switch the mlx5 debug option (in the process
> > > > > of the debugging/testing datapath and checking the resulting
> > > > > performance, by directly defining NDEBUG in mlx5.h and not
> > > > > reconfiguring/rebuilding the
> > > > entire DPDK), this fine grained option seems to be useful.
> > > >
> > > > I don't like having mlx PMDs behave differently.
> > > > It make things difficult for newcomers.
> > > > And with meson, such options are cleaned up.
> > >
> > > Do you mean we should eliminate NDEBUG usage and convert it to some
> > explicit "MLX5_NDEBUG"
> > > (and convert "assert" to "MLX5_ASSERT") ?
> >
> > I mean we should use RTE_ASSERT in mlx5, as it is already done in some files.
> >
> This would make not possible to engage asserts in mlx5 module only.
> It is a question of structuring/layering, not "different behavior".
> As for me - it is very nice to have fine grained debug control option,
> and I use this feature actively, it just saves my time. Also, it seems
> these options are implemented in many other PMDs
> (with its own xxx_ASSERTs).
I disagree, it is not nice. It makes it more complicate to use.
Can you imagine every file having its own tools and configs
in a project? As a maintainer, my role is to make things simpler
for everyone in general so we can know easily how things work.
About time saving, I also disagree. If you enable assert for the whole project
during all your development, it is a good practice which does not cost any time.
About other PMDs, they must be fixed.
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Friday, January 10, 2020 15:11
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org; Matan Azrad
> <matan@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>; Ori
> Kam <orika@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on completion
> queue
>
> 10/01/2020 10:55, Slava Ovsiienko:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 10/01/2020 10:28, Slava Ovsiienko:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 09/01/2020 17:22, Slava Ovsiienko:
> > > > > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > > > > On 1/9/2020 3:27 PM, Slava Ovsiienko wrote:
> > > > > > > > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > > > > >> On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
> > > > > > > >>> + assert(ci != txq->cq_pi);
> > > > > > > >>> + assert((txq->fcqs[ci & txq->cqe_m] >> 16) ==
> cqe-
> > > > > > > >>> wqe_counter);
> > > > > > > >>
> > > > > > > >> And same comments on these as previous patches, we spend
> > > > > > > >> some effort to remove the 'rte_panic' from drivers, this
> > > > > > > >> is almost same
> > > thing.
> > > > > > > >>
> > > > > > > >> I think a driver shouldn't decide to exit whole
> > > > > > > >> application, it's effect should be limited to the driver.
> > > > > > > >>
> > > > > > > >> Assert is useful for debug and during development, but
> > > > > > > >> not sure having them in the production code.
> > > > > > > >
> > > > > > > > IIRC, "assert" is standard C function. Compiled only if
> > > > > > > > there is no NDEBUG
> > > > > > > defined.
> > > > > > > > So, assert does exactly what you are saying - provide the
> > > > > > > > debug break not allowing the bug to evolve. And no this
> > > > > > > > break in production
> > > > > code.
> > > > > > > >
> > > > > > >
> > > > > > > Since mlx driver is using NDEBUG defined, what you said is
> > > > > > > right indeed. But why not using RTE_ASSERT to be consistent with
> rest.
> > > > > > > There is a specific config option to control assert
> > > > > > > (RTE_ENABLE_ASSERT) and anyone using it will get different
> > > > > > > behavior with
> > > > > mlx5.
> > > > > >
> > > > > > We have the dedicated option to control mlx5 debug:
> > > > > > CONFIG_RTE_ENABLE_ASSERT controls the whole DPDK.
> > > > >
> > > > > No, it controls the whole DPDK except mlx PMDs.
> > > > >
> > > > > > CONFIG_RTE_LIBRTE_MLX5_DEBUG controls NDEBUG for mlx5
> > > > > >
> > > > > > From my practice - I switch the mlx5 debug option (in the
> > > > > > process of the debugging/testing datapath and checking the
> > > > > > resulting performance, by directly defining NDEBUG in mlx5.h
> > > > > > and not reconfiguring/rebuilding the
> > > > > entire DPDK), this fine grained option seems to be useful.
> > > > >
> > > > > I don't like having mlx PMDs behave differently.
> > > > > It make things difficult for newcomers.
> > > > > And with meson, such options are cleaned up.
> > > >
> > > > Do you mean we should eliminate NDEBUG usage and convert it to
> > > > some
> > > explicit "MLX5_NDEBUG"
> > > > (and convert "assert" to "MLX5_ASSERT") ?
> > >
> > > I mean we should use RTE_ASSERT in mlx5, as it is already done in some
> files.
> > >
> > This would make not possible to engage asserts in mlx5 module only.
> > It is a question of structuring/layering, not "different behavior".
> > As for me - it is very nice to have fine grained debug control option,
> > and I use this feature actively, it just saves my time. Also, it seems
> > these options are implemented in many other PMDs (with its own
> > xxx_ASSERTs).
>
> I disagree, it is not nice. It makes it more complicate to use.
> Can you imagine every file having its own tools and configs in a project? As a
> maintainer, my role is to make things simpler for everyone in general so we
> can know easily how things work.
Not every file, but every module. It is quite common practice to have local
configuration options for module in the large projects. So, it is native for
PMDs to have its dedicated configuration options. And what we have
currently follows this approach.
>
> About time saving, I also disagree. If you enable assert for the whole project
> during all your development, it is a good practice which does not cost any
> time.
During the day I might switch multiple times between debug (with assert enabled)
and performance check (debug/assert disabled) modes. Now I can switch it easily and quickly,
just commenting [out] NDEBUG in mlx5.h. In the large projects the global configuration changing price
is getting higher. You just propose to pay this price ☹ - I would have to reconfig/recompile
the entire DPDK. The same might concern the other PMDs - many of them have the private
debug option(s).
>
> About other PMDs, they must be fixed.
I suppose the developers of other PMDs use the module debug options either :)
>
I agree the NDEBUG is "different behavior", we should think how to eliminate it.
But I do not understand why we should drop the partial configuration, the feature that is actively used.
Now code is split into several debug domains (at least for assert), we can control each domain in independent
fashion, and the practice shows it is useful and saves developer's time. DPDK is the large project definitely,
it contains a lot of modules, we can't address all development needs with one simple common configuration
option.
Having the module/private debug options does not eliminate the introducing the global control -
say "RTE_CONFIG_ENABLE_MODULE_DEBUG_OPTIONS" to provide production code generation
"in one click", but just dropping the module debug options is not nice as for me.
With best regards, Slava
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, January 9, 2020 17:44
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>; Thomas
> Monjalon <thomas@monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on completion
> queue
>
> On 1/9/2020 3:27 PM, Slava Ovsiienko wrote:
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> >> Sent: Thursday, January 9, 2020 17:19
> >> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> >> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> >> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>; Thomas
> Monjalon
> >> <thomas@monjalon.net>
> >> Subject: Re: [dpdk-dev] [PATCH v2 4/4] net/mlx5: engage free on
> >> completion queue
> >>
> >> On 1/9/2020 10:56 AM, Viacheslav Ovsiienko wrote:
> >>> The free on completion queue keeps the indices of elts array, all
> >>> mbuf stored below this index should be freed on arrival of normal
> >>> send completion. In debug version it also contains an index of
> >>> completed transmitting descriptor (WQE) to check queues
> synchronization.
> >>>
> >>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> >>> Acked-by: Matan Azrad <matan@mellanox.com>
> >>
> >> <...>
> >>
> >>> @@ -2108,17 +2108,18 @@ enum mlx5_txcmp_code {
> >>> /*
> >>> * We are going to fetch all entries with
> >>> * MLX5_CQE_SYNDROME_WR_FLUSH_ERR status.
> >>> + * The send queue is supposed to be empty.
> >>> */
> >>> ++ci;
> >>> + txq->cq_pi = ci;
> >>> + last_cqe = NULL;
> >>> continue;
> >>> }
> >>> /* Normal transmit completion. */
> >>> + assert(ci != txq->cq_pi);
> >>> + assert((txq->fcqs[ci & txq->cqe_m] >> 16) == cqe-
> >>> wqe_counter);
> >>
> >> And same comments on these as previous patches, we spend some effort
> >> to remove the 'rte_panic' from drivers, this is almost same thing.
> >>
> >> I think a driver shouldn't decide to exit whole application, it's
> >> effect should be limited to the driver.
> >>
> >> Assert is useful for debug and during development, but not sure
> >> having them in the production code.
> >
> > IIRC, "assert" is standard C function. Compiled only if there is no NDEBUG
> defined.
> > So, assert does exactly what you are saying - provide the debug break
> > not allowing the bug to evolve. And no this break in production code.
> >
>
> Since mlx driver is using NDEBUG defined, what you said is right indeed. But
> why not using RTE_ASSERT to be consistent with rest. There is a specific config
> option to control assert (RTE_ENABLE_ASSERT) and anyone using it will get
> different behavior with mlx5.
OK, to summarize (after discussions with Ferruh and Thomas):
1. assert/NDEBUG in mlx5 should be fixed
2 in mlx5 assert should be replaced by MLX5_ASSERT macro, it provides "in-development" assert control if needed
3. mlx5 PMDs local debug configuration option will be removed from global build configuration
(it is absent in meson build now, and as makefile is subject to drop - this option will not be supported anymore)
4. The patch in subject ("net/mlx5: engage free on completion queue") is not related to bullets 1-3 and
should be accepted to Upstream, and the is the claimed commitment from my side to provide
the dedicated patch for 1-3.
With best regards, Slava
@@ -2043,8 +2043,7 @@ enum mlx5_txcmp_code {
uint16_t tail;
txq->wqe_pi = rte_be_to_cpu_16(last_cqe->wqe_counter);
- tail = ((volatile struct mlx5_wqe_cseg *)
- (txq->wqes + (txq->wqe_pi & txq->wqe_m)))->misc;
+ tail = txq->fcqs[(txq->cq_ci - 1) & txq->cqe_m];
if (likely(tail != txq->elts_tail)) {
mlx5_tx_free_elts(txq, tail, olx);
assert(tail == txq->elts_tail);
@@ -2095,6 +2094,7 @@ enum mlx5_txcmp_code {
* here, before we might perform SQ reset.
*/
rte_wmb();
+ txq->cq_ci = ci;
ret = mlx5_tx_error_cqe_handle
(txq, (volatile struct mlx5_err_cqe *)cqe);
if (unlikely(ret < 0)) {
@@ -2108,17 +2108,18 @@ enum mlx5_txcmp_code {
/*
* We are going to fetch all entries with
* MLX5_CQE_SYNDROME_WR_FLUSH_ERR status.
+ * The send queue is supposed to be empty.
*/
++ci;
+ txq->cq_pi = ci;
+ last_cqe = NULL;
continue;
}
/* Normal transmit completion. */
+ assert(ci != txq->cq_pi);
+ assert((txq->fcqs[ci & txq->cqe_m] >> 16) == cqe->wqe_counter);
++ci;
last_cqe = cqe;
-#ifndef NDEBUG
- if (txq->cq_pi)
- --txq->cq_pi;
-#endif
/*
* We have to restrict the amount of processed CQEs
* in one tx_burst routine call. The CQ may be large
@@ -2127,7 +2128,7 @@ enum mlx5_txcmp_code {
* multiple iterations may introduce significant
* latency.
*/
- if (--count == 0)
+ if (likely(--count == 0))
break;
} while (true);
if (likely(ci != txq->cq_ci)) {
@@ -2177,15 +2178,15 @@ enum mlx5_txcmp_code {
/* Request unconditional completion on last WQE. */
last->cseg.flags = RTE_BE32(MLX5_COMP_ALWAYS <<
MLX5_COMP_MODE_OFFSET);
- /* Save elts_head in unused "immediate" field of WQE. */
- last->cseg.misc = head;
- /*
- * A CQE slot must always be available. Count the
- * issued CEQ "always" request instead of production
- * index due to here can be CQE with errors and
- * difference with ci may become inconsistent.
- */
- assert(txq->cqe_s > ++txq->cq_pi);
+ /* Save elts_head in dedicated free on completion queue. */
+#ifdef NDEBUG
+ txq->fcqs[txq->cq_pi++ & txq->cqe_m] = head;
+#else
+ txq->fcqs[txq->cq_pi++ & txq->cqe_m] = head |
+ (last->cseg.opcode >> 8) << 16;
+#endif
+ /* A CQE slot must always be available. */
+ assert((txq->cq_pi - txq->cq_ci) <= txq->cqe_s);
}
}
@@ -273,9 +273,7 @@ struct mlx5_txq_data {
uint16_t wqe_thres; /* WQE threshold to request completion in CQ. */
/* WQ related fields. */
uint16_t cq_ci; /* Consumer index for completion queue. */
-#ifndef NDEBUG
- uint16_t cq_pi; /* Counter of issued CQE "always" requests. */
-#endif
+ uint16_t cq_pi; /* Production index for completion queue. */
uint16_t cqe_s; /* Number of CQ elements. */
uint16_t cqe_m; /* Mask for CQ indices. */
/* CQ related fields. */
@@ -298,7 +296,7 @@ struct mlx5_txq_data {
struct mlx5_wqe *wqes; /* Work queue. */
struct mlx5_wqe *wqes_end; /* Work queue array limit. */
#ifdef NDEBUG
- uint32_t *fcqs; /* Free completion queue. */
+ uint16_t *fcqs; /* Free completion queue. */
#else
uint32_t *fcqs; /* Free completion queue (debug extended). */
#endif
@@ -717,9 +717,7 @@ struct mlx5_txq_obj *
txq_data->cq_db = cq_info.dbrec;
txq_data->cqes = (volatile struct mlx5_cqe *)cq_info.buf;
txq_data->cq_ci = 0;
-#ifndef NDEBUG
txq_data->cq_pi = 0;
-#endif
txq_data->wqe_ci = 0;
txq_data->wqe_pi = 0;
txq_data->wqe_comp = 0;