Re: [LTP] semctl01: tolerate SEM_STAT races under parallel LTP runs

[linuxtestproject.agent@gmail.com]

Hi Stephen,

On Tue, 28 Jul 2026 12:23:50 -0400, Stephen Bertram wrote:
> semctl01: tolerate SEM_STAT races under parallel LTP runs

> When multiple LTP workers run IPC tests concurrently, SEM_STAT can fail
> with EIDRM/EINVAL because the index from IPC_INFO can disappear before
> SEM_STAT runs. Retry a few times instead of treating that as TBROK.

The TBROK is still there though, see do_sem_stat() below, so the message
promises a bit more than the code delivers.

> The test remains single-threaded; shared globals are intentional.

This reads like an answer to review feedback rather than something a
future reader of the git log needs. Could it be dropped?

> +static int try_sem_stat(union semun *arg)
> +{
> +	int info_id = 0;
> +	int idx;
> +
> +	idx = SAFE_SEMCTL(info_id, 0, IPC_INFO, (union semun)&ipc_buf);
> +	return semctl(idx, 0, SEM_STAT, *arg);
> +}

idx here is ipc_get_maxidx(), i.e. the highest in-use index in the whole
namespace. That is almost never the set this test created - it is whatever
set some other process happens to own at that moment.

So the race is not really being tolerated, it is being retried against a
moving target. Would it be better to look up the index of the test's own
set instead, e.g. walk 0..max_idx with SEM_STAT until the returned id
equals sem_id? That makes the whole sequence deterministic and removes the
need for a retry loop.

There is also EACCES to consider: if the highest index belongs to another
uid, ipcperms() in semctl_stat() rejects it and sem_stat_succeeded() goes
straight to TBROK. On a shared machine that is the same class of spurious
failure the patch is trying to remove.

info_id is always 0 and semctl_info() ignores the semid argument entirely,
so the variable does not carry any information. Passing sem_id would at
least match the rest of the file.

ipc_buf is the global that the IPC_INFO and SEM_INFO test cases point at
through tc->arg. Refilling it from a helper for an unrelated command is a
hidden side effect - a local struct seminfo would keep it contained.

> +static int do_sem_stat(union semun arg)
> +{
> +	int ret;
> +
> +	ret = TST_RETRY_FUNC(try_sem_stat(&arg), sem_stat_succeeded);
> +	if (ret < 0)
> +		tst_brk(TBROK | TERRNO,
> +			"semctl(SEM_STAT) still failing after retries");
> +
> +	return ret;
> +}

TST_RETRY_FUNC() backs off for roughly one second in total and then just
returns the last value, so this path ends in exactly the TBROK the commit
message says is being removed. Under sustained parallel IPC churn the
original failure mode is still reachable, only less likely. 8000 clean runs
show the window got smaller, not that it closed.

>  static void func_sstat(int semidx)
>  {
>  	if (semidx >= 0)

Since do_sem_stat() already brk's on anything negative, this arm can only
ever be TPASS - the test case cannot fail any more. Comparing semidx
against sem_id would give it something real to verify, and it falls out
naturally from the index lookup suggested above.

>  static void func_iinfo(int hidx)
>  {
>  	if (hidx >= 0) {
> -		sem_index = hidx;
>  		tst_res(TPASS, "the highest index is correct");
>  	} else {
> -		sem_index = 0;
>  		tst_res(TFAIL, "the highest index is incorrect");
>  	}
>  }

Both arms are single statements now, so this trips checkpatch:

  semctl01.c:212: WARNING: braces {} are not necessary for any arm of this
  statement

It is not present on master, "make check-semctl01" flags it after the
patch.

> +
>  	if (tc->func_setup) {

> -
>  	if (tc->cmd == GETNCNT || tc->cmd == GETZCNT)

Both hunks are pure whitespace churn unrelated to the fix, and the second
one drops the separation before the cleanup check. Could they be left
alone?

> -	rval = SAFE_SEMCTL(*(tc->semid), tc->semnum, tc->cmd, tc->arg);
> -	switch (tc->cmd) {
> +	if (tc->cmd == SEM_STAT) {
> +		rval = do_sem_stat(tc->arg);
> +		tc->func_test(rval);
> +	} else {
> +		rval = SAFE_SEMCTL(sem_id, tc->semnum, tc->cmd, tc->arg);
> +		switch (tc->cmd) {

The switch gets duplicated for one command. Selecting only the call and
leaving the switch alone keeps the diff to a few lines:

	if (tc->cmd == SEM_STAT)
		rval = do_sem_stat(tc->arg);
	else
		rval = SAFE_SEMCTL(sem_id, tc->semnum, tc->cmd, tc->arg);

	switch (tc->cmd) {
	...
	}

Verdict - Needs revision

---
Note:

The agent can sometimes produce false positives although often its
findings are genuine. If you find issues with the review, please
comment this email or ignore the suggestions.

Regards,
LTP AI Reviewer

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