Messages in this thread | | | Date | Sat, 25 May 2013 22:00:17 +0200 | From | Manfred Spraul <> | Subject | Re: [PATCH v2] ipc/sem.c: fix lockup, restore FIFO behavior |
| |
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 /* * 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; } /* * 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; } 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 *~
/* * 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; } [unhandled content-type:application/x-shellscript] | |