[ovs-dev] [netdev-dpdk 1/5] ovs-numa: Add ovs-numa.{c, h} for extracting and storing cpu socket and cpu core info.

Thomas Graf graf at noironetworks.com
Mon Jun 23 22:40:36 UTC 2014


On 06/23/14 at 12:37pm, Alex Wang wrote:
> Signed-off-by: Alex Wang <alexw at nicira.com>

Some comments below

> ---
>  lib/automake.mk         |    2 +
>  lib/ovs-numa.c          |  208 +++++++++++++++++++++++++++++++++++++++++++++++
>  lib/ovs-numa.h          |   33 ++++++++
>  tests/ofproto-macros.at |    1 +
>  vswitchd/bridge.c       |    2 +
>  5 files changed, 246 insertions(+)
>  create mode 100644 lib/ovs-numa.c
>  create mode 100644 lib/ovs-numa.h
> 
> diff --git a/lib/automake.mk b/lib/automake.mk
> index 3f984d9..ca6c890 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -150,6 +150,8 @@ lib_libopenvswitch_la_SOURCES = \
>  	lib/ovs-atomic-locked.h \
>  	lib/ovs-atomic-pthreads.h \
>  	lib/ovs-atomic.h \
> +	lib/ovs-numa.c \
> +	lib/ovs-numa.h \
>  	lib/ovs-rcu.c \
>  	lib/ovs-rcu.h \
>  	lib/ovs-thread.c \
> diff --git a/lib/ovs-numa.c b/lib/ovs-numa.c
> new file mode 100644
> index 0000000..2bcef72
> --- /dev/null
> +++ b/lib/ovs-numa.c
> @@ -0,0 +1,208 @@
> +/*
> + * Copyright (c) 2014 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 "ovs-numa.h"
> +
> +#include <ctype.h>
> +#include <dirent.h>
> +#include <sys/types.h>
> +
> +#include "hash.h"
> +#include "hmap.h"
> +#include "list.h"
> +#include "ovs-thread.h"
> +#include "vlog.h"
> +
> +VLOG_DEFINE_THIS_MODULE(ovs_numa);
> +
> +#define MAX_CPU_SOCKETS 8

Can we bump this to something like 128? SPARC64 and the like have
been pretty crazy on the subject of number of sockets. It shouldn't
hurt as you break out of the loop anyway upon the first NULL 
eturn of opendir().

> +#define SYS_SOCKET_DIR "/sys/devices/system/node/node%d"
> +
> +/* Cpu socket. */
> +struct cpu_socket {
> +    struct hmap_node hmap_node;     /* In the 'all_cpu_sockets'. */
> +    struct list cores;         /* List of cpu cores on the socket. */
> +    uint32_t socket_id;        /* Socket id. */
> +};
> +
> +/* Cpu core on a cpu socket. */
> +struct cpu_core {
> +    struct hmap_node hmap_node;/* In the 'all_cpu_coress'. */
                                                         ^^

Minor typo above

> +    struct list list_node;     /* In 'cpu_socket->cores' list. */
> +    struct cpu_socket *socket; /* Socket containing the core. */
> +    uint32_t core_id;          /* Core id. */
> +    bool pinned;               /* If a thread has been pinned to the core. */
> +};
> +
> +/* Contains all 'struct cpu_socket's. */
> +static struct hmap all_cpu_sockets = HMAP_INITIALIZER(&all_cpu_sockets);
> +/* Contains all 'struct cpu_core's. */
> +static struct hmap all_cpu_cores = HMAP_INITIALIZER(&all_cpu_cores);
> +
> +/* Returns true if 'str' contains all digits.  Returns false otherwise. */
> +static bool
> +contain_all_digits(const char *str)
> +{
> +    size_t i = 0;
> +
> +    while (str[i] != '\0') {
> +        if (!isdigit(str[i++])) {
> +            return false;
> +        }
> +    }
> +
> +    return true;
> +}
> +
> +/* Discovers all cpu sockets and the corresponding cpu cores for each socket.
> + * Constructs the 'struct cpu_socket' and 'struct cpu_core'. */
> +static void
> +discover_sockets_and_cores(void)
> +{
> +    int n_cpus = 0;
> +    int i;
> +
> +    for (i = 0; i < MAX_CPU_SOCKETS; i++) {
> +        DIR *dir;
> +        char path[PATH_MAX];
> +        int path_len;
> +
> +        /* Constructs the path to node /sys/devices/system/nodeX. */
> +        path_len = snprintf(path, sizeof(path), SYS_SOCKET_DIR, i);
> +
> +        if (path_len <= 0 || path_len >= sizeof(path)) {
> +            VLOG_WARN("Path to cpu socket %d exceeds the length limit", i);
> +            break;
> +        }
> +
> +        dir = opendir(path);
> +
> +        /* Creates 'struct cpu_socket' if the 'dir' is non-null. */
> +        if (dir) {
> +            struct cpu_socket *s = xzalloc(sizeof *s);
> +            struct dirent *subdir;
> +            char *endptr = NULL;
> +
> +            hmap_insert(&all_cpu_sockets, &s->hmap_node, hash_int(i, 0));
> +            list_init(&s->cores);
> +            s->socket_id = i;
> +
> +            while ((subdir = readdir(dir)) != NULL) {

Maybe use readdir_r() just to be safe for the future? It would also
allow you to handle error conditions correctly.

> +                if (!strncmp(subdir->d_name, "cpu", 3)
> +                    && contain_all_digits(subdir->d_name + 3)){
> +                    struct cpu_core *c = xzalloc(sizeof *c);
> +                    uint32_t core_id;
> +
> +                    core_id = strtoul(subdir->d_name + 3, &endptr, 10);
> +                    hmap_insert(&all_cpu_cores, &c->hmap_node,
> +                                hash_int(core_id, 0));
> +                    list_insert(&s->cores, &c->list_node);
> +                    c->core_id = core_id;
> +                    n_cpus++;
> +                }
> +            }
> +        } else {
> +            break;
> +        }
> +	closedir(dir);
   ^^^^
Indent

> +    }
> +
> +    VLOG_INFO("Discovered %"PRIu64" CPU Sockets and %d CPU cores",
> +               hmap_count(&all_cpu_sockets), n_cpus);
> +}
> +
> +/* Extracts the numa node and core info from the 'sysfs'. */
> +void
> +ovs_numa_init(void)
> +{
> +    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +
> +    if (ovsthread_once_start(&once)) {
> +        discover_sockets_and_cores();
> +        ovsthread_once_done(&once);
> +    }
> +}
> +
> +/* Returns the number of cpu sockets. */
> +uint32_t
> +ovs_numa_get_n_sockets(void)
> +{
> +    return hmap_count(&all_cpu_sockets);
> +}
> +
> +/* Returns the number of cpu cores. */
> +uint32_t
> +ovs_numa_get_n_cores(void)
> +{
> +    return hmap_count(&all_cpu_cores);
> +}
> +
> +/* Searches through all cores for an unpinned core.  Returns the core_id
> + * if found and set the 'core->pinned' to true.  Otherwise, returns -1. */
> +int
> +ovs_numa_get_unpinned_core_any(void)

How about defining this as uint32_t to be consistent ...

> +{
> +    struct cpu_core *core;
> +
> +    HMAP_FOR_EACH(core, hmap_node, &all_cpu_cores) {
> +        if (!core->pinned) {
> +            core->pinned = true;
> +            return core->core_id;
> +        }
> +    }
> +
> +    return -1;

... and then

#define OVS_CORE_UNSPEC UINT32_MAX

respectively:

return OVS_CORE_UNSPEC?

> +}
> +
> +/* Searches through all cores on socket with 'socket_id' for an unpinned core.
> + * Returns the core_id if found and sets the 'core->pinned' to true.
> + * Otherwise, returns -1. */
> +int
> +ovs_numa_get_unpinned_core_on_socket(uint32_t socket_id)
> +{
> +    struct cpu_socket *socket;
> +    struct cpu_core *core;
> +
> +    CPU_SOCKET_ID_ASSERT(socket_id);
> +
> +    socket = CONTAINER_OF(hmap_first_with_hash(&all_cpu_sockets,
> +                                               hash_int(socket_id, 0)),
> +                          struct cpu_socket, hmap_node);
> +    LIST_FOR_EACH(core, list_node, &socket->cores) {
> +        if (!core->pinned) {
> +            core->pinned = true;
> +            return core->core_id;
> +        }
> +    }
> +
> +    return -1;

Same here



More information about the dev mailing list