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

[Petr Vorel <pvorel@suse.cz>]

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,
> +};

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