[LTP] [PATCH] nice05: Add testcase for nice() syscall

[zhaogongyi]

Hi Petr,

Thanks for your review!

> make check complains about style, could you please fix it?
> 
> $ make check-nice05
> CHECK testcases/kernel/syscalls/nice/nice05.c
> nice05.c:48: ERROR: "foo* bar" should be "foo *bar"
> nice05.c:48: ERROR: "foo* bar" should be "foo *bar"
> nice05.c:53: ERROR: "(foo*)" should be "(foo *)"
> nice05.c:70: ERROR: "foo* bar" should be "foo *bar"
> nice05.c:70: ERROR: "foo* bar" should be "foo *bar"
> nice05.c:75: ERROR: "(foo*)" should be "(foo *)"
> nice05.c:104: WARNING: braces {} are not necessary for single statement
> blocks
> nice05.c:114: WARNING: braces {} are not necessary for single statement
> blocks
> nice05.c:138: ERROR: "(foo*)" should be "(foo *)"
> nice05.c:139: ERROR: "(foo*)" should be "(foo *)"
> nice05.c:148: ERROR: space required before the open parenthesis '('
> nice05.c:154: ERROR: space required before the open parenthesis '('
> make: [../../../../include/mk/rules.mk:56: check-nice05] Error 1 (ignored)
>
I have fix it with "make check-nice05".

> > +#define LIMIT_TIME 3
> > +#define RUN_TIMES 1000000
> It might be useful if user could change this value with getopt switch (to
> debug on error to speedup). I also wonder if we hit timeout on some slow
> machine.
>
I have rewrite the check point with new implement.

> Maybe just:
> 	if (pthread_barrier_init(&barrier, NULL, 3) != 0)
> 		tst_brk(TBROK | TERRNO, "pthread_barrier_init() failed");
>

pthread_barrier_init do not set errno when it is failed according to "man 3"


> if (pthread_cancel(nice_high))
> 	tst_brk(TBROK | TERRNO, "pthread_cancel() failed");
It's ok now.


> > +	if (time_nice_low > time_nice_high) {
> > +		tst_res(TPASS, "time_nice_low = %lld time_nice_high = %lld",
> > +			time_nice_low, time_nice_high);
> > +	} else {
> > +		tst_brk(TFAIL | TTERRNO, "Test failed :"
> Wrong space between ':', missing after it, use:
> 		tst_brk(TFAIL | TTERRNO, "Test failed: "
Thanks, it's ok now.


I have rewrite the patch and submit it, please see: https://patchwork.ozlabs.org/project/ltp/patch/20220618014014.224668-1-zhaogongyi@huawei.com/

Best wishes,
Gongyi



> 
> Hi Zhao,
> 
> on first look LGTM, few notes below.
> 
> make check complains about style, could you please fix it?
> 
> $ make check-nice05
> CHECK testcases/kernel/syscalls/nice/nice05.c
> nice05.c:48: ERROR: "foo* bar" should be "foo *bar"
> nice05.c:48: ERROR: "foo* bar" should be "foo *bar"
> nice05.c:53: ERROR: "(foo*)" should be "(foo *)"
> nice05.c:70: ERROR: "foo* bar" should be "foo *bar"
> nice05.c:70: ERROR: "foo* bar" should be "foo *bar"
> nice05.c:75: ERROR: "(foo*)" should be "(foo *)"
> nice05.c:104: WARNING: braces {} are not necessary for single statement
> blocks
> nice05.c:114: WARNING: braces {} are not necessary for single statement
> blocks
> nice05.c:138: ERROR: "(foo*)" should be "(foo *)"
> nice05.c:139: ERROR: "(foo*)" should be "(foo *)"
> nice05.c:148: ERROR: space required before the open parenthesis '('
> nice05.c:154: ERROR: space required before the open parenthesis '('
> make: [../../../../include/mk/rules.mk:56: check-nice05] Error 1 (ignored)
> 
> 
> > Add test verify that the low nice thread execute more cycles than
>                                            ^ executes
> 
> > the high nice thread since the two thread binded on the same cpu.
> 
> > Signed-off-by: Zhao Gongyi <zhaogongyi@huawei.com>
> 
> ...
> > diff --git a/testcases/kernel/syscalls/nice/nice05.c
> > b/testcases/kernel/syscalls/nice/nice05.c
> ...
> > +/*\
> > + * [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 execute more cycles than
>                                          ^ executes
> 
> > + *    the high nice thread
> > + */
> > +
> > +#define _GNU_SOURCE
> > +#include <pthread.h>
> > +#include "tst_test.h"
> > +#include "tst_safe_pthread.h"
> > +
> > +#define LIMIT_TIME 3
> > +#define RUN_TIMES 1000000
> It might be useful if user could change this value with getopt switch (to
> debug on error to speedup). I also wonder if we hit timeout on some slow
> machine.
> 
> ...
> > +static void verify_nice(void)
> > +{
> > +	int ret;
> > +	int nice_inc_high = -1;
> > +	int nice_inc_low = -2;
> > +	pthread_t nice_low, nice_high;
> > +
> > +	ret = pthread_barrier_init(&barrier, NULL, 3);
> > +	if (ret != 0) {
> > +		tst_brk(TBROK, "pthread_barrier_init() returned %s",
> > +			tst_strerrno(-ret));
> > +	}
> Maybe just:
> 	if (pthread_barrier_init(&barrier, NULL, 3) != 0)
> 		tst_brk(TBROK | TERRNO, "pthread_barrier_init() failed");
> 
> Or feel free to use ret if you need it (see below), the point is with tst_brk()
> use TERRNO. Or am I missing something pthread specific?
> 
> > +	SAFE_PTHREAD_CREATE(&nice_high, NULL, nice_high_thread,
> (void*)&nice_inc_high);
> > +	SAFE_PTHREAD_CREATE(&nice_low, NULL, nice_low_thread,
> > +(void*)&nice_inc_low);
> > +
> > +	pthread_barrier_wait(&barrier);
> Not an expert on pthread, but IMHO you should compare
> PTHREAD_BARRIER_SERIAL_THREAD with the result of
> pthread_barrier_wait(), with your current code you compare with the
> result of pthread_barrier_init().
> > +	if (ret != 0 && ret != PTHREAD_BARRIER_SERIAL_THREAD)
> > +		tst_brk(TBROK | TERRNO, "pthread_barrier_wait() failed");
> > +
> > +	sleep(LIMIT_TIME);
> > +
> > +	ret = pthread_cancel(nice_high);
> > +	if(ret) {
> > +		tst_brk(TBROK, "pthread_cancel() returned %s",
> > +			tst_strerrno(-ret));
> if (pthread_cancel(nice_high))
> 	tst_brk(TBROK | TERRNO, "pthread_cancel() failed");
> 
> > +	}
> > +
> > +	ret = pthread_cancel(nice_low);
> > +	if(ret) {
> > +		tst_brk(TBROK, "pthread_cancel() returned %s",
> > +			tst_strerrno(-ret));
> and here as well.
> > +	}
> > +
> > +	ret = pthread_barrier_destroy(&barrier);
> > +	if (ret) {
> > +		tst_brk(TBROK, "pthread_barrier_destroy() returned %s",
> > +			tst_strerrno(-ret));
> > +	}
> > +
> > +	SAFE_PTHREAD_JOIN(nice_high, NULL);
> > +	SAFE_PTHREAD_JOIN(nice_low, NULL);
> > +
> > +	if (time_nice_low > time_nice_high) {
> > +		tst_res(TPASS, "time_nice_low = %lld time_nice_high = %lld",
> > +			time_nice_low, time_nice_high);
> > +	} else {
> > +		tst_brk(TFAIL | TTERRNO, "Test failed :"
> Wrong space between ':', missing after it, use:
> 		tst_brk(TFAIL | TTERRNO, "Test failed: "
> 
> 
> Kind regards,
> Petr
> 
> > +			"time_nice_low = %lld time_nice_high = %lld",
> > +			time_nice_low, time_nice_high);
> > +	}
> > +}
> > +
> > +static struct tst_test test = {
> > +	.setup = setup,
> > +	.cleanup = cleanup,
> > +	.test_all = verify_nice,
> > +	.needs_root = 1,
> > +};