[ovs-dev] northd: Add config option to specify # of threads

Uses northd database to specify number of threads that should be used
when lflow parallel computation is enabled.

Example:
    ovn-nbctl set NB_Global . options:num_parallel_threads=16

Reported at:
    https://bugzilla.redhat.com/show_bug.cgi?id=1975345

Signed-off-by: Fabrizio D'Angelo <fdangelo@redhat.com>
---
 lib/ovn-parallel-hmap.c | 12 ++++++------
 lib/ovn-parallel-hmap.h |  5 +++--
 northd/ovn-northd.c     |  7 ++++++-
 ovn-nb.xml              | 10 ++++++++++
 4 files changed, 25 insertions(+), 9 deletions(-)

Bleep bloop.  Greetings Fabrizio D'Angelo, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 81 characters long (recommended limit is 79)
#98 FILE: lib/ovn-parallel-hmap.h:269:
#define add_worker_pool(start, thread_num) ovn_add_worker_pool(start, thread_num)

Lines checked: 157, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

On Wed, Jul 7, 2021 at 3:15 AM Fabrizio D'Angelo <fdangelo@redhat.com> wrote:
>
> Uses northd database to specify number of threads that should be used
> when lflow parallel computation is enabled.
>
> Example:
>     ovn-nbctl set NB_Global . options:num_parallel_threads=16
>
> Reported at:
>     https://bugzilla.redhat.com/show_bug.cgi?id=1975345
>
> Signed-off-by: Fabrizio D'Angelo <fdangelo@redhat.com>

Hi Fabrizio,

Thanks for the patch.

I tested this patch and I don't think it is working as expected.
Mainly because of the way ovn-parallel-hmap.c
sets up the pools.

When ovn-northd is started, it calls ovn_can_parallelize_hashes()
first and this sets up the worker pools
by calling setup_worker_pools().

The function setup_worker_pools() is never called later because of
atomic_compare_exchange_strong()
present in ovn_can_parallelize_hashes() and ovn_add_worker_pool().

I think unfortunately it requires some fixes there so that when the
number of threads is configured later, it takes
into effect.  Right now it doesn't.

I think ovn_can_parallelize_hashes() should not try to setup the pool
size, instead it should check if ovn-northd
can do parallelization or not based on the ovs_numa_get_n_cores() and
ovs_numa_get_n_numas().
And if force is set, it should enable parallel processing.

Would you mind taking another look at the functions -
setup_worker_pools(), ovn_can_parallelize_hashes()
and ovn_add_worker_pool()  ?  So that we can enable or disable
parallelization dynamically
and also override the pool size with your newly added option -
num_parallel_threads.

Thanks
Numan

> ---
>  lib/ovn-parallel-hmap.c | 12 ++++++------
>  lib/ovn-parallel-hmap.h |  5 +++--
>  northd/ovn-northd.c     |  7 ++++++-
>  ovn-nb.xml              | 10 ++++++++++
>  4 files changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/lib/ovn-parallel-hmap.c b/lib/ovn-parallel-hmap.c
> index b8c7ac786..cae0b3110 100644
> --- a/lib/ovn-parallel-hmap.c
> +++ b/lib/ovn-parallel-hmap.c
> @@ -62,7 +62,7 @@ static int pool_size;
>  static int sembase;
>
>  static void worker_pool_hook(void *aux OVS_UNUSED);
> -static void setup_worker_pools(bool force);
> +static void setup_worker_pools(bool force, unsigned int thread_num);
>  static void merge_list_results(struct worker_pool *pool OVS_UNUSED,
>                                 void *fin_result, void *result_frags,
>                                 int index);
> @@ -86,14 +86,14 @@ ovn_can_parallelize_hashes(bool force_parallel)
>              &test,
>              true)) {
>          ovs_mutex_lock(&init_mutex);
> -        setup_worker_pools(force_parallel);
> +        setup_worker_pools(force_parallel, 0);
>          ovs_mutex_unlock(&init_mutex);
>      }
>      return can_parallelize;
>  }
>
>  struct worker_pool *
> -ovn_add_worker_pool(void *(*start)(void *))
> +ovn_add_worker_pool(void *(*start)(void *), unsigned int thread_num)

I'd suggest renaming the parameter from 'thread_num' to 'num_threads'.

Thanks
Numan

>  {
>      struct worker_pool *new_pool = NULL;
>      struct worker_control *new_control;
> @@ -109,7 +109,7 @@ ovn_add_worker_pool(void *(*start)(void *))
>              &test,
>              true)) {
>          ovs_mutex_lock(&init_mutex);
> -        setup_worker_pools(false);
> +        setup_worker_pools(false, thread_num);
>          ovs_mutex_unlock(&init_mutex);
>      }
>
> @@ -401,14 +401,14 @@ worker_pool_hook(void *aux OVS_UNUSED) {
>  }
>
>  static void
> -setup_worker_pools(bool force) {
> +setup_worker_pools(bool force, unsigned int thread_num) {
>      int cores, nodes;
>
>      nodes = ovs_numa_get_n_numas();
>      if (nodes == OVS_NUMA_UNSPEC || nodes <= 0) {
>          nodes = 1;
>      }
> -    cores = ovs_numa_get_n_cores();
> +    cores = thread_num ? thread_num : ovs_numa_get_n_cores();
>
>      /* If there is no NUMA config, use 4 cores.
>       * If there is NUMA config use half the cores on
> diff --git a/lib/ovn-parallel-hmap.h b/lib/ovn-parallel-hmap.h
> index 0af8914c4..9637a273d 100644
> --- a/lib/ovn-parallel-hmap.h
> +++ b/lib/ovn-parallel-hmap.h
> @@ -95,7 +95,8 @@ struct worker_pool {
>  /* Add a worker pool for thread function start() which expects a pointer to
>   * a worker_control structure as an argument. */
>
> -struct worker_pool *ovn_add_worker_pool(void *(*start)(void *));
> +struct worker_pool *ovn_add_worker_pool(void *(*start)(void *),
> +                                        unsigned int thread_num);
>
>  /* Setting this to true will make all processing threads exit */
>
> @@ -265,7 +266,7 @@ bool ovn_can_parallelize_hashes(bool force_parallel);
>
>  #define stop_parallel_processing() ovn_stop_parallel_processing()
>
> -#define add_worker_pool(start) ovn_add_worker_pool(start)
> +#define add_worker_pool(start, thread_num) ovn_add_worker_pool(start, thread_num)
>
>  #define fast_hmap_size_for(hmap, size) ovn_fast_hmap_size_for(hmap, size)
>
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 570c6a3ef..ffefac361 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -4153,6 +4153,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
>   * logical datapath only by creating a datapath group. */
>  static bool use_logical_dp_groups = false;
>  static bool use_parallel_build = true;
> +static unsigned int num_parallel_threads;
>
>  static struct hashrow_locks lflow_locks;
>
> @@ -12219,7 +12220,8 @@ init_lflows_thread_pool(void)
>      int index;
>
>      if (!pool_init_done) {
> -        struct worker_pool *pool = add_worker_pool(build_lflows_thread);
> +        struct worker_pool *pool = add_worker_pool(build_lflows_thread,
> +                                                   num_parallel_threads);
>          pool_init_done = true;
>          if (pool) {
>              build_lflows_pool = xmalloc(sizeof(*build_lflows_pool));
> @@ -13456,6 +13458,9 @@ ovnnb_db_run(struct northd_context *ctx,
>          (smap_get_bool(&nb->options, "use_parallel_build", false) &&
>           ovn_can_parallelize_hashes(false));
>
> +    num_parallel_threads =
> +        smap_get_uint(&nb->options, "num_parallel_threads", 0);
> +
>      use_logical_dp_groups = smap_get_bool(&nb->options,
>                                            "use_logical_dp_groups", false);
>      use_ct_inv_match = smap_get_bool(&nb->options,
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 36a77097c..d5bfb7ece 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -226,6 +226,16 @@
>            The default value is <code>false</code>.
>          </p>
>        </column>
> +      <column name="options" key="num_parallel_threads">
> +        <p>
> +          Manually specify the number of threads to be used for parallel
> +          logical flow computation.
> +        </p>
> +        <p>
> +          The number of threads utilized will be determined by the number
> +          of vCPUs on the system if this value is not specified.
> +        </p>
> +      </column>
>
>        <column name="options" key="ignore_lsp_down">
>          <p>
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>