On Wed, Jul 7, 2021 at 3:15 AM Fabrizio D'Angelo <fdang...@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 <fdang...@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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
