Re: [PATCH 6/6] ipc/sem.c: make semctl(,,{GETNCNT,GETZCNT}) standard compliant

Davidlohr Bueso
2014-05-14T14:52:44Z

On Sat, 2014-05-10 at 12:03 +0200, Manfred Spraul wrote:
> Per definition, a task waits on exactly one semaphore:
> The semaphore from the first operation in the sop array that cannot proceed.
>
> The Linux implementation never followed the standard, it tried to count all
> semaphores that might be the reason why a task sleeps.
>
> This patch fixes that.
>
> Note:
> The implementation assumes that GETNCNT and GETZCNT are rare operations,
> therefore the code counts them only on demand.
> (If they wouldn't be rare, then the non-compliance would have
> been found earlier)
>
> Signed-off-by: Manfred Spraul <manfred@colorfullife.com>
> ---
> ipc/sem.c | 37 ++++++++++++++++---------------------
> 1 file changed, 16 insertions(+), 21 deletions(-)
>
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 22a4c12..5e8bcde 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -993,38 +993,33 @@ static void do_smart_update(struct sem_array *sma, struct sembuf *sops, int nsop
> }
>
> /*
> - * check_qop: Test how often a queued operation sleeps on the semaphore semnum
> + * check_qop: Test if a queued operation sleeps on the semaphore semnum
> */
> static int check_qop(struct sem_array *sma, int semnum, struct sem_queue *q,
> bool count_zero)
> {
> - struct sembuf *sops = q->sops;
> - int nsops = q->nsops;
> - int i, semcnt;
> + struct sembuf *sop = q->blocking;
>
> - semcnt = 0;
> + BUG_ON(sop->sem_flg & IPC_NOWAIT);
> + BUG_ON(sop->sem_op > 0);

Hmm in light of Linus' recent criticism about randomly sprinkling
BUG_ONs in the kernel I'm not sure we want this. Yes, all those calls
are correct from a logical pov and should never occur, however, would
WARN be more suitable instead? I don't know.

Andrew, any thoughts?

Thanks.