Re: [PATCH v2] ipc/sem.c: fix lockup, restore FIFO behavior

[Manfred Spraul <manfred@colorfullife.com>]

[-- Attachment #1: Type: text/plain, Size: 3183 bytes --]

On 05/25/2013 08:32 PM, Davidlohr Bueso wrote:
> Yep, could you please explain what benefits you see in keeping FIFO order?
a) It's user space visible.

b) It's a well-defined behavior that might even make sense for some 
applications.
     Right now, a 2 semop operation with "+1, then -2" is priorized over 
a semop with "-1".

And: It doesn't cost much:
- no impact for users that use only single-op operations.
- no impact for users that use only multi-op operations
- for users that use both types: In the worst case some linked list 
splicing.

Actually, the code is probably faster because wait-for-zero ops are only 
scanned when the semaphore values are 0.

>> Acked-by: Rik van Riel <riel@redhat.com>
>>
>>> - simpler check_restart logic.
>>> - Efficient handling of wait-for-zero semops, both simple and complex.
>>> - Fewer restarts in update_queue(), because pending wait-for-zero do not
>>>     force a restart anymore.
>>>
>>> Other changes:
>>> - try_atomic_semop() also performs the semop. Thus rename the function.
>>>
>>> It passes tests with qemu, but not boot-tested due to EFI problems.
> I think this still needs a *lot* of testing - I don't have my Oracle
> workload available right now, but I will definitely see how this patch
> behaves on it. That said, I believe Oracle is are already quite happy
> with the sem improvements.
Ah, ok.
The change is good for one application and the risk of breaking other 
apps is considered as negligible.

>
> Furthermore, this patch is way too invasive for considering it for 3.10
> - I like Rik's patch better because it simply addresses the issue and
> nothing more.
I would disagree:
My patch is testable - with it applied, linux-3.0.10 should behave 
exactly as linux-3.0.9.
Except the scalability - the new sem_lock from Rik is great.

My problem with Rik's patch is that it is untestable:
It changes the behavior and we must hope that nothing breaks.

Actually, the latest patch makes it a bit worse:
> @@ -720,16 +718,11 @@ static int update_queue(struct sem_array *sma, int semnum, struct list_head *pt)
>   
>   		unlink_queue(sma, q);
>   
> -		if (error) {
> -			restart = 0;
> -		} else {
> -			semop_completed = 1;
> -			restart = check_restart(sma, q);
> -		}
> +		semop_completed = 1;
> +		if (check_restart(sma, q))
> +			*restart = 1;
>   
>   		wake_up_sem_queue_prepare(pt, q, error);
> -		if (restart)
> -			goto again;
If check_restart returns "1", then the current (3.0.10-rc1) code 
restarts immediately ("goto a again").
Now the rest of the queue is processed completely and only afterwards it 
is scanned again.

This means that wait-for-zero now succeeds only if a semaphore value 
stays zero.
For 3.0.9, it was sufficient if the value was temporarily zero.
Before the change, complex wait-for-zero would work, only simple 
wait-for-zero would be starved.
Now all operations are starved.

I've attached a test case:
     ./test5.sh
linux-3.0.9 completes all operations
With Rik's patch, the wait-for-zero remains running.

--
     Manfred

P.S.:
Btw, I found some code that uses a semop with 2 ops:
http://publib.boulder.ibm.com/infocenter/iseries/v5r3/index.jsp?topic=%2Fapis%2Fapiexusmem.htm

[-- Attachment #2: change.c --]
[-- Type: text/plain, Size: 1862 bytes --]

/*
 * Copyright (C) 1999 by Manfred Spraul.
 * 
 * Redistribution of this file is permitted under the terms of the GNU 
 * General Public License (GPL)
 * $Header: /home/manfred/cvs-tree/manfred/ipcsem/dec.c,v 1.5 2003/06/17 16:16:55 manfred Exp $
 */

#include <sys/types.h>
#include <sys/sem.h>
#include <unistd.h>
#include <stdlib.h>
#include <stdio.h>
#include <errno.h>
#include <string.h>
#include <assert.h>

#define TRUE	1
#define FALSE	0

union semun {
	int val;
	struct semid_ds *buf;
	unsigned short int *array;
	struct seminfo* __buf;
};


int main(int argc,char** argv)
{
	int id;
	int key;
	int res;
	int *nr;
	int *val;
	int i;
	int sems;

	printf("change <id> <semnum> <change> [...]\n");
	if(argc < 4 || ((argc % 2) == 1)) {
		printf("Invalid parameters.\n");
		return 1;
	}
	
	key = atoi(argv[1]);
	if(key == 0) {
		printf("Invalid parameters: Key invalid.\n");
		return 1;
	}
	if (key > 0) {
		id = semget(key,1,0);
		if(id == -1) {
			printf(" findkey() failed.\n");
			return 1;
		}
	} else {
		id = -key;
		printf(" findkey() bypassed, using id %d.\n", id);
	}
	sems = (argc-2)/2;
	nr=(int*)malloc(sizeof(int)*sems);
	val=(int*)malloc(sizeof(int)*sems);
	if (!nr || !val) {
		printf("Malloc failed.\n");
		return 1;
	}
	printf("pid %d: changing %d semaphores:\n",getpid(), sems);
	for (i=0;i<sems;i++) {
		nr[i] = atoi(argv[2+2*i]);
		val[i] = atoi(argv[3+2*i]);
		printf("       sem %3d by %2d\n",nr[i],val[i]);
	}


	{
		struct sembuf *sop;

		sop = malloc(sizeof(*sop)*sems);
		if (!sop) {
			printf("malloc() failed.\n");
			return 1;
		}
		for (i=0;i<sems;i++) {
			sop[i].sem_num=nr[i];
			sop[i].sem_op=val[i];
			sop[i].sem_flg=0;
		}

		res = semop(id,sop,sems);
		if(res==-1) {
			printf("pid %d: semop() failed, errno %d.\n", getpid(), errno);
			return 1;
		}
	}
	printf("pid %d: semop() successful.\n", getpid());
	return 0;
}

[-- Attachment #3: createary.c --]
[-- Type: text/plain, Size: 899 bytes --]

/*
 * Copyright (C) 1999 by Manfred Spraul.
 * 
 * Redistribution of this file is permitted under the terms of the GNU 
 * General Public License (GPL)
 * $Header: /home/manfred/cvs-tree/manfred/ipcsem/createary.c,v 1.3 2003/06/17 16:16:55 manfred Exp $
 */

#include <sys/sem.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>


int main(int argc,char** argv)
{
	int id, nsems, res, perms;

	printf("createary <id> <nsems> [<perms>]\n");
	if(argc < 3 || argc > 4) {
		printf("Invalid parameters.\n");
		return 1;
	}
	id = atoi(argv[1]);
	if(id <= 0) {
		printf("Invalid parameters.\n");
		return 1;
	}
	nsems = atoi(argv[2]);
	if(nsems <= 0) {
		printf("Invalid parameters.\n");
		return 1;
	}	
	if (argc > 3) {
		perms = atoi(argv[3]);
	} else {
		perms = 0777;
	}
	res = semget(id, nsems, perms | IPC_CREAT);
	if(res == -1) {
		printf(" create failed.\n");
		return 1;
	}
	return 0;
}

[-- Attachment #4: Makefile --]
[-- Type: text/plain, Size: 261 bytes --]

OUT_FILES=createary removeary change

CFLAGS = -Wall -g -O2 -pthread
LFLAGS = -static
#LFLAGS =

CC=gcc
CPP=g++

%:	%.cpp
	$(CPP) $(CFLAGS) -o $@ $< $(LFLAGS)

%:	%.c
	$(CC) $(CFLAGS) -o $@ $< $(LFLAGS)

all: $(OUT_FILES)

clean:
	rm -f $(OUT_FILES)
	rm -f *~


[-- Attachment #5: removeary.c --]
[-- Type: text/plain, Size: 900 bytes --]

/*
 * Copyright (C) 1999 by Manfred Spraul.
 * 
 * Redistribution of this file is permitted under the terms of the GNU 
 * General Public License (GPL)
 * $Header: /home/manfred/cvs-tree/manfred/ipcsem/removeary.c,v 1.3 2003/06/17 16:16:55 manfred Exp $
 */

#include <sys/sem.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>

union semun {
	int val;
	struct semid_ds *buf;
	unsigned short int *array;
	struct seminfo* __buf;
};

int main(int argc,char** argv)
{
	int id;
	int res;
	union semun arg;

	printf("removeary <id>\n");
	if(argc != 2) {
		printf("Invalid parameters.\n");
		return 1;
	}
	id = atoi(argv[1]);
	if(id <= 0) {
		printf("Invalid parameters.\n");
		return 1;
	}
	res = semget(id,1,0);
	if(res == -1) {
		printf(" open failed.\n");
		return 1;
	}
	id = res;
	res = semctl(id,1,IPC_RMID,arg);
	if(res == -1) {
		printf(" semctl failed.\n");
		return 1;
	}
return 0;
}

[-- Attachment #6: test5.sh --]
[-- Type: application/x-shellscript, Size: 514 bytes --]