Re: [PATCH 2/3] eal: implement functions for get/set thread affinity

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

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 <navasile@microsoft.com>
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

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 <stdint.h>
> > +
> >  #include <rte_os.h>
> >  #include <rte_compat.h>
> >  
> > @@ -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;
> > +}
> [...]