From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7CBBCA034C; Sat, 9 Apr 2022 10:02:17 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 243FB4067E; Sat, 9 Apr 2022 10:02:17 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id CE0874067C for ; Sat, 9 Apr 2022 10:02:15 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id B99DC20444A8; Sat, 9 Apr 2022 01:02:14 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com B99DC20444A8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1649491334; bh=JXL6P2gaj0E7Rm5f4SAP+og53+40lepOG2yZpP5TtP0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=RlgQvDlsvJnE/1JT6W3ObTOknV4mRdVSbxbCzG0192/sETcY7AYpZDaJzEbbgWqc5 2OSfwNkqsSDJyU7h1JI9RCSX+4FJRDlm5lGAS9myAIg+1uttJoW/CtO4q2lnW5oTm7 7PG/fOeYrG0/NsNs16gA7S2hfFAmLXuyM5vAm7iI= Date: Sat, 9 Apr 2022 01:02:14 -0700 From: Tyler Retzlaff To: Dmitry Kozlyuk Cc: dev@dpdk.org, thomas@monjalon.net, anatoly.burakov@intel.com, Narcisa Vasile Subject: Re: [PATCH 2/3] eal: implement functions for get/set thread affinity Message-ID: <20220409080214.GA4153@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1648819793-18948-1-git-send-email-roretzla@linux.microsoft.com> <1648819793-18948-3-git-send-email-roretzla@linux.microsoft.com> <20220408170150.794bdbc4@sovereign> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220408170150.794bdbc4@sovereign> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Fri, Apr 08, 2022 at 05:01:50PM +0300, Dmitry Kozlyuk wrote: > 2022-04-01 06:29 (UTC-0700), Tyler Retzlaff: > > Implement functions for getting/setting thread affinity. > > Threads can be pinned to specific cores by setting their > > affinity attribute. > > > > Signed-off-by: Narcisa Vasile > > Signed-off-by: Tyler Retzlaff > > Acked-by: Dmitry Kozlyuk thanks, will address all items mentioned in v2 in addition to some minor issues detected with CI pipeline. > > Please see some small comments below. > > > --- > > lib/eal/include/rte_thread.h | 45 ++++++++++ > > lib/eal/unix/rte_thread.c | 16 ++++ > > lib/eal/version.map | 4 + > > lib/eal/windows/eal_lcore.c | 173 +++++++++++++++++++++++++++++---------- > > lib/eal/windows/eal_windows.h | 10 +++ > > lib/eal/windows/include/rte_os.h | 2 + > > lib/eal/windows/rte_thread.c | 131 ++++++++++++++++++++++++++++- > > 7 files changed, 336 insertions(+), 45 deletions(-) > > > > diff --git a/lib/eal/include/rte_thread.h b/lib/eal/include/rte_thread.h > > index 8be8ed8..4eb113f 100644 > > --- a/lib/eal/include/rte_thread.h > > +++ b/lib/eal/include/rte_thread.h > > @@ -2,6 +2,8 @@ > > * Copyright(c) 2021 Mellanox Technologies, Ltd > > */ > > > > +#include > > + > > #include > > #include > > > > @@ -21,6 +23,13 @@ > > #endif > > > > /** > > + * Thread id descriptor. > > + */ > > +typedef struct rte_thread_tag { > > + uintptr_t opaque_id; /**< thread identifier */ > > +} rte_thread_t; > > + > > +/** > > * TLS key type, an opaque pointer. > > */ > > typedef struct eal_tls_key *rte_thread_key; > > @@ -28,6 +37,42 @@ > > #ifdef RTE_HAS_CPUSET > > > > /** > > Missing a common part for experimental functions: > > * @warning > * @b EXPERIMENTAL: this API may change without prior notice. > > > + * Set the affinity of thread 'thread_id' to the cpu set > > + * specified by 'cpuset'. > > + * > > + * @param thread_id > > + * Id of the thread for which to set the affinity. > > + * > > + * @param cpuset > > + * Pointer to CPU affinity to set. > > + * > > + * @return > > + * On success, return 0. > > + * On failure, return a positive errno-style error number. > > + */ > > +__rte_experimental > > +int rte_thread_set_affinity_by_id(rte_thread_t thread_id, > > + const rte_cpuset_t *cpuset); > > + > > +/** > > Same here. > > > + * Get the affinity of thread 'thread_id' and store it > > + * in 'cpuset'. > > + * > > + * @param thread_id > > + * Id of the thread for which to get the affinity. > > + * > > + * @param cpuset > > + * Pointer for storing the affinity value. > > + * > > + * @return > > + * On success, return 0. > > + * On failure, return a positive errno-style error number. > > + */ > > +__rte_experimental > > +int rte_thread_get_affinity_by_id(rte_thread_t thread_id, > > + rte_cpuset_t *cpuset); > > + > > +/** > > * Set core affinity of the current thread. > > * Support both EAL and non-EAL thread and update TLS. > > * > > [...] > > > +static int > > +eal_query_group_affinity(void) > > +{ > > + SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX *infos = NULL; > > + unsigned int *cpu_count = &cpu_map.cpu_count; > > + DWORD infos_size = 0; > > + int ret = 0; > > + USHORT group_count; > > + KAFFINITY affinity; > > + USHORT group_no; > > + unsigned int i; > > + > > + if (!GetLogicalProcessorInformationEx(RelationGroup, NULL, > > + &infos_size)) { > > + DWORD error = GetLastError(); > > + if (error != ERROR_INSUFFICIENT_BUFFER) { > > + log_early("Cannot get group information size, " > > + "error %lu\n", error); > > Please don't break string constants for easy search. > > > + rte_errno = EINVAL; > > + ret = -1; > > + goto cleanup; > > + } > > + } > [...] > > > +static bool > > +eal_create_lcore_map(const SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX *info) > > +{ > > + const unsigned int node_id = info->NumaNode.NodeNumber; > > + const GROUP_AFFINITY *cores = &info->NumaNode.GroupMask; > > + struct lcore_map *lcore; > > + unsigned int socket_id; > > + unsigned int i; > > + > > + /* NUMA node may be reported multiple times if it includes > > + * cores from different processor groups, e. g. 80 cores > > + * of a physical processor comprise one NUMA node, but two > > + * processor groups, because group size is limited by 32/64. > > + */ > > + for (socket_id = 0; socket_id < cpu_map.socket_count; socket_id++) { > > + if (cpu_map.sockets[socket_id].node_id == node_id) > > + break; > > + } > > Nit: multi-line comments should start with a line containing just "/*", > and {} are no needed here. > > [...] > > +static int > > +rte_convert_cpuset_to_affinity(const rte_cpuset_t *cpuset, > > + PGROUP_AFFINITY affinity) > > +{ > > + int ret = 0; > > + PGROUP_AFFINITY cpu_affinity = NULL; > > + unsigned int cpu_idx; > > + > > + memset(affinity, 0, sizeof(GROUP_AFFINITY)); > > + affinity->Group = (USHORT)-1; > > + > > + /* Check that all cpus of the set belong to the same processor group and > > + * accumulate thread affinity to be applied. > > + */ > > + for (cpu_idx = 0; cpu_idx < CPU_SETSIZE; cpu_idx++) { > > + if (!CPU_ISSET(cpu_idx, cpuset)) > > + continue; > > + > > + cpu_affinity = eal_get_cpu_affinity(cpu_idx); > > + > > + if (affinity->Group == (USHORT)-1) { > > + affinity->Group = cpu_affinity->Group; > > + } else if (affinity->Group != cpu_affinity->Group) { > > + ret = EINVAL; > > Should it be ENOTSUP, because we don't support this case? > This limitation is also worth documenting in the commit log. > > > + goto cleanup; > > + } > > + > > + affinity->Mask |= cpu_affinity->Mask; > > + } > > + > > + if (affinity->Mask == 0) { > > + ret = EINVAL; > > + goto cleanup; > > + } > > + > > +cleanup: > > + return ret; > > +} > [...]