[ovs-dev] [OVN Patch v10 1/4] ovn-libs: Add support for parallel processing

[Ilya Maximets]

On 1/11/21 10:51 AM, anton.ivanov at cambridgegreys.com wrote:
> From: Anton Ivanov <anton.ivanov at cambridgegreys.com>
> 
> This adds a set of functions and macros intended to process
> hashes in parallel.
> 
> The principles of operation are documented in the fasthmap.h
> 
> If these one day go into the OVS tree, the OVS tree versions
> would be used in preference.
> 
> Signed-off-by: Anton Ivanov <anton.ivanov at cambridgegreys.com>

Hi.

Not a full review.  Just a couple of first glance comments inline.

Best regards, Ilya Maximets.


> ---
>  lib/automake.mk |   2 +
>  lib/fasthmap.c  | 281 ++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/fasthmap.h  | 206 +++++++++++++++++++++++++++++++++++
>  3 files changed, 489 insertions(+)
>  create mode 100644 lib/fasthmap.c
>  create mode 100644 lib/fasthmap.h
> 
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 250c7aefa..d7e4b20cf 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -13,6 +13,8 @@ lib_libovn_la_SOURCES = \
>  	lib/expr.c \
>  	lib/extend-table.h \
>  	lib/extend-table.c \
> +	lib/fasthmap.h \
> +	lib/fasthmap.c \
>  	lib/ip-mcast-index.c \
>  	lib/ip-mcast-index.h \
>  	lib/mcast-group-index.c \
> diff --git a/lib/fasthmap.c b/lib/fasthmap.c
> new file mode 100644
> index 000000000..3096c90d3
> --- /dev/null
> +++ b/lib/fasthmap.c
> @@ -0,0 +1,281 @@
> +/*
> + * Copyright (c) 2020 Red Hat, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2012, 2013, 2015, 2019 Nicira, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <semaphore.h>
> +#include "fatal-signal.h"
> +#include "util.h"
> +#include "openvswitch/vlog.h"
> +#include "openvswitch/hmap.h"
> +#include "openvswitch/thread.h"
> +#include "fasthmap.h"
> +#include "ovs-atomic.h"
> +#include "ovs-thread.h"
> +#include "ovs-numa.h"
> +
> +VLOG_DEFINE_THIS_MODULE(fasthmap);
> +
> +
> +static bool worker_pool_setup = false;
> +static bool workers_must_exit = false;

This must be atomic as it constantly used and updated frm different
threads without the lock.

> +static bool can_parallelize = false;
> +
> +static struct ovs_list worker_pools = OVS_LIST_INITIALIZER(&worker_pools);
> +
> +static struct ovs_mutex init_mutex = OVS_MUTEX_INITIALIZER;
> +
> +static int pool_size;
> +
> +static void worker_pool_hook(void *aux OVS_UNUSED) {
> +    int i;
> +    static struct worker_pool *pool;
> +    workers_must_exit = true; /* all workers must honour this flag */
> +    atomic_thread_fence(memory_order_release);

What is the purpose of these thread fences here and there?
All of them has 'release' memory ordering semantics, but there
is no any code that uses 'acquire'.  If you're trying to ensure
ordering within current thread, this must be documented.
Basically, please, add comments to all thread fences you're using.
It's not trivial to figure out why they are there.