[PATCH rt-tests v2 0/3] Fix a few fallouts

[PATCH rt-tests v2 0/3] Fix a few fallouts

[Daniel Wagner]

v1:
 - initial version
v2:
 - update error message for sched_get_affinity()


original cover letter:

Another round of small cleanups and fixes.

The first one fixes static builds for armhf host. Patch two is another
of the printf format specifier which I got wrong. No idea why we
didn't see this earlier. Anyway gcc on armhf was not happy. The same
for the third fix. Why the gcc on armhf complains and not on anything
else, is dubios to me.


Daniel Wagner (3):
  rt-numa: Use sched_getaffinity() instead of pthread_getaffinity_np()
  signaltest: Fix printf format specifier
  cyclicdeadline: Fix buffer allocation

 src/lib/rt-numa.c                   | 6 ++----
 src/sched_deadline/cyclicdeadline.c | 2 +-
 src/signaltest/signaltest.c         | 6 +++---
 3 files changed, 6 insertions(+), 8 deletions(-)

-- 
2.32.0

[PATCH rt-tests v2 1/3] rt-numa: Use sched_getaffinity() instead of pthread_getaffinity_np()

[Daniel Wagner]

pthread_getaffinity_np() prevents static builds as glibc does not
expose it for this configuration. Instead use sched_getaffinity()
which is always present and has the exact same semantics.

Fixes: f240656b056b ("rt-tests: cyclictest: Fix -t without a user specified [NUM]")

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 src/lib/rt-numa.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/lib/rt-numa.c b/src/lib/rt-numa.c
index babcc634d57e..bb0121a65eca 100644
--- a/src/lib/rt-numa.c
+++ b/src/lib/rt-numa.c
@@ -68,15 +68,13 @@ int cpu_for_thread_sp(int thread_num, int max_cpus, struct bitmask *cpumask)
 int cpu_for_thread_ua(int thread_num, int max_cpus)
 {
 	int res, num_cpus, i, m, cpu;
-	pthread_t thread;
 	cpu_set_t cpuset;
 
-	thread = pthread_self();
 	CPU_ZERO(&cpuset);
 
-	res = pthread_getaffinity_np(thread, sizeof(cpu_set_t), &cpuset);
+	res = sched_getaffinity(0, sizeof(cpu_set_t), &cpuset);
 	if (res != 0)
-		fatal("pthread_getaffinity_np failed: %s\n", strerror(res));
+		fatal("sched_getaffinity failed: %s\n", strerror(res));
 
 	num_cpus = CPU_COUNT(&cpuset);
 	m = thread_num % num_cpus;
-- 
2.32.0

Re: [PATCH rt-tests v2 1/3] rt-numa: Use sched_getaffinity() instead of pthread_getaffinity_np()

[John Kacur]

On Thu, 8 Jul 2021, Daniel Wagner wrote:

> pthread_getaffinity_np() prevents static builds as glibc does not
> expose it for this configuration. Instead use sched_getaffinity()
> which is always present and has the exact same semantics.
> 
> Fixes: f240656b056b ("rt-tests: cyclictest: Fix -t without a user specified [NUM]")
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  src/lib/rt-numa.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/lib/rt-numa.c b/src/lib/rt-numa.c
> index babcc634d57e..bb0121a65eca 100644
> --- a/src/lib/rt-numa.c
> +++ b/src/lib/rt-numa.c
> @@ -68,15 +68,13 @@ int cpu_for_thread_sp(int thread_num, int max_cpus, struct bitmask *cpumask)
>  int cpu_for_thread_ua(int thread_num, int max_cpus)
>  {
>  	int res, num_cpus, i, m, cpu;
> -	pthread_t thread;
>  	cpu_set_t cpuset;
>  
> -	thread = pthread_self();
>  	CPU_ZERO(&cpuset);
>  
> -	res = pthread_getaffinity_np(thread, sizeof(cpu_set_t), &cpuset);
> +	res = sched_getaffinity(0, sizeof(cpu_set_t), &cpuset);
>  	if (res != 0)
> -		fatal("pthread_getaffinity_np failed: %s\n", strerror(res));
> +		fatal("sched_getaffinity failed: %s\n", strerror(res));
>  
>  	num_cpus = CPU_COUNT(&cpuset);
>  	m = thread_num % num_cpus;
> -- 
> 2.32.0
> 
> 

It looks okay on the surface but I would like to know where you tested.

Also, in your message you explain that you can't do a static build
but we never made static builds a requirment for rt-tests. I know from
your cover letter than you are trying to do something with arm, if this
is your motivation you should put it in your message. Even if this is your 
motivation is there a reason we have to have a static build there?

Finally, you put "fixes f240656b056b", it is useful to know where this
was introduced in trying to figure out whether we can really replace a 
pthread call with the sched call, but then phrase it that way, because
as far as I know that commit is not a problem needing fixing, so put
"pthread_getaffinity_np call introduced in f240656b056b"
or something like that.

Thanks

John

Re: [PATCH rt-tests v2 1/3] rt-numa: Use sched_getaffinity() instead of pthread_getaffinity_np()

[Daniel Wagner]

On Fri, Jul 09, 2021 at 01:58:51PM -0400, John Kacur wrote:
> It looks okay on the surface but I would like to know where you tested.

x86_64 and armv7. FWIW, pthread_getaffinity_np() is a fancy wrapper
around sched_getaffinity():

  https://github.com/bminor/glibc/blob/master/nptl/pthread_getaffinity.c#L34

> Also, in your message you explain that you can't do a static build
> but we never made static builds a requirment for rt-tests.

So you are saying static builds are not supported at all?

> I know from
> your cover letter than you are trying to do something with arm, if this
> is your motivation you should put it in your message. Even if this is your 
> motivation is there a reason we have to have a static build there?

Sure thing, I'll add more to the commit message. I just find it strange
that you are starting to argue static builds are not supported. This is
very handy especially since the linker dependency on libnuma. I can drop
the binary on the target without the need to install libnuma into the
rootfs. It is not available on all distro for armv7.

Another user of static builds is the LAVA test suite. It ships the
rt-tests programs as static builds so that the test can run without the
need to pre populate the rootfs with the correct binary:

  https://github.com/Linaro/test-definitions/tree/master/automated/linux/cyclictest/bin
  https://github.com/Linaro/test-definitions/blob/master/automated/linux/cyclictest/cyclictest.sh#L41

I am not really a big fan of this either and wont defend it. I am just
listening existing users.

> Finally, you put "fixes f240656b056b", it is useful to know where this
> was introduced in trying to figure out whether we can really replace a 
> pthread call with the sched call, but then phrase it that way, because
> as far as I know that commit is not a problem needing fixing, so put
> "pthread_getaffinity_np call introduced in f240656b056b"
> or something like that.

From my POV it is fixing a bug. If you decide static builds are not
supported, please say so. This allows me to discuss this issue with the
LAVA folks.

Thanks,
Daniel

[PATCH rt-tests v2 2/3] signaltest: Fix printf format specifier

[Daniel Wagner]

The fields are not uint64 just longs, update the printf format
specifiers.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 src/signaltest/signaltest.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/signaltest/signaltest.c b/src/signaltest/signaltest.c
index 6e8f6b51b003..4d89a1aba9d9 100644
--- a/src/signaltest/signaltest.c
+++ b/src/signaltest/signaltest.c
@@ -393,9 +393,9 @@ static void write_stats(FILE *f, void *data)
 	for (i = 0; i < num_threads; i++) {
 		fprintf(f, "    \"%u\": {\n", i);
 		s = &par->stats[i];
-		fprintf(f, "      \"cycles\": %" PRIu64 ",\n", s->cycles);
-		fprintf(f, "      \"min\": %" PRIu64 ",\n", s->min);
-		fprintf(f, "      \"max\": %" PRIu64 ",\n", s->max);
+		fprintf(f, "      \"cycles\": %ld,\n", s->cycles);
+		fprintf(f, "      \"min\": %ld,\n", s->min);
+		fprintf(f, "      \"max\": %ld,\n", s->max);
 		fprintf(f, "      \"avg\": %.2f,\n", s->avg/s->cycles);
 		fprintf(f, "      \"cpu\": %d\n", par->cpu);
 		fprintf(f, "    }%s\n", i == num_threads - 1 ? "" : ",");
-- 
2.32.0

[PATCH rt-tests v2 3/3] cyclicdeadline: Fix buffer allocation

[Daniel Wagner]

gcc complains with "‘sprintf’ output between 2 and 12 bytes" but
the buffer is only 10 bytes long. Update the buffer size to hold
the complete range of [-2147483648, 2147483646].

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 src/sched_deadline/cyclicdeadline.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
index ffefa9e6fecb..8447424273ee 100644
--- a/src/sched_deadline/cyclicdeadline.c
+++ b/src/sched_deadline/cyclicdeadline.c
@@ -1092,7 +1092,7 @@ int main(int argc, char **argv)
 
 	/* Default cpu to use is the last one */
 	if (!all_cpus && !setcpu) {
-		setcpu_buf = malloc(10);
+		setcpu_buf = malloc(12);
 		if (!setcpu_buf)
 			fatal("malloc");
 		sprintf(setcpu_buf, "%d", cpu_count - 1);
-- 
2.32.0

Re: [PATCH rt-tests v2 3/3] cyclicdeadline: Fix buffer allocation

[John Kacur]

[-- Attachment #1: Type: text/plain, Size: 1882 bytes --]



On Thu, 8 Jul 2021, Daniel Wagner wrote:

> gcc complains with "‘sprintf’ output between 2 and 12 bytes" but
> the buffer is only 10 bytes long. Update the buffer size to hold
> the complete range of [-2147483648, 2147483646].
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  src/sched_deadline/cyclicdeadline.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
> index ffefa9e6fecb..8447424273ee 100644
> --- a/src/sched_deadline/cyclicdeadline.c
> +++ b/src/sched_deadline/cyclicdeadline.c
> @@ -1092,7 +1092,7 @@ int main(int argc, char **argv)
>  
>  	/* Default cpu to use is the last one */
>  	if (!all_cpus && !setcpu) {
> -		setcpu_buf = malloc(10);
> +		setcpu_buf = malloc(12);
>  		if (!setcpu_buf)
>  			fatal("malloc");
>  		sprintf(setcpu_buf, "%d", cpu_count - 1);
> -- 
> 2.32.0
> 
> 
Signed-off-by: John Kacur <jkacur@redhat.com>

I applied this but there are others.

git grep PRIu64
src/oslat/oslat.c:              snprintf(bucket_name, sizeof(bucket_name), 
"%03"PRIu64
src/oslat/oslat.c:              putfield(bucket_name, t[i].buckets[j], 
PRIu64,
src/oslat/oslat.c:      putfield("Minimum", t[i].minlat, PRIu64, " (us)");
src/oslat/oslat.c:      putfield("Maximum", t[i].maxlat, PRIu64, " (us)");
src/oslat/oslat.c:      putfield("Max-Min", t[i].maxlat - t[i].minlat, 
PRIu64, " (us)");
src/oslat/oslat.c:              fprintf(f, "      \"min\": %" PRIu64 
",\n", t[i].minlat);
src/oslat/oslat.c:              fprintf(f, "      \"max\": %" PRIu64 
",\n", t[i].maxlat);
src/oslat/oslat.c:                      fprintf(f, "        \"%" PRIu64 
"\": %" PRIu64,
src/oslat/oslat.c:      printf("Workload mem: \t\t%"PRIu64" (KiB)\n",
src/pi_tests/pi_stress.c:               printf(" runtime %" PRIu64 " 
deadline %" PRIu64 " period %" PRIu64 "\n",

Re: [PATCH rt-tests v2 3/3] cyclicdeadline: Fix buffer allocation

[Daniel Wagner]

On Fri, Jul 09, 2021 at 02:01:35PM -0400, John Kacur wrote:
> I applied this but there are others.

All those variables are defined as uint64_t, hence the PRIu64 is
correct.

Re: [PATCH rt-tests v2 3/3] cyclicdeadline: Fix buffer allocation

[John Kacur]

[-- Attachment #1: Type: text/plain, Size: 963 bytes --]



On Thu, 8 Jul 2021, Daniel Wagner wrote:

> gcc complains with "‘sprintf’ output between 2 and 12 bytes" but
> the buffer is only 10 bytes long. Update the buffer size to hold
> the complete range of [-2147483648, 2147483646].
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  src/sched_deadline/cyclicdeadline.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/sched_deadline/cyclicdeadline.c b/src/sched_deadline/cyclicdeadline.c
> index ffefa9e6fecb..8447424273ee 100644
> --- a/src/sched_deadline/cyclicdeadline.c
> +++ b/src/sched_deadline/cyclicdeadline.c
> @@ -1092,7 +1092,7 @@ int main(int argc, char **argv)
>  
>  	/* Default cpu to use is the last one */
>  	if (!all_cpus && !setcpu) {
> -		setcpu_buf = malloc(10);
> +		setcpu_buf = malloc(12);
>  		if (!setcpu_buf)
>  			fatal("malloc");
>  		sprintf(setcpu_buf, "%d", cpu_count - 1);
> -- 
> 2.32.0
> 
> 
Signed-off-by: John Kacur <jkacur@redhat.com>