Re: [PATCH v2] rt-numa: ignore runtime cpumask if -a CPULIST is specified

[John Kacur <jkacur@redhat.com>]

On Tue, 25 Jan 2022, Marcelo Tosatti wrote:

>  
> When using isolcpus kernel command line option, the CPUs
> specified at isolcpus= are not part of the run time environment
> cpumask.
> 
> This causes "cyclictest -a isolatedcpus" to fail with:
> 
> WARN: Couldn't setaffinity in main thread: Invalid argument
> FATAL: No allowable cpus to run on
> # /dev/cpu_dma_latency set to 0us
> 
> To fix this, ignore the runtime cpumask if neither "+", "!"
> or "all" are specified in the cpu list string.
> 
> Suggested by Sebastian Andrzej Siewior.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> ---
> 
> v2: fix changelog typo
> 
> diff --git a/src/lib/rt-numa.c b/src/lib/rt-numa.c
> index ee5ab99..d887355 100644
> --- a/src/lib/rt-numa.c
> +++ b/src/lib/rt-numa.c
> @@ -9,6 +9,7 @@
>  #include <errno.h>
>  #include <sched.h>
>  #include <pthread.h>
> +#include <stdlib.h>
>  
>  #include "rt-error.h"
>  #include "rt-numa.h"
> @@ -96,13 +97,21 @@ int cpu_for_thread_ua(int thread_num, int max_cpus)
>   * the user supplied affinity mask and the affinity mask from the run
>   * time environment
>   */
> -static void use_current_cpuset(int max_cpus, struct bitmask *cpumask)
> +static void use_current_cpuset(char *str, int max_cpus, struct bitmask *cpumask)
>  {
>  	struct bitmask *curmask;
>  	int i;
>  
>  	curmask = numa_allocate_cpumask();
> -	numa_sched_getaffinity(getpid(), curmask);
> +
> +	if (strchr(str, '!') == NULL && strchr(str, '+') == NULL &&
> +	    strstr(str, "all") == NULL) {
> +		int conf_cpus = numa_num_configured_cpus();
> +
> +		for (i = 0; i < conf_cpus; i++)
> +			numa_bitmask_setbit(curmask, i);
> +	} else
> +		numa_sched_getaffinity(getpid(), curmask);
>  
>  	/*
>  	 * Clear bits that are not set in both the cpuset from the
> @@ -131,7 +140,7 @@ int parse_cpumask(char *str, int max_cpus, struct bitmask **cpumask)
>  		return 0;
>  	}
>  
> -	use_current_cpuset(max_cpus, mask);
> +	use_current_cpuset(str, max_cpus, mask);
>  	*cpumask = mask;
>  
>  	return 0;
> 
> 

I agree that our current method is too restrictive, so I like the basic 
idea here.

I think we also need to differentiate between -a with no arguments
and -a all. -a with no arguments should inherit the cpumask from the 
runtime environment but -a all, should be the same as specifying all the 
cpus and ignoring the inherited cpumask.

The work of the function parse_cpumask() is essentially in the call to 
numa_parse_cpustring_all() and then pointing the affinity_mask to the 
result. So your patch is unnecessarily complicated. Just don't call 
use_current_cpuset(). All that is needed is to change parse_cpumask()
with

-       use_current_cpuset(max_cpus, mask);
+       if (strchr(str, '!') != NULL || strchr(str, '+') != NULL)
+               use_current_cpuset(max_cpus, mask);

So, if you want to send me the above patch, I'll apply it.

While testing, I noticed that there is a bug in calling parse_cpumask
such that -aall works but -a all (with a space) was not working correctly.
I will send a patch for that.

Thanks

John