x86 rwsem: up_read() does not check active count in fast path

x86 rwsem: up_read() does not check active count in fast path

[Michel Lespinasse]

Hi,

Looking at the x86 rwsem code, I have been wondering about the up_read() path.
__rwsem_do_wake() comment mentions that one should check the active count on
the way there; however I could not find that check when coming from up_read().

If there are multiple read locks active on a given semaphore, and there is
a write lock queued, each read lock will go:
__up_read
-> notice sem value is <0
-> rwsem_wake
-> take spinlock
-> add 1 in __rwsem_do_wake
-> notice there were other active lockers
-> substract 1 again under 'undo'
-> release spinlock

This could be avoided by adding a check for the active count in __up_write(),
as in the following untested patch:

diff --git a/arch/x86/include/asm/rwsem.h b/arch/x86/include/asm/rwsem.h
index 606ede1..915941f 100644
--- a/arch/x86/include/asm/rwsem.h
+++ b/arch/x86/include/asm/rwsem.h
@@ -200,11 +200,18 @@ static inline void __up_read(struct rw_semaphore *sem)
 		     LOCK_PREFIX "  xadd      %1,(%2)\n\t"
 		     /* subtracts 1, returns the old value */
 		     "  jns        1f\n\t"
+#ifdef CONFIG_X86_64
+		     "  dec        %%edx\n\t" /* %edx = low 32 bits of %1 */
+#else
+		     "  dec        %1\n\t"
+		     "  and        %4,%1\n\t"
+#endif
+		     "  jnz        1f\n\t"
 		     "  call call_rwsem_wake\n"
 		     "1:\n"
 		     "# ending __up_read\n"
 		     : "+m" (sem->count), "=d" (tmp)
-		     : "a" (sem), "1" (tmp)
+		     : "a" (sem), "1" (tmp), "i" (RWSEM_ACTIVE_MASK)
 		     : "memory", "cc");
 }
 
Is there any reason not to do this ? I have to mention I don't have any
specific workload in mind that might benefit from this; I'm only sending this
because the code contradicts the __rwsem_do_wake() comment.

Thanks,

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.

Re: x86 rwsem: up_read() does not check active count in fast path

[David Howells]

Michel Lespinasse <walken@google.com> wrote:

> Looking at the x86 rwsem code, I have been wondering about the up_read()
> path.  __rwsem_do_wake() comment mentions that one should check the active
> count on the way there; however I could not find that check when coming from
> up_read().

Look in call_rwsem_wake(), which is implemented in assembly in two places in
the x86 arch:

	arch/x86/lib/rwsem_64.S
	arch/x86/lib/semaphore_32.S

Note that the x86_64 version contains a bug, a patch for which is attached.

David
---
From: David Howells <dhowells@redhat.com>
Subject: [PATCH] Fix the x86_64 implementation of call_rwsem_wait()

The x86_64 call_rwsem_wait() treats the active state counter part of the R/W
semaphore state as being 16-bit when it's actually 32-bit (it's half of the
64-bit state).  It should do "decl %edx" not "decw %dx".

Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/x86/lib/rwsem_64.S |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)


diff --git a/arch/x86/lib/rwsem_64.S b/arch/x86/lib/rwsem_64.S
index 15acecf..41fcf00 100644
--- a/arch/x86/lib/rwsem_64.S
+++ b/arch/x86/lib/rwsem_64.S
@@ -60,7 +60,7 @@ ENTRY(call_rwsem_down_write_failed)
 	ENDPROC(call_rwsem_down_write_failed)
 
 ENTRY(call_rwsem_wake)
-	decw %dx    /* do nothing if still outstanding active readers */
+	decl %edx	/* do nothing if still outstanding active readers */
 	jnz 1f
 	save_common_regs
 	movq %rax,%rdi

Re: x86 rwsem: up_read() does not check active count in fast path

[Michel Lespinasse]

On Tue, May 4, 2010 at 5:41 AM, David Howells <dhowells@redhat.com> wrote:
> Michel Lespinasse <walken@google.com> wrote:
>
>> Looking at the x86 rwsem code, I have been wondering about the up_read()
>> path.  __rwsem_do_wake() comment mentions that one should check the active
>> count on the way there; however I could not find that check when coming from
>> up_read().
>
> Look in call_rwsem_wake(), which is implemented in assembly in two places in
> the x86 arch:
>
>        arch/x86/lib/rwsem_64.S
>        arch/x86/lib/semaphore_32.S

Gah thanks, I had totally missed that part (I knew about these files
but I had taken these as pure ABI wrappers; I did not notice the one
nugget of interesting logic hiding in there).

BTW, what's the reason for doing this check in call_rwsem_wake ? The
check is entirely redundant in the __up_write() case; and in
__up_read() things might be less confusing if edx was used locally
rather than in a separate function.

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.