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

From: Manfred Spraul
Date: Sat May 25 2013 - 16:00:24 EST

Next message: Wim Van Sebroeck: "Re: [PATCH RESEND 2/5] watchdog: imx2_wdt: use devm_clk_get()"
Previous message: Rafael J. Wysocki: "[GIT PULL] Power management and ACPI fixes for v3.10-rc3"
In reply to: Davidlohr Bueso: "Re: [PATCH v2] ipc/sem.c: fix lockup, restore FIFO behavior"
Next in thread: Mike Galbraith: "Re: [PATCH v2] ipc/sem.c: fix lockup, restore FIFO behavior"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>

- 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;
}

Attachment: test5.sh
Description: application/shellscript

Next message: Wim Van Sebroeck: "Re: [PATCH RESEND 2/5] watchdog: imx2_wdt: use devm_clk_get()"
Previous message: Rafael J. Wysocki: "[GIT PULL] Power management and ACPI fixes for v3.10-rc3"
In reply to: Davidlohr Bueso: "Re: [PATCH v2] ipc/sem.c: fix lockup, restore FIFO behavior"
Next in thread: Mike Galbraith: "Re: [PATCH v2] ipc/sem.c: fix lockup, restore FIFO behavior"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]