Re: [LTP] [PATCH v1] starvation.c: New case for sched starvation

[Petr Vorel <pvorel@suse.cz>]

Hi Wei,

could you please fix warnings? Both should be trivial.

starvation.c: In function ‘child’:
starvation.c:70:1: warning: no return statement in function returning non-void [-Wreturn-type]
   70 | }
      | ^
In file included from ../../../../include/tst_test.h:110,
                 from starvation.c:22:
starvation.c: In function ‘do_test’:
../../../../include/tst_safe_macros.h:493:51: warning: passing argument 4 of ‘safe_signal’ from incompatible pointer type [-Wincompatible-pointer-types]
  493 |         safe_signal(__FILE__, __LINE__, (signum), (handler))
      |                                                   ^~~~~~~~~
      |                                                   |
      |                                                   void (*)(void)
starvation.c:81:9: note: in expansion of macro ‘SAFE_SIGNAL’
   81 |         SAFE_SIGNAL(SIGUSR1, handler);
      |         ^~~~~~~~~~~
../../../../include/tst_safe_macros.h:490:34: note: expected ‘sighandler_t’ but argument is of type ‘void (*)(void)’
  490 |         int signum, sighandler_t handler);
      |                     ~~~~~~~~~~~~~^~~~~~~

make check-starvation
make -C "/home/pevik/install/src/ltp.git/tools/sparse" all
...
CHECK testcases/kernel/sched/cfs-scheduler/starvation.c
starvation.c:80:9: warning: incorrect type in argument 4 (different argument counts)
starvation.c:80:9:    expected void ( *[usertype] handler )( ... )
starvation.c:80:9:    got void ( * )( ... )


> Signed-off-by: Wei Gao <wegao@suse.com>

> Add scheduller thread starvation test case base following link:
> https://lwn.net/ml/linux-kernel/9fd2c37a05713c206dcbd5866f67ce779f315e9e.camel@gmx.de/
nit: we should use lore, not lwn:

https://lore.kernel.org/lkml/9fd2c37a05713c206dcbd5866f67ce779f315e9e.camel@gmx.de/

...
> +++ b/testcases/kernel/sched/cfs-scheduler/starvation.c
> @@ -0,0 +1,97 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/* Copyright 2023 Mike Galbraith <efault-AT-gmx.de> */
You can add your credit for porting to LTP :).

> +/*\
> + *
> + * [Description]
> + *
> + * Thread starvation test.
> + * This case copy from following link:
> + * https://lwn.net/ml/linux-kernel/9fd2c37a05713c206dcbd5866f67ce779f315e9e.camel@gmx.de/
https://lore.kernel.org/lkml/9fd2c37a05713c206dcbd5866f67ce779f315e9e.camel@gmx.de/
> + *
Please avoid extra blank line here.
> + */
> +
> +#define _GNU_SOURCE
> +#include <stdio.h>
> +#include <signal.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <stdlib.h>
> +#include <sched.h>
> +
> +#include "tst_test.h"
> +
> +static unsigned long loop = 10000000;
It'd be nice to have option to run less iterations.
> +
> +static int wait_for_pid(pid_t pid)
> +{
> +	int status, ret;
> +
> +again:
> +	ret = waitpid(pid, &status, 0);
> +	if (ret == -1) {
> +		if (errno == EINTR)
> +			goto again;
> +
> +		return -1;
> +	}
> +
> +	if (WIFSIGNALED(status))
> +		return 0;
> +
> +	return -1;
> +}
I'm not sure by this part. I'd be good if somebody else reviewed it.
We cannot use tst_reap_children(), because we want to be signaled.

> +
> +static void setup(void)
> +{
> +	cpu_set_t mask;
> +
> +	CPU_ZERO(&mask);
> +
> +	CPU_SET(0, &mask);
> +
> +	TST_EXP_POSITIVE(sched_setaffinity(0, sizeof(mask), &mask));
> +}
> +
> +static void handler(void)
> +{
> +	if (loop > 0)
> +		--loop;
> +}
> +
> +static int child(void)
> +{
> +	pid_t ppid = getppid();
> +
> +	TST_CHECKPOINT_WAIT(0);
> +
> +	while (1)
> +		kill(ppid, SIGUSR1);
SAFE_KILL(ppid, SIGUSR1) ?
> +}
> +
> +static void do_test(void)
> +{
> +	pid_t child_pid;
> +
> +	child_pid = SAFE_FORK();
> +
> +	if (!child_pid)
> +		child();
> +
> +	SAFE_SIGNAL(SIGUSR1, handler);
> +	TST_CHECKPOINT_WAKE(0);
> +
> +	while (loop)
> +		sleep(1);
> +
> +	kill(child_pid, SIGTERM);
> +	TST_EXP_PASS(wait_for_pid(child_pid));
> +}
> +
> +static struct tst_test test = {
> +	.test_all = do_test,
> +	.setup = setup,
> +	.forks_child = 1,
> +	.needs_checkpoints = 1,
> +	.max_runtime = 120,
Are you sure 2 min is enough? Maybe we need to use tst_remaining_runtime() to
check if we're not running out of time.
Also, if we set getopt to choose number of options, we'd need to adjust it by
tst_set_max_runtime().

Kind regards,
Petr

> +};

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