Re: [dpdk-dev] [PATCH] net/sfc: allow control threads for counter queue polling

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 10/21/21 11:28 PM, David Marchand wrote:
> On Thu, Oct 21, 2021 at 9:04 AM Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru> wrote:
>>
>> From: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>>
>> MAE counters can be polled from a control thread if no service core is
>> allocated for this.
>>
>> Signed-off-by: Viacheslav Galaktionov <viacheslav.galaktionov@oktetlabs.ru>
>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>> ---
>> The problem to require service cores for HW offload was raised by
>> David on review in 21.08 release cycle.
> 
> Thanks for following up!
> 
> 
>>
>>  doc/guides/rel_notes/release_21_11.rst |   1 +
>>  drivers/net/sfc/sfc_mae.h              |  26 +++++-
>>  drivers/net/sfc/sfc_mae_counter.c      | 120 ++++++++++++++++++++-----
>>  3 files changed, 123 insertions(+), 24 deletions(-)
>>
>> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
>> index 041383ee2a..9517e0fb0a 100644
>> --- a/doc/guides/rel_notes/release_21_11.rst
>> +++ b/doc/guides/rel_notes/release_21_11.rst
>> @@ -158,6 +158,7 @@ New Features
>>
>>    * Added port representors support on SN1000 SmartNICs
>>    * Added flow API transfer proxy support
>> +  * Added support for flow counters without service cores
>>
>>  * **Updated Marvell cnxk crypto PMD.**
>>
>> diff --git a/drivers/net/sfc/sfc_mae.h b/drivers/net/sfc/sfc_mae.h
>> index 23dcf1e482..45a2fdc3bb 100644
>> --- a/drivers/net/sfc/sfc_mae.h
>> +++ b/drivers/net/sfc/sfc_mae.h
>> @@ -127,6 +127,13 @@ struct sfc_mae_counters {
>>         unsigned int                    n_mae_counters;
>>  };
>>
>> +/** Options for MAE counter polling mode */
>> +enum sfc_mae_counter_polling_mode {
>> +       SFC_MAE_COUNTER_POLLING_OFF = 0,
>> +       SFC_MAE_COUNTER_POLLING_SERVICE,
>> +       SFC_MAE_COUNTER_POLLING_THREAD,
>> +};
>> +
>>  struct sfc_mae_counter_registry {
>>         /* Common counter information */
>>         /** Counters collection */
>> @@ -143,10 +150,21 @@ struct sfc_mae_counter_registry {
>>         bool                            use_credits;
>>
>>         /* Information used by configuration routines */
>> -       /** Counter service core ID */
>> -       uint32_t                        service_core_id;
>> -       /** Counter service ID */
>> -       uint32_t                        service_id;
>> +       enum sfc_mae_counter_polling_mode polling_mode;
>> +       union {
>> +               struct {
>> +                       /** Counter service core ID */
>> +                       uint32_t                        core_id;
>> +                       /** Counter service ID */
>> +                       uint32_t                        id;
>> +               } service;
>> +               struct {
>> +                       /** Counter thread ID */
>> +                       pthread_t                       id;
>> +                       /** The thread should keep running */
>> +                       volatile bool                   run;
> 
> volatile is probably unneeded.

Yes, volatile is definitely unnecessary here.
Will remove in v2.

>> +               } thread;
>> +       } polling;
>>  };
>>
>>  /**
>> diff --git a/drivers/net/sfc/sfc_mae_counter.c b/drivers/net/sfc/sfc_mae_counter.c
>> index 418caffe59..5f2aea1bf4 100644
>> --- a/drivers/net/sfc/sfc_mae_counter.c
>> +++ b/drivers/net/sfc/sfc_mae_counter.c
>> @@ -45,9 +45,6 @@ sfc_mae_counter_rxq_required(struct sfc_adapter *sa)
>>         if (encp->enc_mae_supported == B_FALSE)
>>                 return false;
>>
>> -       if (sfc_mae_counter_get_service_lcore(sa) == RTE_MAX_LCORE)
>> -               return false;
>> -
>>         return true;
>>  }
>>
>> @@ -402,6 +399,23 @@ sfc_mae_counter_routine(void *arg)
>>         return 0;
>>  }
>>
>> +static void *
>> +sfc_mae_counter_thread(void *data)
>> +{
>> +       struct sfc_adapter *sa = data;
>> +       struct sfc_mae_counter_registry *counter_registry =
>> +               &sa->mae.counter_registry;
>> +
>> +       /*
>> +        * Check run condition without atomic since it is not a problem
>> +        * if we run a bit more before we notice stop request
>> +        */
> 
> I find it clearer when we have clear pairs of atomic acquire
> load/release store (maybe because I feel like I understand something
> :-)).
> So it may not be a problem, but is there a reason to avoid this
> (acquire) atomic load?

Atomic in a busy polling loop could affect overall system
performance. That's why we avoid it here.
However, maybe better solution is to avoid busy polling -
just sleep a bit if previous poll is empty.
Will address it in v2 in one or another way.

> Otherwise, patch lgtm.
> Thanks.

Many thanks for review,
Andrew.