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

[Cyril Hrubis <chrubis@suse.cz>]

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

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