lkml.org 
[lkml]   [2013]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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]
\
 
 \ /
  Last update: 2013-05-25 22:21    [W:0.512 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site