Re: [dpdk-dev] [PATCH] eal: fix mem alloc from control thread if socket 0 is unused

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

On Fri, Oct 29, 2021 at 11:49 AM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> From: Ilyes Ben Hamouda <ilyes.ben_hamouda@6wind.com>
>
> When using rte_malloc() from a control thread, the used heap is the one
> from numa socket 0, which may not have available memory.
>
> Fix this by selecting the first socket which has available memory.
>
> Note: malloc_get_numa_socket() is only used from one .c file, so move
> it there, and remove the inline keyword.
>
> Fixes: b94580d6887e ("malloc: avoid unknown socket id")
> Cc: stable@dpdk.org
>
> Signed-off-by: Ilyes Ben Hamouda <ilyes.ben_hamouda@6wind.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  lib/eal/common/malloc_heap.c | 20 ++++++++++++++++++++
>  lib/eal/common/malloc_heap.h | 11 -----------
>  2 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/lib/eal/common/malloc_heap.c b/lib/eal/common/malloc_heap.c
> index ee400f38ec..6eff9a2284 100644
> --- a/lib/eal/common/malloc_heap.c
> +++ b/lib/eal/common/malloc_heap.c
> @@ -694,6 +694,26 @@ malloc_heap_alloc_on_heap_id(const char *type, size_t size,
>         return ret;
>  }
>
> +static unsigned int
> +malloc_get_numa_socket(void)
> +{
> +       const struct internal_config *conf = eal_get_internal_configuration();
> +       unsigned int socket_id = rte_socket_id();
> +       unsigned int idx;
> +
> +       if (socket_id != (unsigned int)SOCKET_ID_ANY)
> +               return socket_id;
> +

Strictly speaking, this affects more than control threads.

socket_id == SOCKET_ID_ANY is possible if current thread is started on
cores from 2 different sockets.
See thread_update_affinity(), which calls eal_cpuset_socket_id().

As a consequence, this change affects lcores too like if a lcore is
pinned on cores from two sockets like --lcores 0@(1,2) with core 1 on
numa1 and core 2 on numa2 (giving this odd example on purpose).
Previously, all allocations from this lcore would end up on numa0,
regardless of memory availability.
So this change fixes allocations for this odd setup too.


> +       /* for control threads, return first socket where memory is available */
> +       for (idx = 0; idx < rte_socket_count(); idx++) {
> +               socket_id = rte_socket_id_by_idx(idx);
> +               if (conf->socket_mem[socket_id] != 0)
> +                       return socket_id;
> +       }

We could look at current thread cpu affinity to check to which sockets
it is bound to (like what is done in eal_cpuset_socket_id()).
But that would make the code rather complex and the setups in which it
helps are surely even even more odd than what I mentionned above.

Your proposed heuristic looks fine to me, let's go with it if nobody objects.


> +
> +       return rte_socket_id_by_idx(0);
> +}
> +
>  void *
>  malloc_heap_alloc(const char *type, size_t size, int socket_arg,
>                 unsigned int flags, size_t align, size_t bound, bool contig)
> diff --git a/lib/eal/common/malloc_heap.h b/lib/eal/common/malloc_heap.h
> index 3a6ec6ecf0..3a29d024b4 100644
> --- a/lib/eal/common/malloc_heap.h
> +++ b/lib/eal/common/malloc_heap.h
> @@ -33,17 +33,6 @@ struct malloc_heap {
>         char name[RTE_HEAP_NAME_MAX_LEN];
>  } __rte_cache_aligned;
>
> -static inline unsigned
> -malloc_get_numa_socket(void)
> -{
> -       unsigned socket_id = rte_socket_id();
> -
> -       if (socket_id == (unsigned)SOCKET_ID_ANY)
> -               return 0;
> -
> -       return socket_id;
> -}
> -
>  void *
>  malloc_heap_alloc(const char *type, size_t size, int socket, unsigned int flags,
>                 size_t align, size_t bound, bool contig);
> --
> 2.30.2
>


-- 
David Marchand