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

[David Marchand <david.marchand@redhat.com>]

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.


> +               } 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?


Otherwise, patch lgtm.
Thanks.


-- 
David Marchand