Re: [LTP] [PATCH v2] Migrating the libhugetlbfs/testcases/alloc-instantiate-race.c test

[Petr Vorel <pvorel@suse.cz>]

Hi Samir,

Not a whole review, just few notes.

nit: missing static:
$ make check-hugemmap36
hugemmap36.c:28:15: warning: Symbol 'totpages' has no prototype or library ('tst_') prefix. Should it be static?
hugemmap36.c:91:6: warning: Symbol 'check_online_cpus' has no prototype or library ('tst_') prefix. Should it be static?

...
> diff --git a/runtest/hugetlb b/runtest/hugetlb
> index f294e9aaa..cc1e56f16 100644
> --- a/runtest/hugetlb
> +++ b/runtest/hugetlb
> @@ -35,6 +35,7 @@ hugemmap29 hugemmap29
>  hugemmap30 hugemmap30
>  hugemmap31 hugemmap31
>  hugemmap32 hugemmap32
> +hugemmap36 hugemmap36
You added hugemmap36 without -m ... parameter, but that fails.
I also wonder if we should test both private and shared or just one.
Also, if one more important, maybe it could be a default (thus no mandatory to
pass it with -m ...) and the other one optional.

Also I get warning for shared:

./hugemmap36 -m shared
tst_hugepage.c:84: TINFO: 2 hugepage(s) reserved
tst_tmpdir.c:316: TINFO: Using /tmp/LTP_hugL2gsLX as tmpdir (tmpfs filesystem)
tst_test.c:1100: TINFO: Mounting none to /tmp/LTP_hugL2gsLX/hugetlbfs fstyp=hugetlbfs flags=0
tst_test.c:1890: TINFO: LTP version: 20240930-63-g6408294d8
tst_test.c:1894: TINFO: Tested kernel: 6.12.0-rc6-1.gb3de43a-default #1 SMP PREEMPT_DYNAMIC Mon Nov  4 00:37:44 UTC 2024 (b3de43a) x86_64
tst_test.c:1725: TINFO: Timeout per run is 0h 00m 30s
hugemmap36.c:241: TINFO: Mapping 1/2 pages..
hugemmap36.c:213: TINFO: instantiating..
hugemmap36.c:132: TINFO: Mapping final page..
hugemmap36.c:145: TINFO: Child 1 status: 0

hugemmap36.c:149: TINFO: Child 2 status: 0

hugemmap36.c:219: TPASS: Test done
hugemmap36.c:251: TWARN: kill(3887,SIGKILL) failed: ESRCH (3)
hugemmap36.c:253: TWARN: kill(3888,SIGKILL) failed: ESRCH (3)

Test is failing when run more than once:
./hugemmap36 -m private -i2
tst_hugepage.c:84: TINFO: 2 hugepage(s) reserved
tst_tmpdir.c:316: TINFO: Using /tmp/LTP_hugBynfWl as tmpdir (tmpfs filesystem)
tst_test.c:1100: TINFO: Mounting none to /tmp/LTP_hugBynfWl/hugetlbfs fstyp=hugetlbfs flags=0
tst_test.c:1890: TINFO: LTP version: 20240930-63-g6408294d8
tst_test.c:1894: TINFO: Tested kernel: 6.12.0-rc6-1.gb3de43a-default #1 SMP PREEMPT_DYNAMIC Mon Nov  4 00:37:44 UTC 2024 (b3de43a) x86_64
tst_test.c:1725: TINFO: Timeout per run is 0h 00m 30s
hugemmap36.c:241: TINFO: Mapping 1/2 pages.. 
hugemmap36.c:213: TINFO: instantiating.. 
hugemmap36.c:132: TINFO: Mapping final page.. 
hugemmap36.c:219: TPASS: Test done
hugemmap36.c:213: TINFO: instantiating.. 
hugemmap36.c:105: TBROK: At least 2 online CPUs are required : ECHILD (10)

Ideally it'd allow to run test more times with -iN or -IN.

Could you please rebase your branch before posting a new version?
(conflict in the runtest and .gitignore)

> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap36.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap36.c
...
> +static int one_racer(void *p, int cpu,
> +		     volatile int *mytrigger, volatile int *othertrigger)
> +{
> +	volatile int *pi = p;
> +	cpu_set_t *cpuset;
> +	size_t mask_size;
> +	int err;
> +
> +	// Allocate CPU mask dynamically
> +	cpuset = CPU_ALLOC(cpu + 1);
> +	if (!cpuset)
> +		tst_brk(TBROK | TERRNO, "CPU_ALLOC() failed");
> +	// Get the required size for the allocated CPU set
nit: some comments for obvious things (like this one) are IMHO useless
> +	mask_size = CPU_ALLOC_SIZE(cpu + 1);
> +
> +	/* Split onto different CPUs to encourage the race */
> +	CPU_ZERO_S(mask_size, cpuset);
> +	CPU_SET_S(cpu, mask_size, cpuset);
> +	// Set CPU affinity using the allocated mask size
> +	err = sched_setaffinity(getpid(), mask_size, cpuset);
> +	if (err == -1)
> +		tst_brk(TBROK | TERRNO, "sched_setaffinity() failed");
> +	/* Ready */
> +	*mytrigger = 1;
> +	/* Wait for the other trigger to be set */
> +	while (!*othertrigger)
> +		;

Shouldn't be here at least minimal usleep() to avoid burning CPU?
Maybe not, because test is really fast.

> +	/* Set the shared value */
> +	*pi = 1;
> +	// Free the dynamically allocated CPU set
Do we need to document this?
> +	CPU_FREE(cpuset);
> +	return 0;
> +}
...

> +static void setup(void)
> +{
> +	totpages = SAFE_READ_MEMINFO(MEMINFO_HPAGE_FREE);
> +	hpage_size = tst_get_hugepage_size();
> +
> +	if (str_op)
> +		if (strcmp(str_op, "shared") == 0)
> +			race_type = MAP_SHARED;
> +		else if (strcmp(str_op, "private") == 0)
> +			race_type = MAP_PRIVATE;
> +		else
> +			tst_res(TFAIL, "Usage:mmap<private|shared>");
It should tst_brk(TBROK, ...) on invalid parameter:
1) tst_brk() to quit the testing immediately
2) TBROK is more suitable than TFAIL in this case.
I also find "Usage:mmap<private|shared>" misleading - I first tried
"mmapprivate".

> +	else
> +		tst_res(TFAIL, "Usage:mmap<private|shared>");
As I said, one could of these be a default.
> +
> +	fd_sync = tst_creat_unlinked(MNTPOINT, 0);
> +	/* Get a shared normal page for synchronization */
> +	q_sync = SAFE_MMAP(NULL, getpagesize(), PROT_READ|PROT_WRITE,
> +			MAP_SHARED|MAP_ANONYMOUS, -1, 0);
> +	tst_res(TINFO, "Mapping %ld/%ld pages.. ", totpages-1, totpages);
> +	p_sync = SAFE_MMAP(NULL, (totpages-1)*hpage_size, PROT_READ|PROT_WRITE,
> +			MAP_SHARED, fd_sync, 0);
> +}
> +
> +static void cleanup(void)
> +{
> +	if (fd_sync >= 0)
> +		SAFE_CLOSE(fd_sync);
> +	if (child1)
> +		SAFE_KILL(child1, SIGKILL);
> +	if (child2)
> +		SAFE_KILL(child2, SIGKILL);
> +}
> +
> +
> +static struct tst_test test = {
> +	.options = (struct  tst_option[]){
> +		{"m:", &str_op, "Usage:mmap<private|shared>"},
maybe:
		{"m:", &str_op, "Type of mmap() mapping <private|shared>"},

(see ./hugemmap36 -h for whole output)

Kind regards,
Petr

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