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

[Richard Palethorpe <rpalethorpe@suse.de>]

Hello,

zhaogongyi <zhaogongyi@huawei.com> writes:

> Hi!
>
>> 
>> 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_setscheduler0
>> > +++ 4.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.
>
> Agree, I will fix it in the next version.
>
>> 
>> > +}
>> > +
>> > +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.
>
> If move it to setup function, we run the test with option "-I 200", it
> will fail.

Why?

>
>> 
>> > +
>> > +	test_sched_setscheduler01();
>> > +	test_sched_setscheduler02();
>> 
>> This should be replaced with .tcnt = 2 and .test. or else just merge them
>> into run.
>
> Agree, I will fix it int the next version.
>
>> 
>> > +}
>> > +
>> > +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
>> }
>> 
>
> In this testcase, we just increase the priority, should we add the checking of capability?

For the test to run we need CAP_SYS_NICE. If you want to test that the
priority can only be set when CAP_SYS_NICE is available, then that is a
seperate test in my opinion.

-- 
Thank you,
Richard.

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