[PATCH] rt-numa: Use a reasonable default max CPU value.

[PATCH] rt-numa: Use a reasonable default max CPU value.

[Sebastian Andrzej Siewior]

numa_num_task_cpus() returns here > 100 CPUs while the system has only
32 populated. The BIOS assumes that I can probably use larger CPUs (with
more cores) on the socket so the number of "configured CPUs" is rather
high.
For default configuration, only with the -S option, it makes sense to
use the current affinity instead looking at the number of possible CPUs
which could be brought online. It still depends on the affinity of the
created threads if the additional CPUs can be used. In a container setup
this may not be the case.

Use sched_getaffinity() to figure out the number of possible CPUs.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 src/lib/rt-numa.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/lib/rt-numa.c b/src/lib/rt-numa.c
index 3eead80c3b2b3..1b09cff8d9485 100644
--- a/src/lib/rt-numa.c
+++ b/src/lib/rt-numa.c
@@ -35,10 +35,19 @@ int numa_initialize(void)
 
 int get_available_cpus(struct bitmask *cpumask)
 {
+	cpu_set_t cpuset;
+	int ret;
+
 	if (cpumask)
 		return numa_bitmask_weight(cpumask);
 
-	return numa_num_task_cpus();
+	CPU_ZERO(&cpuset);
+
+	ret = sched_getaffinity(0, sizeof(cpu_set_t), &cpuset);
+	if (ret < 0)
+		fatal("sched_getaffinity failed: %m\n");
+
+	return CPU_COUNT(&cpuset);
 }
 
 int cpu_for_thread_sp(int thread_num, int max_cpus, struct bitmask *cpumask)
-- 
2.39.0

Re: [PATCH] rt-numa: Use a reasonable default max CPU value.

[John Kacur]

On Thu, 12 Jan 2023, Sebastian Andrzej Siewior wrote:

> numa_num_task_cpus() returns here > 100 CPUs while the system has only
> 32 populated. The BIOS assumes that I can probably use larger CPUs (with
> more cores) on the socket so the number of "configured CPUs" is rather
> high.
> For default configuration, only with the -S option, it makes sense to
> use the current affinity instead looking at the number of possible CPUs
> which could be brought online. It still depends on the affinity of the
> created threads if the additional CPUs can be used. In a container setup
> this may not be the case.
> 
> Use sched_getaffinity() to figure out the number of possible CPUs.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  src/lib/rt-numa.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/lib/rt-numa.c b/src/lib/rt-numa.c
> index 3eead80c3b2b3..1b09cff8d9485 100644
> --- a/src/lib/rt-numa.c
> +++ b/src/lib/rt-numa.c
> @@ -35,10 +35,19 @@ int numa_initialize(void)
>  
>  int get_available_cpus(struct bitmask *cpumask)
>  {
> +	cpu_set_t cpuset;
> +	int ret;
> +
>  	if (cpumask)
>  		return numa_bitmask_weight(cpumask);
>  
> -	return numa_num_task_cpus();
> +	CPU_ZERO(&cpuset);
> +
> +	ret = sched_getaffinity(0, sizeof(cpu_set_t), &cpuset);
> +	if (ret < 0)
> +		fatal("sched_getaffinity failed: %m\n");
> +
> +	return CPU_COUNT(&cpuset);
>  }
>  
>  int cpu_for_thread_sp(int thread_num, int max_cpus, struct bitmask *cpumask)
> -- 
> 2.39.0
> 

In cyclictest we have

	if (num_threads == -1)
		num_threads = get_available_cpus(affinity_mask);


But the num_threads can be set to -1 in OPT_THREADS too,
not just for OPT_SMP

John Kacur

Re: [PATCH] rt-numa: Use a reasonable default max CPU value.

[Ahmed S. Darwish]

On Mon, 16 Jan 2023, John Kacur wrote:
>
> On Thu, 12 Jan 2023, Sebastian Andrzej Siewior wrote:
>
> > numa_num_task_cpus() returns here > 100 CPUs while the system has only
> > 32 populated. The BIOS assumes that I can probably use larger CPUs (with
> > more cores) on the socket so the number of "configured CPUs" is rather
> > high.
> > For default configuration, only with the -S option, it makes sense to
> > use the current affinity instead looking at the number of possible CPUs
> > which could be brought online. It still depends on the affinity of the
> > created threads if the additional CPUs can be used. In a container setup
> > this may not be the case.
> >
> > Use sched_getaffinity() to figure out the number of possible CPUs.
...
>
> In cyclictest we have
>
> 	if (num_threads == -1)
> 		num_threads = get_available_cpus(affinity_mask);
>
>
> But the num_threads can be set to -1 in OPT_THREADS too,
> not just for OPT_SMP

A sane default for --smp, which is almost-always used in sane cyclictest
invocations, is still valuable IMHO.

Thanks,

--
Ahmed S. Darwish
Linutronix GmbH

Re: [PATCH] rt-numa: Use a reasonable default max CPU value.

[John Kacur]

On Mon, 16 Jan 2023, Ahmed S. Darwish wrote:

> On Mon, 16 Jan 2023, John Kacur wrote:
> >
> > On Thu, 12 Jan 2023, Sebastian Andrzej Siewior wrote:
> >
> > > numa_num_task_cpus() returns here > 100 CPUs while the system has only
> > > 32 populated. The BIOS assumes that I can probably use larger CPUs (with
> > > more cores) on the socket so the number of "configured CPUs" is rather
> > > high.
> > > For default configuration, only with the -S option, it makes sense to
> > > use the current affinity instead looking at the number of possible CPUs
> > > which could be brought online. It still depends on the affinity of the
> > > created threads if the additional CPUs can be used. In a container setup
> > > this may not be the case.
> > >
> > > Use sched_getaffinity() to figure out the number of possible CPUs.
> ...
> >
> > In cyclictest we have
> >
> > 	if (num_threads == -1)
> > 		num_threads = get_available_cpus(affinity_mask);
> >
> >
> > But the num_threads can be set to -1 in OPT_THREADS too,
> > not just for OPT_SMP
> 
> A sane default for --smp, which is almost-always used in sane cyclictest
> invocations, is still valuable IMHO.
> 
> Thanks,
> 
> --
> Ahmed S. Darwish
> Linutronix GmbH
> 

Okay, not sure how that adds to the conversation. I'm all for "sane" 
defaults for --smp too, but we care about other kinds of invocations too. 
As soon as I get a patch that doesn't break other things as well, I'll 
apply it.

John

Re: [PATCH] rt-numa: Use a reasonable default max CPU value.

[Sebastian Andrzej Siewior]

On 2023-01-16 13:58:42 [-0500], John Kacur wrote:
> 
> Okay, not sure how that adds to the conversation. I'm all for "sane" 
> defaults for --smp too, but we care about other kinds of invocations too. 
> As soon as I get a patch that doesn't break other things as well, I'll 
> apply it.

BROKEN IT IS.

I want to use that and not fix every time I use it. Using
	-S -p 90 -d0 -m

are already a lot of options just for a simple test to use all CPUs on a
system. This used to work on this machine. Now it starts way more
threads than I have CPUs. I accept that this might be something that
someone wants to test so he can use -t 100. But the simple test that is
no longer working. srsly. Starting more threads than the current
available online CPUs or even CPU affinity makes no sense since the
results with equal priority distribution or not reproducible.

> John

Sebastian

Re: [PATCH] rt-numa: Use a reasonable default max CPU value.

[John Kacur]

On Tue, 17 Jan 2023, Sebastian Andrzej Siewior wrote:

> On 2023-01-16 13:58:42 [-0500], John Kacur wrote:
> > 
> > Okay, not sure how that adds to the conversation. I'm all for "sane" 
> > defaults for --smp too, but we care about other kinds of invocations too. 
> > As soon as I get a patch that doesn't break other things as well, I'll 
> > apply it.
> 
> BROKEN IT IS.

Well something is broken isn't it? But could it be your BIOS then?
This does work for most people on most machines.

The thing I'm always on the look out for is making sure a fix for one 
problem doesn't cause problems elsewhere.

> 
> I want to use that and not fix every time I use it. Using
> 	-S -p 90 -d0 -m
> 
> are already a lot of options just for a simple test to use all CPUs on a
> system. This used to work on this machine. Now it starts way more
> threads than I have CPUs. I accept that this might be something that
> someone wants to test so he can use -t 100. But the simple test that is
> no longer working. srsly. Starting more threads than the current
> available online CPUs or even CPU affinity makes no sense since the
> results with equal priority distribution or not reproducible.

Right, your patch said
"For default configuration, only with the -S option"

So, I looked at the code, and thought, this could be called
for OPT_THREADS as well as for OPT_SMP

That's the only thing I asked you to address.

However, taking a step back, I think it will work fine for OPT_THREADS as 
well. In fact we used to do it that way, it got changed here.

commit 76ad600da83771dc4985b4a9db1f63344bb364cb

with what certainely seemed like a sane clean-up.

So, I'm just going to do a little more testing, and make sure
it doesn't break rteval, and then I'll likely apply your patch.

John

> 
> > John
> 
> Sebastian
> 

[PATCH] rt-numa: Use a reasonable default max CPU value.

[John Kacur]

From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

numa_num_task_cpus() returns here > 100 CPUs while the system has only
32 populated. The BIOS assumes that I can probably use larger CPUs (with
more cores) on the socket so the number of "configured CPUs" is rather
high.

It makes sense to use the current affinity instead of looking
at the number of possible CPUs which could be brought online.
It still depends on the affinity of the created threads
if the additional CPUs can be used. In a container setup
this may not be the case.

Use sched_getaffinity() to figure out the number of possible CPUs.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
- Edited the commit message to remove reference to -S
Signed-off-by: John Kacur <jkacur@redhat.com>
---
 src/lib/rt-numa.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/lib/rt-numa.c b/src/lib/rt-numa.c
index 3eead80c3b2b..1b09cff8d948 100644
--- a/src/lib/rt-numa.c
+++ b/src/lib/rt-numa.c
@@ -35,10 +35,19 @@ int numa_initialize(void)
 
 int get_available_cpus(struct bitmask *cpumask)
 {
+	cpu_set_t cpuset;
+	int ret;
+
 	if (cpumask)
 		return numa_bitmask_weight(cpumask);
 
-	return numa_num_task_cpus();
+	CPU_ZERO(&cpuset);
+
+	ret = sched_getaffinity(0, sizeof(cpu_set_t), &cpuset);
+	if (ret < 0)
+		fatal("sched_getaffinity failed: %m\n");
+
+	return CPU_COUNT(&cpuset);
 }
 
 int cpu_for_thread_sp(int thread_num, int max_cpus, struct bitmask *cpumask)
-- 
2.39.0