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

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

[Pekka Enberg]

On Sat, Jun 26, 2010 at 12:20 AM, 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]

Is this related to the SLUB patches? Regardless, lets add Andrew and
linux-kernel on CC.

> 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>
>
> ---
>  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;
> +}
> +
> +
>  SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
>                unsigned, nsops, const struct timespec __user *, timeout)
>  {
> @@ -1409,11 +1435,7 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
>        else
>                schedule();
>
> -       error = queue.status;
> -       while(unlikely(error == IN_WAKEUP)) {
> -               cpu_relax();
> -               error = queue.status;
> -       }
> +       error = get_queue_result(&queue);
>
>        if (error != -EINTR) {
>                /* fast path: update_queue already obtained all requested
> @@ -1427,10 +1449,12 @@ SYSCALL_DEFINE4(semtimedop, int, semid, struct sembuf __user *, tsops,
>                goto out_free;
>        }
>
> +       error = get_queue_result(&queue);
> +
>        /*
>         * If queue.status != -EINTR we are woken up by another process
>         */
> -       error = queue.status;
> +
>        if (error != -EINTR) {
>                goto out_unlock_free;
>        }
> --
> 1.7.0.1
>
>
> --
> 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>
>

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

[Christoph Lameter]

This is a patch from Manfred. Required to make 2.6.35-rc3 work.

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

[Manfred Spraul]

Hi Andrew,

On 06/29/2010 09:08 PM, Andrew Morton wrote:
> On Tue, 29 Jun 2010 10:42:42 -0500 (CDT)
> Christoph Lameter<cl@linux-foundation.org>  wrote:
>
>    
>> This is a patch from Manfred. Required to make 2.6.35-rc3 work.
>>
>>      
> My current version of the patch is below.
>
> I believe that Luca has still seen problems with this patch applied so
> its current status is "stuck, awaiting developments".
>
> Is that a correct determination?
>    

I would propose that you forward a patch to Linus - either the one you 
have in your tree or the v2 that I've just posted.
With stock 2.6.35-rc3, my semtimedop() stress tests produces an oops or 
an invalid return value (i.e.:semtimedop() returns with "1") within a 
fraction of a second.

With either of the patches applied, my test apps show the expected behavior.

--
     Manfred

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

[Andrew Morton]

On Wed, 30 Jun 2010 21:38:43 +0200
Manfred Spraul <manfred@colorfullife.com> wrote:

> Hi Andrew,
> 
> On 06/29/2010 09:08 PM, Andrew Morton wrote:
> > On Tue, 29 Jun 2010 10:42:42 -0500 (CDT)
> > Christoph Lameter<cl@linux-foundation.org>  wrote:
> >
> >    
> >> This is a patch from Manfred. Required to make 2.6.35-rc3 work.
> >>
> >>      
> > My current version of the patch is below.
> >
> > I believe that Luca has still seen problems with this patch applied so
> > its current status is "stuck, awaiting developments".
> >
> > Is that a correct determination?
> >    
> 
> I would propose that you forward a patch to Linus - either the one you 
> have in your tree or the v2 that I've just posted.

OK, I added the incremental change:

--- a/ipc/sem.c~ipc-semc-bugfix-for-semop-not-reporting-successful-operation-update
+++ a/ipc/sem.c
@@ -1440,7 +1440,14 @@ SYSCALL_DEFINE4(semtimedop, int, semid, 
 
 	if (error != -EINTR) {
 		/* fast path: update_queue already obtained all requested
-		 * resources */
+		 * resources.
+		 * Perform a smp_mb(): User space could assume that semop()
+		 * is a memory barrier: Without the mb(), the cpu could
+		 * speculatively read in user space stale data that was
+		 * overwritten by the previous owner of the semaphore.
+		 */
+		smp_mb();
+
 		goto out_free;
 	}
 
_

> With stock 2.6.35-rc3, my semtimedop() stress tests produces an oops or 
> an invalid return value (i.e.:semtimedop() returns with "1") within a 
> fraction of a second.
> 
> With either of the patches applied, my test apps show the expected behavior.

OK, I'll queue it up.