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

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

[zhaogongyi via ltp]

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.

> 
> > +
> > +	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?


Regards,
Gongyi

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

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

[Richard Palethorpe]

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

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

[zhaogongyi via ltp]

Hi!

> > >> > +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?

sched_prio is a global variable, and it increase in any running loop, thus it will be out of range and report:

sched_setscheduler04.c:52: TPASS: param.sched_priority == sched_prio (99)
sched_setscheduler04.c:53: TPASS: new_policy == EXP_POLICY (1)
sched_setscheduler04.c:34: TINFO: Setting of tid: 70774
sched_setscheduler04.c:40: TBROK: sched_setscheduler(70774, 1, ...) failed: EINVAL (22)

Summary:
passed   196
failed   0
broken   1
skipped  0
warnings 0


> >
> >>
> >> > +
> >> > +	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.

Anybody can call nice() to increase the priority, i have test it with normal user and passed.

And if decrease the priority, the test will report EPERM when running with normal user.

Regards,
Gongyi

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

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

[zhaogongyi via ltp]

Hi!

> 
> > > >> > +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?
> 
> sched_prio is a global variable, and it increase in any running loop, thus it
> will be out of range and report:
> 
> sched_setscheduler04.c:52: TPASS: param.sched_priority == sched_prio
> (99)
> sched_setscheduler04.c:53: TPASS: new_policy == EXP_POLICY (1)
> sched_setscheduler04.c:34: TINFO: Setting of tid: 70774
> sched_setscheduler04.c:40: TBROK: sched_setscheduler(70774, 1, ...)
> failed: EINVAL (22)
> 
> Summary:
> passed   196
> failed   0
> broken   1
> skipped  0
> warnings 0
> 
> 
> > >
> > >>
> > >> > +
> > >> > +	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.
> 
> Anybody can call nice() to increase the priority, i have test it with normal
> user and passed.
> 
> And if decrease the priority, the test will report EPERM when running with
> normal user.
> 

I have add a checking of capability in the v2 ptath, please see: https://patchwork.ozlabs.org/project/ltp/patch/20221116021651.21104-1-zhaogongyi@huawei.com/

Regards,
Gongyi

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