Re: [S+Q 01/16] [PATCH] ipc/sem.c: Bugfix for semop() not reporting successful operation

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

On Mon, 28 Jun 2010 18:45:25 +0200
Manfred Spraul <manfred@colorfullife.com> wrote:

> On 06/28/2010 04:17 AM, KAMEZAWA Hiroyuki wrote:
> > On Fri, 25 Jun 2010 16:20:27 -0500
> > Christoph Lameter<cl@linux-foundation.org>  wrote:
> >
> >    
> >> [Necessary to make 2.6.35-rc3 not deadlock. Not sure if this is the "right"(tm)
> >> fix]
> >>
> >> The last change to improve the scalability moved the actual wake-up out of
> >> the section that is protected by spin_lock(sma->sem_perm.lock).
> >>
> >> This means that IN_WAKEUP can be in queue.status even when the spinlock is
> >> acquired by the current task. Thus the same loop that is performed when
> >> queue.status is read without the spinlock acquired must be performed when
> >> the spinlock is acquired.
> >>
> >> Signed-off-by: Manfred Spraul<manfred@colorfullife.com>
> >> Signed-off-by: Christoph Lameter<cl@linux-foundation.org>
> >>      
> >
> > Hmm, I'm sorry if I don't understand the code...
> >
> >    
> >> ---
> >>   ipc/sem.c |   36 ++++++++++++++++++++++++++++++------
> >>   1 files changed, 30 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/ipc/sem.c b/ipc/sem.c
> >> index 506c849..523665f 100644
> >> --- a/ipc/sem.c
> >> +++ b/ipc/sem.c
> >> @@ -1256,6 +1256,32 @@ out:
> >>   	return un;
> >>   }
> >>
> >> +
> >> +/** get_queue_result - Retrieve the result code from sem_queue
> >> + * @q: Pointer to queue structure
> >> + *
> >> + * The function retrieve the return code from the pending queue. If
> >> + * IN_WAKEUP is found in q->status, then we must loop until the value
> >> + * is replaced with the final value: This may happen if a task is
> >> + * woken up by an unrelated event (e.g. signal) and in parallel the task
> >> + * is woken up by another task because it got the requested semaphores.
> >> + *
> >> + * The function can be called with or without holding the semaphore spinlock.
> >> + */
> >> +static int get_queue_result(struct sem_queue *q)
> >> +{
> >> +	int error;
> >> +
> >> +	error = q->status;
> >> +	while(unlikely(error == IN_WAKEUP)) {
> >> +		cpu_relax();
> >> +		error = q->status;
> >> +	}
> >> +
> >> +	return error;
> >> +}
> >>      
> > no memory barrier is required ?
> >
> >    
> No.
> q->status is the only field that is read in the exit path of 
> sys_semtimedop():
> After that, q->status is used as the return value of sys_semtimedop(), 
> without accessing any other field.
> Thus no memory barrier is required: there is just no other read/write 
> operation against which the read of q->status must be serialized.
> 
> There is a smp_wmb() wake_up_sem_queue_do(), to ensure that all writes 
> that are done by the cpu that does the wake-up are completed before 
> q->status is set to the final value.
> 

Thanks. BTW, cpu_relax() always includes asm("":::"memory") for avoiding
optimization ?

Thanks,
-Kame



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>