All of lore.kernel.org
 help / color / mirror / Atom feed
From: David Howells <dhowells@redhat.com>
To: Ingo Molnar <mingo@redhat.com>
Cc: torvalds@osdl.org, akpm@osdl.org, linux-kernel@vger.kernel.org
Subject: R/W semaphore changes
Date: Tue, 04 Jul 2006 13:47:42 +0100	[thread overview]
Message-ID: <14683.1152017262@warthog.cambridge.redhat.com> (raw)


I see you've made some R/W semaphore changes...

| /*
|  * nested locking:
|  */

This comment is inadequate.  Please be more explicit about when you're allowed
to do this.

| extern void down_read_nested(struct rw_semaphore *sem, int subclass);
| extern void down_write_nested(struct rw_semaphore *sem, int subclass);

Please, please, please don't.  R/W semaphores are _not_ permitted to nest.
That way lies deadlock.

| /*
|  * Take/release a lock when not the owner will release it:
|  */
| extern void down_read_non_owner(struct rw_semaphore *sem);
| extern void up_read_non_owner(struct rw_semaphore *sem);

Does that mean that the owner may not then release the semaphore?  I presume
not, but it's not entirely clear.

| # define down_read_nested(sem, subclass)		down_read(sem)
| # define down_write_nested(sem, subclass)	down_write(sem)

This is _not_ okay.


I also notice that you've out-of-lined the rwsems debugging wrappers in all
situations.  Please don't.  On some archs this is going to slow things down.
On FRV for example, this is converted to:

	0xc00437f0 <down_read+0>:	addi sp,-16,sp
	0xc00437f4 <down_read+4>:	sti fp,@(sp,0)
	0xc00437f8 <down_read+8>:	ori sp,0,fp
	0xc00437fc <down_read+12>:	movsg lr,gr5
	0xc0043800 <down_read+16>:	sti gr5,@(fp,8)
	0xc0043804 <down_read+20>:	call 0xc01ea910 <__down_read>
	0xc0043808 <down_read+24>:	ldi @(fp,8),gr5
	0xc004380c <down_read+28>:	ld @(fp,gr0),fp
	0xc0043810 <down_read+32>:	addi sp,16,sp
	0xc0043814 <down_read+36>:	jmpl @(gr5,gr0)

When previously __down_read would be called directly...  This applies to every
arch that uses the spinlock-based R/W sems.  In FRV's case, you've required
the loading of extra icache lines to no useful purpose.  Now I do have GDB
stub compiled in, so normally it would be shorter than this as it wouldn't
normally save the frame pointer, and would probably tail-call.

If you must put your annotations somewhere, please place them in lib/rwsem*.c
directly.

Please _only_ out-of-line the rwsem debugging wrappers when the appropriate
config options are set.  It is reasonable to do it then - it is debugging code
after all, and therefore not enabled for normal operation.

Oh, and if you're going to out-of-line the rwsem debugging wrappers like this,
please remove all the then-redundant PUSHL instructions from the inline asm in
include/asm-i386/rwsem.h.  They're then no longer required as there's no need
to save registers that are callee-clobbered, since you're marking them
clobbered anyway by interpolating an extra function call.

             reply	other threads:[~2006-07-04 12:47 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-07-04 12:47 David Howells [this message]
2006-07-04 12:52 ` R/W semaphore changes Arjan van de Ven
2006-07-04 13:05   ` David Howells
2006-07-04 13:17     ` Arjan van de Ven
2006-07-04 13:21     ` Ingo Molnar
2006-07-04 13:33       ` [patch] lockdep: add more rwsem.h documentation Ingo Molnar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=14683.1152017262@warthog.cambridge.redhat.com \
    --to=dhowells@redhat.com \
    --cc=akpm@osdl.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=torvalds@osdl.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.