Re: [LTP] [PATCH] syscalls/sched_setscheduler04: new test for sched_setscheduler()

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

Zhao Gongyi via ltp <ltp@lists.linux.it> writes:

> Verify that the scheduling policy and parameters are in fact per-thread
> attributes on Linux:
> 1. Specifying pid as 0 will operate on the attributes of the calling thread
> 2. The value returned from a call to gettid(2) can be passed in the argument
>    pid.
> 3. Passing the value returned from a call to getpid(2) will operate on the
>    attributes of the main thread of the  thread  group
>
> Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> ---
>  runtest/syscalls                              |  1 +
>  .../syscalls/sched_setscheduler/.gitignore    |  1 +
>  .../syscalls/sched_setscheduler/Makefile      |  2 +
>  .../sched_setscheduler/sched_setscheduler04.c | 91 +++++++++++++++++++
>  4 files changed, 95 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/sched_setscheduler/sched_setscheduler04.c
>
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 3dc6fa397..ff516af3d 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -1204,6 +1204,7 @@ sched_getscheduler02 sched_getscheduler02
>  sched_setscheduler01 sched_setscheduler01
>  sched_setscheduler02 sched_setscheduler02
>  sched_setscheduler03 sched_setscheduler03
> +sched_setscheduler04 sched_setscheduler04
>
>  sched_yield01 sched_yield01
>
> diff --git a/testcases/kernel/syscalls/sched_setscheduler/.gitignore b/testcases/kernel/syscalls/sched_setscheduler/.gitignore
> index aa8ad9695..1b8860d2c 100644
> --- a/testcases/kernel/syscalls/sched_setscheduler/.gitignore
> +++ b/testcases/kernel/syscalls/sched_setscheduler/.gitignore
> @@ -1,3 +1,4 @@
>  /sched_setscheduler01
>  /sched_setscheduler02
>  /sched_setscheduler03
> +/sched_setscheduler04
> diff --git a/testcases/kernel/syscalls/sched_setscheduler/Makefile b/testcases/kernel/syscalls/sched_setscheduler/Makefile
> index 044619fb8..e3d54e33e 100644
> --- a/testcases/kernel/syscalls/sched_setscheduler/Makefile
> +++ b/testcases/kernel/syscalls/sched_setscheduler/Makefile
> @@ -3,6 +3,8 @@
>
>  top_srcdir		?= ../../../..
>
> +sched_setscheduler04: CFLAGS += -pthread
> +
>  include $(top_srcdir)/include/mk/testcases.mk
>
>  include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/sched_setscheduler/sched_setscheduler04.c b/testcases/kernel/syscalls/sched_setscheduler/sched_setscheduler04.c
> new file mode 100644
> index 000000000..6f985be88
> --- /dev/null
> +++ b/testcases/kernel/syscalls/sched_setscheduler/sched_setscheduler04.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright(c) 2022 Huawei Technologies Co., Ltd
> + * Author: Zhao Gongyi <zhaogongyi@huawei.com>
> + */
> +
> +/*\
> + * [Description]
> + *
> + * Verify that the scheduling policy and parameters are in fact per-thread
> + * attributes on Linux:
> + * 1. Specifying pid as 0 will operate on the attributes of the calling thread
> + * 2. The value returned from a call to gettid(2) can be passed in the argument
> + *    pid.
> + * 3. Passing the value returned from a call to getpid(2) will operate on the
> + *    attributes of the main thread of the  thread  group
> + */
> +#include "tst_test.h"
> +#include "lapi/syscalls.h"
> +#include "tst_safe_pthread.h"
> +#include <pthread.h>
> +
> +static struct sched_param param;
> +static volatile int sched_prio;
> +
> +#define SCHED_POLICY SCHED_FIFO
> +
> +static void set_param(int tid)
> +{
> +	param.sched_priority = sched_prio;
> +
> +	if (sched_setscheduler(tid, SCHED_POLICY, &param)) {
> +		tst_brk(TBROK | TERRNO,
> +			"sched_setscheduler(%d, %d, ...) failed",
> +			tid, SCHED_POLICY);
> +	}
> +}
> +
> +static void check_prio(int tid)
> +{
> +	if (sched_getparam(tid, &param) != 0)
> +		tst_brk(TBROK | TERRNO, "sched_getparam() failed");
> +
> +	if (param.sched_priority != sched_prio)
> +		tst_res(TFAIL, "Checking of thread priority failed");
> +	else
> +		tst_res(TPASS, "Checking of thread priority passed");

How are we going to debug a test failure?

This can be replaced with the TST_EXP_* macros which will print more
info if used correctly.

At the least we should print what thread or pid we are operating
on. What the priority and policies were originally, what we changed them
to and what they are at the end.

> +}
> +
> +static void *thread_func(LTP_ATTRIBUTE_UNUSED void *arg)
> +{
> +	pid_t threadid = tst_syscall(__NR_gettid);
> +
> +	sched_prio++;
> +	set_param(0);
> +	check_prio(threadid);
> +
> +	sched_prio++;
> +	set_param(threadid);
> +	check_prio(threadid);
> +
> +	return NULL;
> +}
> +
> +static void test_sched_setscheduler01(void)
> +{
> +	sched_prio++;
> +	set_param(getpid());
> +	check_prio(tst_syscall(__NR_gettid));
> +}
> +
> +static void test_sched_setscheduler02(void)
> +{
> +	pthread_t tid;
> +
> +	SAFE_PTHREAD_CREATE(&tid, NULL, thread_func, NULL);
> +	SAFE_PTHREAD_JOIN(tid, NULL);
> +}
> +
> +
> +static void run(void)
> +{
> +	sched_prio = sched_get_priority_min(SCHED_POLICY);

This can go in the setup function.

> +
> +	test_sched_setscheduler01();
> +	test_sched_setscheduler02();

This should be replaced with .tcnt = 2 and .test. or else just merge
them into run.

> +}
> +
> +static struct tst_test test = {
> +	.test_all = run,

We probably need to ensure CAP_SYS_NICE. Full details are in 'man 7
sched'

i.e. .caps = (struct tst_cap[]) {
  TST_CAP(TST_CAP_REQ, CAP_SYS_NICE),
   null
}

> +};
> --
> 2.17.1

Setting patchwork to changes requested for the reasons above.

-- 
Thank you,
Richard.

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