* x86 rwsem: up_read() does not check active count in fast path
@ 2010-05-03 22:51 Michel Lespinasse
2010-05-04 12:41 ` David Howells
0 siblings, 1 reply; 3+ messages in thread
From: Michel Lespinasse @ 2010-05-03 22:51 UTC (permalink / raw
To: David Howells, Andrew Morton; +Cc: LKML
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.
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: x86 rwsem: up_read() does not check active count in fast path
2010-05-03 22:51 x86 rwsem: up_read() does not check active count in fast path Michel Lespinasse
@ 2010-05-04 12:41 ` David Howells
2010-05-04 22:38 ` Michel Lespinasse
0 siblings, 1 reply; 3+ messages in thread
From: David Howells @ 2010-05-04 12:41 UTC (permalink / raw
To: Michel Lespinasse; +Cc: dhowells, Andrew Morton, LKML
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: x86 rwsem: up_read() does not check active count in fast path
2010-05-04 12:41 ` David Howells
@ 2010-05-04 22:38 ` Michel Lespinasse
0 siblings, 0 replies; 3+ messages in thread
From: Michel Lespinasse @ 2010-05-04 22:38 UTC (permalink / raw
To: David Howells; +Cc: Andrew Morton, LKML
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-05-04 22:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-03 22:51 x86 rwsem: up_read() does not check active count in fast path Michel Lespinasse
2010-05-04 12:41 ` David Howells
2010-05-04 22:38 ` Michel Lespinasse
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.