Re: [LTP] [PATCH v4] syscalls/nice05: Add testcase for nice() syscall

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

zhaogongyi via ltp <ltp@lists.linux.it> writes:

> Hi Cyril,
>
> Thanks for your review! I have resubmit a patch according to your
> suggestion. Please see:
> https://patchwork.ozlabs.org/project/ltp/patch/20220824095144.259871-1-zhaogongyi@huawei.com/

Merged now thanks!

If the test fails randomly then increasing max_runtime will probably help.

>
> Best Wishes,
> Gongyi
>
>> 
>> Hi!
>> > Add test verifies that the low nice thread executes more time than the
>> > high nice thread since the two thread binded on the same cpu.
>> 
>> Looks very good now, there are few very minor points see below.
>> 
>> > Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
>> > ---
>> > v3->v4: Replace getting exec time from sum_exec_runtime with
>> pthread_getcpuclockid().
>> >
>> >  runtest/syscalls                          |   1 +
>> >  testcases/kernel/syscalls/nice/.gitignore |   1 +
>> >  testcases/kernel/syscalls/nice/Makefile   |   2 +
>> >  testcases/kernel/syscalls/nice/nice05.c   | 188
>> ++++++++++++++++++++++
>> >  4 files changed, 192 insertions(+)
>> >  create mode 100644 testcases/kernel/syscalls/nice/nice05.c
>> >
>> > diff --git a/runtest/syscalls b/runtest/syscalls index
>> > 9d58e0aa1..98fcbbe1e 100644
>> > --- a/runtest/syscalls
>> > +++ b/runtest/syscalls
>> > @@ -903,6 +903,7 @@ nice01 nice01
>> >  nice02 nice02
>> >  nice03 nice03
>> >  nice04 nice04
>> > +nice05 nice05
>> >
>> >  open01 open01
>> >  open01A symlink01 -T open01
>> > diff --git a/testcases/kernel/syscalls/nice/.gitignore
>> > b/testcases/kernel/syscalls/nice/.gitignore
>> > index 9d7a1bb43..58d64779e 100644
>> > --- a/testcases/kernel/syscalls/nice/.gitignore
>> > +++ b/testcases/kernel/syscalls/nice/.gitignore
>> > @@ -2,3 +2,4 @@
>> >  /nice02
>> >  /nice03
>> >  /nice04
>> > +/nice05
>> > diff --git a/testcases/kernel/syscalls/nice/Makefile
>> > b/testcases/kernel/syscalls/nice/Makefile
>> > index 044619fb8..02e78a295 100644
>> > --- a/testcases/kernel/syscalls/nice/Makefile
>> > +++ b/testcases/kernel/syscalls/nice/Makefile
>> > @@ -3,6 +3,8 @@
>> >
>> >  top_srcdir		?= ../../../..
>> >
>> > +nice05: CFLAGS += -pthread
>> > +
>> >  include $(top_srcdir)/include/mk/testcases.mk
>> >
>> >  include $(top_srcdir)/include/mk/generic_leaf_target.mk
>> > diff --git a/testcases/kernel/syscalls/nice/nice05.c
>> > b/testcases/kernel/syscalls/nice/nice05.c
>> > new file mode 100644
>> > index 000000000..8ef33f932
>> > --- /dev/null
>> > +++ b/testcases/kernel/syscalls/nice/nice05.c
>> > @@ -0,0 +1,188 @@
>> > +// SPDX-License-Identifier: GPL-2.0-or-later
>> > +/*
>> > + * Copyright(c) 2022 Huawei Technologies Co., Ltd
>> > + * Author: Li Mengfei <limengfei4@huawei.com>
>> > + *         Zhao Gongyi <zhaogongyi@huawei.com>
>> > + */
>> > +
>> > +/*\
>> > + * [Description]
>> > + *
>> > + * 1. Create a high nice thread and a low nice thread, the main
>> > + *    thread wake them at the same time
>> > + * 2. Both threads run on the same CPU
>> > + * 3. Verify that the low nice thread executes more time than
>> > + *    the high nice thread
>> > + */
>> > +
>> > +#define _GNU_SOURCE
>> > +#include <pthread.h>
>> > +#include <sys/types.h>
>> > +#include <stdio.h>
>> > +#include "tst_test.h"
>> > +#include "tst_safe_pthread.h"
>> > +#include "lapi/syscalls.h"
>> > +
>> > +#define SEC2NS(sec)  ((sec) * 1000000000LL)
>> > +
>> > +static pthread_barrier_t barrier;
>> > +static int some_cpu;
>> > +static cpu_set_t *set;
>> > +
>> > +static void set_nice(int nice_inc)
>> > +{
>> > +	int orig_nice;
>> > +
>> > +	orig_nice = SAFE_GETPRIORITY(PRIO_PROCESS, 0);
>> > +
>> > +	TEST(nice(nice_inc));
>> > +
>> > +	if (TST_RET != (orig_nice + nice_inc)) {
>> > +		tst_brk(TBROK | TTERRNO, "nice(%d) returned %li, expected %i",
>> > +			nice_inc, TST_RET, orig_nice + nice_inc);
>> > +	}
>> > +
>> > +	if (TST_ERR)
>> > +		tst_brk(TBROK | TTERRNO, "nice(%d) failed", nice_inc); }
>> > +
>> > +static void *nice_low_thread(void *arg) {
>> > +	volatile int number = 0;
>> > +
>> > +	set_nice((intptr_t)arg);
>> > +	TEST(pthread_barrier_wait(&barrier));
>> > +	if (TST_RET != 0 && TST_RET != PTHREAD_BARRIER_SERIAL_THREAD)
>> > +		tst_brk(TBROK | TRERRNO, "pthread_barrier_wait() failed");
>> > +
>> > +	while (1)
>> > +		number++;
>> > +
>> > +	return NULL;
>> > +}
>> > +
>> > +static void *nice_high_thread(void *arg) {
>> > +	volatile int number = 0;
>> > +
>> > +	set_nice((intptr_t)arg);
>> > +	TEST(pthread_barrier_wait(&barrier));
>> > +	if (TST_RET != 0 && TST_RET != PTHREAD_BARRIER_SERIAL_THREAD)
>> > +		tst_brk(TBROK | TRERRNO, "pthread_barrier_wait() failed");
>> 
>> It may be worth to add SAFE_PTHREAD_BARRIER_WAIT() to the
>> tst_safe_pthread_h to make the code nicer.
>> 
>> > +	while (1)
>> > +		number++;
>> > +
>> > +	return NULL;
>> > +}
>> > +
>> > +static void setup(void)
>> > +{
>> > +	size_t size;
>> > +	size_t i;
>> > +	int nrcpus = 1024;
>> > +
>> > +	set = CPU_ALLOC(nrcpus);
>> > +	if (!set)
>> > +		tst_brk(TBROK | TERRNO, "CPU_ALLOC()");
>> > +
>> > +	size = CPU_ALLOC_SIZE(nrcpus);
>> > +	CPU_ZERO_S(size, set);
>> > +	if (sched_getaffinity(0, size, set) < 0)
>> > +		tst_brk(TBROK | TERRNO, "sched_getaffinity()");
>> > +
>> > +	for (i = 0; i < size * 8; i++)
>> > +		if (CPU_ISSET_S(i, size, set))
>> > +			some_cpu = i;
>> > +
>> > +	CPU_ZERO_S(size, set);
>> > +	CPU_SET_S(some_cpu, size, set);
>> > +	if (sched_setaffinity(0, size, set) < 0)
>> > +		tst_brk(TBROK | TERRNO, "sched_setaffinity()"); }
>> > +
>> > +static void cleanup(void)
>> > +{
>> > +	if (set)
>> > +		CPU_FREE(set);
>> 
>> This is very minor however we do not seem to use set anywhere outside
>> the setup so we may as well free it there.
>> 
>> > +}
>> > +
>> > +static void verify_nice(void)
>> > +{
>> > +	intptr_t nice_inc_high = -1;
>> > +	intptr_t nice_inc_low = -2;
>> > +	clockid_t nice_low_clockid, nice_high_clockid;
>> > +	struct timespec nice_high_ts, nice_low_ts;
>> > +	long long delta;
>> > +	pid_t pid;
>> > +	pthread_t thread[2];
>> > +
>> > +	pid = SAFE_FORK();
>> > +	if (!pid) {
>> 
>> Is there a reason why we run the actual test in the child?
>> 
>> > +		TEST(pthread_barrier_init(&barrier, NULL, 3));
>> > +		if (TST_RET != 0) {
>> > +			tst_brk(TBROK | TTERRNO,
>> > +					"pthread_barrier_init() failed");
>> > +		}
>> > +
>> > +		SAFE_PTHREAD_CREATE(&thread[0], NULL, nice_high_thread,
>> > +				(void *)nice_inc_high);
>> > +		SAFE_PTHREAD_CREATE(&thread[1], NULL, nice_low_thread,
>> > +				(void *)nice_inc_low);
>> > +
>> > +		TEST(pthread_barrier_wait(&barrier));
>> > +		if (TST_RET != 0 && TST_RET !=
>> PTHREAD_BARRIER_SERIAL_THREAD) {
>> > +			tst_brk(TBROK | TTERRNO,
>> > +					"pthread_barrier_wait() failed");
>> > +		}
>> > +
>> > +		sleep(tst_remaining_runtime());
>> > +
>> > +		if (pthread_getcpuclockid(thread[1], &nice_low_clockid) != 0) {
>> > +			perror("clock_getcpuclockid");
>> > +			tst_brk(TBROK | TERRNO,
>> > +					"clock_getcpuclockid() failed");
>> > +		}
>> > +		if (pthread_getcpuclockid(thread[0], &nice_high_clockid) != 0) {
>> > +			perror("clock_getcpuclockid");
>> > +			tst_brk(TBROK | TERRNO,
>> > +					"clock_getcpuclockid() failed");
>> > +		}
>> > +
>> > +		if (clock_gettime(nice_low_clockid, &nice_low_ts) == -1) {
>> > +			tst_brk(TBROK | TERRNO,
>> > +					"clock_getcpuclockid() failed");
>> > +		}
>> > +
>> > +		if (clock_gettime(nice_high_clockid, &nice_high_ts) == -1) {
>> > +			tst_brk(TBROK | TERRNO,
>> > +					"clock_getcpuclockid() failed");
>> > +		}
>> 
>> We do have SAFE_CLOCK_GETTIME() please use them.
>> 
>> > +		tst_res(TINFO, "Nice low thread CPU time: %ld.%09ld s",
>> > +			nice_low_ts.tv_sec, nice_low_ts.tv_nsec);
>> > +		tst_res(TINFO, "Nice high thread CPU time: %ld.%09ld s",
>> > +			nice_high_ts.tv_sec, nice_high_ts.tv_nsec);
>> > +
>> > +		delta = SEC2NS(nice_low_ts.tv_sec - nice_high_ts.tv_sec) +
>> > +			(nice_low_ts.tv_nsec - nice_high_ts.tv_nsec);
>> 
>> We do have a tst_timespec_diff_{us,ns,ms} functions in the tst_timer.h so
>> we may as well use them.
>> 
>> > +		if (delta < 0) {
>> > +			tst_res(TFAIL, "executes less cycles than "
>> > +				"the high nice thread, delta: %lld ns", delta);
>> > +		} else {
>> > +			tst_res(TPASS, "executes more cycles than "
>> > +				"the high nice thread, delta: %lld ns", delta);
>> > +		}
>> > +		return;
>> > +	}
>> > +	SAFE_WAIT(NULL);
>> > +}
>> > +
>> > +static struct tst_test test = {
>> > +	.setup = setup,
>> > +	.cleanup = cleanup,
>> > +	.test_all = verify_nice,
>> > +	.needs_root = 1,
>> > +	.forks_child = 1,
>> > +	.max_runtime = 3,
>> > +};
>> > --
>> > 2.17.1
>> >
>> 
>> --
>> Cyril Hrubis
>> chrubis@suse.cz


-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp