linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Introduce down_try() so we can move away from down_trylock()
@ 2008-07-29  0:15 Rusty Russell
  2008-07-29  0:27 ` Paul Menage
  0 siblings, 1 reply; 17+ messages in thread
From: Rusty Russell @ 2008-07-29  0:15 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: linux-kernel, Matthew Wilcox, Randy.Dunlap, Andrew Morton,
	Christoph Hellwig

I planned on removing the much-disliked down_trylock() (with its
backwards return codes) in 2.6.27, but it's creating something of a
logjam with other patches in -mm and linux-next.

Andrew suggested introducing "down_try" as a wrapper now, to make
the transition easier.  Linus, please apply.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Matthew Wilcox <matthew@wil.cx>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
---
 include/linux/semaphore.h |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff -r 92664ae4130b include/linux/semaphore.h
--- a/include/linux/semaphore.h	Wed May 21 14:54:40 2008 +1000
+++ b/include/linux/semaphore.h	Wed May 21 15:07:31 2008 +1000
@@ -48,4 +48,18 @@ extern int __must_check down_timeout(str
 extern int __must_check down_timeout(struct semaphore *sem, long jiffies);
 extern void up(struct semaphore *sem);
 
+/**
+ * down_try - try to down a semaphore, but don't block
+ * @sem: the semaphore
+ *
+ * This is equivalent to down_trylock(), but has the same return codes as
+ * spin_trylock and mutex_trylock: 1 if semaphore acquired, 0 if not.
+ *
+ * down_trylock() with its confusing return codes will be deprecated
+ * soon.  It will not be missed.
+ */
+static inline int __must_check down_try(struct semaphore *sem)
+{
+        return !down_trylock(sem);
+}
 #endif /* __LINUX_SEMAPHORE_H */

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-07-29  0:15 [PATCH] Introduce down_try() so we can move away from down_trylock() Rusty Russell
@ 2008-07-29  0:27 ` Paul Menage
  2008-07-29 13:01   ` Rusty Russell
  0 siblings, 1 reply; 17+ messages in thread
From: Paul Menage @ 2008-07-29  0:27 UTC (permalink / raw)
  To: Rusty Russell
  Cc: Linus Torvalds, linux-kernel, Matthew Wilcox, Randy.Dunlap,
	Andrew Morton, Christoph Hellwig

On Mon, Jul 28, 2008 at 5:15 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> I planned on removing the much-disliked down_trylock() (with its
> backwards return codes) in 2.6.27, but it's creating something of a
> logjam with other patches in -mm and linux-next.
>
> Andrew suggested introducing "down_try" as a wrapper now, to make
> the transition easier.  Linus, please apply.
>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Matthew Wilcox <matthew@wil.cx>
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
>  include/linux/semaphore.h |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff -r 92664ae4130b include/linux/semaphore.h
> --- a/include/linux/semaphore.h Wed May 21 14:54:40 2008 +1000
> +++ b/include/linux/semaphore.h Wed May 21 15:07:31 2008 +1000
> @@ -48,4 +48,18 @@ extern int __must_check down_timeout(str
>  extern int __must_check down_timeout(struct semaphore *sem, long jiffies);
>  extern void up(struct semaphore *sem);
>
> +/**
> + * down_try - try to down a semaphore, but don't block
> + * @sem: the semaphore
> + *
> + * This is equivalent to down_trylock(), but has the same return codes as
> + * spin_trylock and mutex_trylock: 1 if semaphore acquired, 0 if not.

Is there a reason to avoid using a return type of "bool" for this?

Paul

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-07-29  0:27 ` Paul Menage
@ 2008-07-29 13:01   ` Rusty Russell
  2008-07-29 16:21     ` Randy Dunlap
  2008-08-01 17:26     ` Linus Torvalds
  0 siblings, 2 replies; 17+ messages in thread
From: Rusty Russell @ 2008-07-29 13:01 UTC (permalink / raw)
  To: Paul Menage
  Cc: Linus Torvalds, linux-kernel, Matthew Wilcox, Randy.Dunlap,
	Andrew Morton, Christoph Hellwig

On Tuesday 29 July 2008 10:27:15 Paul Menage wrote:
> On Mon, Jul 28, 2008 at 5:15 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > +/**
> > + * down_try - try to down a semaphore, but don't block
> > + * @sem: the semaphore
>
> Is there a reason to avoid using a return type of "bool" for this?

Probably, but not a good one.

This incorporates Andrew's whitespace fix as well (ie:
introduce-down_try-so-we-can-move-away-from-down_trylock.patch and
introduce-down_try-so-we-can-move-away-from-down_trylock-checkpatch-fixes.patch)

Introduce down_try()

I planned on removing the much-disliked down_trylock() (with its
backwards return codes) in 2.6.27, but it's creating something of a
logjam with other patches in -mm and linux-next.

Andrew suggested introducing "down_try" as a wrapper now, to make
the transition easier.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Matthew Wilcox <matthew@wil.cx>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
---
 include/linux/semaphore.h |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff -r 92664ae4130b include/linux/semaphore.h
--- a/include/linux/semaphore.h	Wed May 21 14:54:40 2008 +1000
+++ b/include/linux/semaphore.h	Wed May 21 15:07:31 2008 +1000
@@ -48,4 +48,18 @@ extern int __must_check down_timeout(str
 extern int __must_check down_timeout(struct semaphore *sem, long jiffies);
 extern void up(struct semaphore *sem);
 
+/**
+ * down_try - try to down a semaphore, but don't block
+ * @sem: the semaphore
+ *
+ * This is equivalent to down_trylock(), but has the same return codes as
+ * spin_trylock and mutex_trylock: 1 if semaphore acquired, 0 if not.
+ *
+ * down_trylock() with its confusing return codes will be deprecated
+ * soon.  It will not be missed.
+ */
+static inline bool __must_check down_try(struct semaphore *sem)
+{
+	return !down_trylock(sem);
+}
 #endif /* __LINUX_SEMAPHORE_H */


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-07-29 13:01   ` Rusty Russell
@ 2008-07-29 16:21     ` Randy Dunlap
  2008-07-29 23:56       ` Rusty Russell
  2008-08-01 17:26     ` Linus Torvalds
  1 sibling, 1 reply; 17+ messages in thread
From: Randy Dunlap @ 2008-07-29 16:21 UTC (permalink / raw)
  To: Rusty Russell
  Cc: Paul Menage, Linus Torvalds, linux-kernel, Matthew Wilcox,
	Randy.Dunlap, Andrew Morton, Christoph Hellwig

On Tue, 29 Jul 2008 23:01:17 +1000 Rusty Russell wrote:

> On Tuesday 29 July 2008 10:27:15 Paul Menage wrote:
> > On Mon, Jul 28, 2008 at 5:15 PM, Rusty Russell <rusty@rustcorp.com.au> wrote:
> > > +/**
> > > + * down_try - try to down a semaphore, but don't block
> > > + * @sem: the semaphore
> >
> > Is there a reason to avoid using a return type of "bool" for this?
> 
> Probably, but not a good one.
> 
> This incorporates Andrew's whitespace fix as well (ie:
> introduce-down_try-so-we-can-move-away-from-down_trylock.patch and
> introduce-down_try-so-we-can-move-away-from-down_trylock-checkpatch-fixes.patch)
> 
> Introduce down_try()
> 
> I planned on removing the much-disliked down_trylock() (with its
> backwards return codes) in 2.6.27, but it's creating something of a
> logjam with other patches in -mm and linux-next.
> 
> Andrew suggested introducing "down_try" as a wrapper now, to make
> the transition easier.

Hi Rusty,

Will you also be making updates to Documentation/DocBook/kernel-locking.tmpl ?


> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Christoph Hellwig <hch@infradead.org>
> Cc: Matthew Wilcox <matthew@wil.cx>
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
>  include/linux/semaphore.h |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff -r 92664ae4130b include/linux/semaphore.h
> --- a/include/linux/semaphore.h	Wed May 21 14:54:40 2008 +1000
> +++ b/include/linux/semaphore.h	Wed May 21 15:07:31 2008 +1000
> @@ -48,4 +48,18 @@ extern int __must_check down_timeout(str
>  extern int __must_check down_timeout(struct semaphore *sem, long jiffies);
>  extern void up(struct semaphore *sem);
>  
> +/**
> + * down_try - try to down a semaphore, but don't block
> + * @sem: the semaphore
> + *
> + * This is equivalent to down_trylock(), but has the same return codes as
> + * spin_trylock and mutex_trylock: 1 if semaphore acquired, 0 if not.
> + *
> + * down_trylock() with its confusing return codes will be deprecated
> + * soon.  It will not be missed.
> + */
> +static inline bool __must_check down_try(struct semaphore *sem)
> +{
> +	return !down_trylock(sem);
> +}
>  #endif /* __LINUX_SEMAPHORE_H */


---
~Randy
Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA
http://linuxplumbersconf.org/

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-07-29 16:21     ` Randy Dunlap
@ 2008-07-29 23:56       ` Rusty Russell
  0 siblings, 0 replies; 17+ messages in thread
From: Rusty Russell @ 2008-07-29 23:56 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Paul Menage, Linus Torvalds, linux-kernel, Matthew Wilcox,
	Andrew Morton, Christoph Hellwig

On Wednesday 30 July 2008 02:21:02 Randy Dunlap wrote:
> On Tue, 29 Jul 2008 23:01:17 +1000 Rusty Russell wrote:
> > Andrew suggested introducing "down_try" as a wrapper now, to make
> > the transition easier.
>
> Hi Rusty,
>
> Will you also be making updates to
> Documentation/DocBook/kernel-locking.tmpl ?

Hi Randy,

   The section on sems has been removed in the latest Linus tree.  We're 
trying to bury semaphores, but it's not clear that will ever happen; hence 
this bet-hedging patch.

Cheers,
Rusty.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-07-29 13:01   ` Rusty Russell
  2008-07-29 16:21     ` Randy Dunlap
@ 2008-08-01 17:26     ` Linus Torvalds
  2008-08-01 17:40       ` Andrew Morton
  2008-08-03  8:33       ` Rusty Russell
  1 sibling, 2 replies; 17+ messages in thread
From: Linus Torvalds @ 2008-08-01 17:26 UTC (permalink / raw)
  To: Rusty Russell
  Cc: Paul Menage, linux-kernel, Matthew Wilcox, Randy.Dunlap,
	Andrew Morton, Christoph Hellwig



On Tue, 29 Jul 2008, Rusty Russell wrote:
> 
> Introduce down_try()

I hate that name. Everybody else uses "xxx_trylock()", now you introduce a 
short version of that that just has the same return value as everybody 
else except for semaphores that admittedly were odd.

Also, all actual _users_ of down_trylock() seem to be prime candidates for 
turning into mutexes anyway - with the _possible_ exception of the console 
semaphore which has problems with the mutex debugging code.

> I planned on removing the much-disliked down_trylock() (with its
> backwards return codes) in 2.6.27, but it's creating something of a
> logjam with other patches in -mm and linux-next.
> 
> Andrew suggested introducing "down_try" as a wrapper now, to make
> the transition easier.

The transition to WHAT? To crap?

There is no need to introduce yet another temporary thing just to make 
things even _more_ confusing.

Yeah, I'm grumpy. I'm always pretty grumpy, but I'm trying to go through 
some backlog where I had been going "hmm, why would I do this", and this 
one wasn't the only one where my reaction was "if I pull/apply this, the 
end result is worse".

Guys, some quality control and critical thinking, please.

			Linus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-08-01 17:26     ` Linus Torvalds
@ 2008-08-01 17:40       ` Andrew Morton
  2008-08-01 18:22         ` Linus Torvalds
  2008-08-03  8:33       ` Rusty Russell
  1 sibling, 1 reply; 17+ messages in thread
From: Andrew Morton @ 2008-08-01 17:40 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Rusty Russell, Paul Menage, linux-kernel, Matthew Wilcox,
	Randy.Dunlap, Christoph Hellwig

On Fri, 1 Aug 2008 10:26:33 -0700 (PDT) Linus Torvalds <torvalds@linux-foundation.org> wrote:

> > I planned on removing the much-disliked down_trylock() (with its
> > backwards return codes) in 2.6.27, but it's creating something of a
> > logjam with other patches in -mm and linux-next.
> > 
> > Andrew suggested introducing "down_try" as a wrapper now, to make
> > the transition easier.
> 
> The transition to WHAT? To crap?
> 

The naming is pretty sad, but the inconsistent return value from
down_trylock() drives me batshit.  It means that every time I ever look
at any sort of trylock call I need to go back to the definition site to
work out if it's the one which returns true or if it's the one which
returns false.

It would be good to get that fixed.  And if we _do_ want to fix it, I
don't see any alternative to creating a new function.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-08-01 17:40       ` Andrew Morton
@ 2008-08-01 18:22         ` Linus Torvalds
  0 siblings, 0 replies; 17+ messages in thread
From: Linus Torvalds @ 2008-08-01 18:22 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Rusty Russell, Paul Menage, linux-kernel, Matthew Wilcox,
	Randy.Dunlap, Christoph Hellwig



On Fri, 1 Aug 2008, Andrew Morton wrote:

> > 
> 
> The naming is pretty sad, but the inconsistent return value from
> down_trylock() drives me batshit.  It means that every time I ever look
> at any sort of trylock call I need to go back to the definition site to
> work out if it's the one which returns true or if it's the one which
> returns false.
> 
> It would be good to get that fixed.  And if we _do_ want to fix it, I
> don't see any alternative to creating a new function.

The alternative is to just get rid of "down_trylock()" entirely. Creatign 
a shadow function with a different return value is just going to confuse 
people even more than the current situation.

That's why I pointed out that all the current users (apart from the 
special console usage) really do look like prime candidates to just 
convert to mutexes.

Of course, regardless, _some_ of those have actually taken the 
down_trylock semantics. See

	#define usb_trylock_device(udev)    down_trylock(&(udev)->dev.sem)

so nothing gets rid of that ;)

But at least it should be possible to replace something like half the 
current users of down_trylock() by just teh trivial conversion to mutexes. 
Which would be a good thing regardless.

			Linus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-08-01 17:26     ` Linus Torvalds
  2008-08-01 17:40       ` Andrew Morton
@ 2008-08-03  8:33       ` Rusty Russell
  2008-08-03 13:07         ` Matthew Wilcox
  2008-08-03 17:33         ` Linus Torvalds
  1 sibling, 2 replies; 17+ messages in thread
From: Rusty Russell @ 2008-08-03  8:33 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Paul Menage, linux-kernel, Matthew Wilcox, Randy.Dunlap,
	Andrew Morton, Christoph Hellwig

On Saturday 02 August 2008 03:26:33 Linus Torvalds wrote:
> On Tue, 29 Jul 2008, Rusty Russell wrote:
> > Introduce down_try()
>
> I hate that name. Everybody else uses "xxx_trylock()", now you introduce a
> short version of that that just has the same return value as everybody
> else except for semaphores that admittedly were odd.

spin_lock => spin_trylock, so down => trydown.  But everyone hated that, too.

I love your suggestion tho.  Oh wait, you didn't make one...

> Also, all actual _users_ of down_trylock() seem to be prime candidates for
> turning into mutexes anyway - with the _possible_ exception of the console
> semaphore which has problems with the mutex debugging code.

And Willy is working on that.  Still.  Frankly, I gave up waiting.

> > Andrew suggested introducing "down_try" as a wrapper now, to make
> > the transition easier.
>
> The transition to WHAT? To crap?
>
> There is no need to introduce yet another temporary thing just to make
> things even _more_ confusing.

And so my patch series replaces all 21 of them.  It's a trivial replace, 
unlike sem -> mutex.

> Guys, some quality control and critical thinking, please.

Good idea.  If we'd done that we wouldn't have the down_trylock() brain 
damage.

Rusty.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-08-03  8:33       ` Rusty Russell
@ 2008-08-03 13:07         ` Matthew Wilcox
  2008-08-03 17:33         ` Linus Torvalds
  1 sibling, 0 replies; 17+ messages in thread
From: Matthew Wilcox @ 2008-08-03 13:07 UTC (permalink / raw)
  To: Rusty Russell
  Cc: Linus Torvalds, Paul Menage, linux-kernel, Randy.Dunlap,
	Andrew Morton, Christoph Hellwig

On Sun, Aug 03, 2008 at 06:33:30PM +1000, Rusty Russell wrote:
> On Saturday 02 August 2008 03:26:33 Linus Torvalds wrote:
> > Also, all actual _users_ of down_trylock() seem to be prime candidates for
> > turning into mutexes anyway - with the _possible_ exception of the console
> > semaphore which has problems with the mutex debugging code.
> 
> And Willy is working on that.  Still.  Frankly, I gave up waiting.

It's low-priority for me.  SSDs are much more exciting.

> > Guys, some quality control and critical thinking, please.
> 
> Good idea.  If we'd done that we wouldn't have the down_trylock() brain 
> damage.

I believe down_trylock() came first.  spin_trylock() was then the one
that was gratuitously different and mutex_trylock() decided to follow
the spinning semantics rather than the sleeping semantics.  But yeah,
whatever, big mess.  I'm not convinced down_try() is an improvement.

But I bet we could have got rid of most of the users of down_trylock()
in the time that's been spent wanking about down_try().  Hey, let's make
it return bool!  Hey, let's argue about the name!  Hey, let's argue
about the documentation!

Sometimes the bikeshed needs to be bulldozed, not given another lick of
paint.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-08-03  8:33       ` Rusty Russell
  2008-08-03 13:07         ` Matthew Wilcox
@ 2008-08-03 17:33         ` Linus Torvalds
  2008-08-03 17:35           ` Linus Torvalds
  1 sibling, 1 reply; 17+ messages in thread
From: Linus Torvalds @ 2008-08-03 17:33 UTC (permalink / raw)
  To: Rusty Russell
  Cc: Paul Menage, linux-kernel, Matthew Wilcox, Randy.Dunlap,
	Andrew Morton, Christoph Hellwig



On Sun, 3 Aug 2008, Rusty Russell wrote:
> 
> I love your suggestion tho.  Oh wait, you didn't make one...

Ok, Rusty, I'm not bothering with this thread any more.

I gave a suggestion.

You didn't like it. Go away.

> And so my patch series replaces all 21 of them.  It's a trivial replace, 
> unlike sem -> mutex.

Your series doesn't "replace" anything.

It renames things with no good reason. The end result is _worse_. I told 
you why. You don't like it.

> > Guys, some quality control and critical thinking, please.
> 
> Good idea.  If we'd done that we wouldn't have the down_trylock() brain 
> damage.

You don't see the difference between "new crap" and "legacy crap that 
people have historical reasons for and that people have learnt to live 
with"?

Anyway, I NAK'ed your patches. Deal with it.

		Linus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-08-03 17:33         ` Linus Torvalds
@ 2008-08-03 17:35           ` Linus Torvalds
  2008-08-04  3:28             ` Rusty Russell
  0 siblings, 1 reply; 17+ messages in thread
From: Linus Torvalds @ 2008-08-03 17:35 UTC (permalink / raw)
  To: Rusty Russell
  Cc: Paul Menage, linux-kernel, Matthew Wilcox, Randy.Dunlap,
	Andrew Morton, Christoph Hellwig



On Sun, 3 Aug 2008, Linus Torvalds wrote:
> 
> Anyway, I NAK'ed your patches. Deal with it.

Btw, "dealing with it" can just be "don't bother".

Leaving old stuff _alone_ is a perfectly good approach. Being slightly 
ugly for historical reasons is much less of a problem than just "change 
for change's sake".

			Linus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-08-03 17:35           ` Linus Torvalds
@ 2008-08-04  3:28             ` Rusty Russell
  2008-08-04  5:53               ` Linus Torvalds
  0 siblings, 1 reply; 17+ messages in thread
From: Rusty Russell @ 2008-08-04  3:28 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Paul Menage, linux-kernel, Matthew Wilcox, Randy.Dunlap,
	Andrew Morton, Christoph Hellwig

On Monday 04 August 2008 03:35:41 Linus Torvalds wrote:
> On Sun, 3 Aug 2008, Linus Torvalds wrote:
> > Anyway, I NAK'ed your patches. Deal with it.
>
> Btw, "dealing with it" can just be "don't bother".

Yes, let's not make a mountain out of a molehill.

Someone sent me a patch documenting the illogic of down_trylock().  I decided 
to try to fix it rather than just bitch and moan.

Kernel coding involves occasionally getting fed a Linus-special shit sandwich 
rant.  I've long given up expecting a polite "No; please just replace them 
all with mutexes" one-liner.  I might have actually been shocked into doing 
it.

Consider the patches forgotten,
Rusty.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-08-04  3:28             ` Rusty Russell
@ 2008-08-04  5:53               ` Linus Torvalds
  2008-08-04  7:57                 ` Rusty Russell
  0 siblings, 1 reply; 17+ messages in thread
From: Linus Torvalds @ 2008-08-04  5:53 UTC (permalink / raw)
  To: Rusty Russell
  Cc: Paul Menage, linux-kernel, Matthew Wilcox, Randy.Dunlap,
	Andrew Morton, Christoph Hellwig



On Mon, 4 Aug 2008, Rusty Russell wrote:
> 
> Someone sent me a patch documenting the illogic of down_trylock().  I decided 
> to try to fix it rather than just bitch and moan.

I do agree that it is illogical. I just think your solution is worse than 
the problem - turning one illogical function into a redundant one seems 
the worse problem.

We could just fix the return value, since it's not used very much, but 
we'd obviously never know what out-of-tree users there might be, so that's 
not really a good solution either. But that's obviously why we'd then have 
to rename it to something else, so whichever way we turn, we'd just be 
screwed.

I'm much happier telling people to "just don't use semaphores any more".
The _legacy_ users get down_trylock() right. 

		Linus

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-08-04  5:53               ` Linus Torvalds
@ 2008-08-04  7:57                 ` Rusty Russell
  2008-08-04  8:45                   ` Alan Cox
  0 siblings, 1 reply; 17+ messages in thread
From: Rusty Russell @ 2008-08-04  7:57 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Paul Menage, linux-kernel, Matthew Wilcox, Randy.Dunlap,
	Andrew Morton, Christoph Hellwig

On Monday 04 August 2008 15:53:59 Linus Torvalds wrote:
> On Mon, 4 Aug 2008, Rusty Russell wrote:
> > Someone sent me a patch documenting the illogic of down_trylock().  I
> > decided to try to fix it rather than just bitch and moan.
>
> I do agree that it is illogical. I just think your solution is worse than
> the problem - turning one illogical function into a redundant one seems
> the worse problem.

Ah, to be clear: my second patch switches down_trylock() in terms of
down_try() and deprecates it.  Then 21 replacement patches.  Then finally a
patch to remove down_trylock() altogether.

This was "minimal obviously correct" patch.

> I'm much happier telling people to "just don't use semaphores any more".
> The _legacy_ users get down_trylock() right.

It just annoys me to see a turd in the tree.  Even a little old one.

Thanks,
Rusty.

Here's the git tree if anyone feels enthused (without final removal patch):

The following changes since commit 2b12a4c524812fb3f6ee590a02e65b95c8c32229:
  Linus Torvalds (1):
        Merge branch 'release' of git://git.kernel.org/.../aegl/linux-2.6

are available in the git repository at:

  ssh://master.kernel.org/pub/scm/linux/kernel/git/rusty/linux-2.6-for-linus.git master

Rusty Russell (24):
      Introduce down_try()
      Deprecate down_trylock()
      down_trylock -> down_try in documentation and comments.
      down_trylock -> down_try in arch/ia64/kernel/salinfo.c
      down_trylock -> down_try in drivers/char/snsc.c
      down_trylock -> down_try in drivers/char/viotape.c
      down_trylock -> down_try in drivers/infiniband/core/user_mad.c
      down_trylock -> down_try in drivers/input/serio/hil_mlc.c
      down_trylock -> down_try in drivers/input/serio/hp_sdc_mlc.c
      down_trylock -> down_try in drivers/md/dm-raid1.c
      down_trylock -> down_try in drivers/net/3c527.c
      down_trylock -> down_try in drivers/net/irda/sir_dev.c
      down_trylock -> down_try in drivers/net/wireless/airo.c
      down_trylock -> down_try in drivers/scsi/aacraid/commsup.c
      down_trylock -> down_try for usb_trylock_device()
      down_trylock -> down_try in drivers/usb/gadget/inode.c
      down_trylock -> down_try in xfs
      down_trylock -> down_try in kernel/printk.c
      down_trylock -> down_try in drivers/watchdog/ar7_wdt.c
      down_trylock -> down_try in drivers/watchdog/it8712f_wdt.c
      down_trylock -> down_try in drivers/watchdog/s3c2410_wdt.c
      down_trylock -> down_try in drivers/watchdog/sc1200wdt.c
      down_trylock -> down_try in drivers/watchdog/scx200_wdt.c
      down_trylock -> down_try in drivers/watchdog/wdt_pci.c

 arch/ia64/kernel/salinfo.c         |    4 ++--
 drivers/char/snsc.c                |    4 ++--
 drivers/char/viotape.c             |    4 ++--
 drivers/infiniband/core/user_mad.c |    2 +-
 drivers/input/serio/hil_mlc.c      |    4 ++--
 drivers/input/serio/hp_sdc_mlc.c   |   14 +++++++-------
 drivers/md/dm-raid1.c              |    2 +-
 drivers/net/3c527.c                |    2 +-
 drivers/net/irda/sir_dev.c         |    2 +-
 drivers/net/wireless/airo.c        |   12 ++++++------
 drivers/scsi/aacraid/commsup.c     |    2 +-
 drivers/usb/core/usb.c             |    2 +-
 drivers/usb/gadget/inode.c         |    2 +-
 drivers/watchdog/ar7_wdt.c         |    2 +-
 drivers/watchdog/it8712f_wdt.c     |    2 +-
 drivers/watchdog/s3c2410_wdt.c     |    2 +-
 drivers/watchdog/sc1200wdt.c       |    2 +-
 drivers/watchdog/scx200_wdt.c      |    2 +-
 drivers/watchdog/wdt_pci.c         |    2 +-
 fs/ocfs2/inode.c                   |    4 ----
 fs/xfs/linux-2.6/sema.h            |    8 +++-----
 fs/xfs/linux-2.6/xfs_buf.c         |    4 ++--
 include/linux/mutex.h              |    4 ----
 include/linux/semaphore.h          |    8 ++++++--
 include/linux/usb.h                |    2 +-
 kernel/mutex.c                     |    4 ++--
 kernel/printk.c                    |    4 ++--
 kernel/semaphore.c                 |   17 ++++++++---------
 28 files changed, 58 insertions(+), 65 deletions(-)
===
diff --git a/arch/ia64/kernel/salinfo.c b/arch/ia64/kernel/salinfo.c
index ecb9eb7..57c10ef 100644
--- a/arch/ia64/kernel/salinfo.c
+++ b/arch/ia64/kernel/salinfo.c
@@ -192,7 +192,7 @@ struct salinfo_platform_oemdata_parms {
 static void
 salinfo_work_to_do(struct salinfo_data *data)
 {
-	down_trylock(&data->mutex);
+	down_try(&data->mutex);
 	up(&data->mutex);
 }
 
@@ -309,7 +309,7 @@ salinfo_event_read(struct file *file, char __user *buffer, size_t count, loff_t
 	int i, n, cpu = -1;
 
 retry:
-	if (cpus_empty(data->cpu_event) && down_trylock(&data->mutex)) {
+	if (cpus_empty(data->cpu_event) && !down_try(&data->mutex)) {
 		if (file->f_flags & O_NONBLOCK)
 			return -EAGAIN;
 		if (down_interruptible(&data->mutex))
diff --git a/drivers/char/snsc.c b/drivers/char/snsc.c
index 3ce60df..548161d 100644
--- a/drivers/char/snsc.c
+++ b/drivers/char/snsc.c
@@ -164,7 +164,7 @@ scdrv_read(struct file *file, char __user *buf, size_t count, loff_t *f_pos)
 	struct subch_data_s *sd = (struct subch_data_s *) file->private_data;
 
 	/* try to get control of the read buffer */
-	if (down_trylock(&sd->sd_rbs)) {
+	if (!down_try(&sd->sd_rbs)) {
 		/* somebody else has it now;
 		 * if we're non-blocking, then exit...
 		 */
@@ -256,7 +256,7 @@ scdrv_write(struct file *file, const char __user *buf,
 	struct subch_data_s *sd = (struct subch_data_s *) file->private_data;
 
 	/* try to get control of the write buffer */
-	if (down_trylock(&sd->sd_wbs)) {
+	if (!down_try(&sd->sd_wbs)) {
 		/* somebody else has it now;
 		 * if we're non-blocking, then exit...
 		 */
diff --git a/drivers/char/viotape.c b/drivers/char/viotape.c
index 7a70a40..884613e 100644
--- a/drivers/char/viotape.c
+++ b/drivers/char/viotape.c
@@ -362,7 +362,7 @@ static ssize_t viotap_write(struct file *file, const char *buf,
 	 * semaphore
 	 */
 	if (noblock) {
-		if (down_trylock(&reqSem)) {
+		if (!down_try(&reqSem)) {
 			ret = -EWOULDBLOCK;
 			goto free_op;
 		}
@@ -452,7 +452,7 @@ static ssize_t viotap_read(struct file *file, char *buf, size_t count,
 	 * semaphore
 	 */
 	if (noblock) {
-		if (down_trylock(&reqSem)) {
+		if (!down_try(&reqSem)) {
 			ret = -EWOULDBLOCK;
 			goto free_op;
 		}
diff --git a/drivers/infiniband/core/user_mad.c b/drivers/infiniband/core/user_mad.c
index 268a2d2..ae7a981 100644
--- a/drivers/infiniband/core/user_mad.c
+++ b/drivers/infiniband/core/user_mad.c
@@ -901,7 +901,7 @@ static int ib_umad_sm_open(struct inode *inode, struct file *filp)
 		return -ENXIO;
 
 	if (filp->f_flags & O_NONBLOCK) {
-		if (down_trylock(&port->sm_sem)) {
+		if (!down_try(&port->sm_sem)) {
 			ret = -EAGAIN;
 			goto fail;
 		}
diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
index 37586a6..e779f3d 100644
--- a/drivers/input/serio/hil_mlc.c
+++ b/drivers/input/serio/hil_mlc.c
@@ -607,7 +607,7 @@ static inline void hilse_setup_input(hil_mlc *mlc, const struct hilse_node *node
 	do_gettimeofday(&(mlc->instart));
 	mlc->icount = 15;
 	memset(mlc->ipacket, 0, 16 * sizeof(hil_packet));
-	BUG_ON(down_trylock(&mlc->isem));
+	BUG_ON(!down_try(&mlc->isem));
 }
 
 #ifdef HIL_MLC_DEBUG
@@ -694,7 +694,7 @@ static int hilse_donode(hil_mlc *mlc)
 	out2:
 		write_unlock_irqrestore(&mlc->lock, flags);
 
-		if (down_trylock(&mlc->osem)) {
+		if (!down_try(&mlc->osem)) {
 			nextidx = HILSEN_DOZE;
 			break;
 		}
diff --git a/drivers/input/serio/hp_sdc_mlc.c b/drivers/input/serio/hp_sdc_mlc.c
index b587e2d..e88a352 100644
--- a/drivers/input/serio/hp_sdc_mlc.c
+++ b/drivers/input/serio/hp_sdc_mlc.c
@@ -148,7 +148,7 @@ static int hp_sdc_mlc_in(hil_mlc *mlc, suseconds_t timeout)
 	priv = mlc->priv;
 
 	/* Try to down the semaphore */
-	if (down_trylock(&mlc->isem)) {
+	if (!down_try(&mlc->isem)) {
 		struct timeval tv;
 		if (priv->emtestmode) {
 			mlc->ipacket[0] =
@@ -186,13 +186,13 @@ static int hp_sdc_mlc_cts(hil_mlc *mlc)
 	priv = mlc->priv;
 
 	/* Try to down the semaphores -- they should be up. */
-	BUG_ON(down_trylock(&mlc->isem));
-	BUG_ON(down_trylock(&mlc->osem));
+	BUG_ON(!down_try(&mlc->isem));
+	BUG_ON(!down_try(&mlc->osem));
 
 	up(&mlc->isem);
 	up(&mlc->osem);
 
-	if (down_trylock(&mlc->csem)) {
+	if (!down_try(&mlc->csem)) {
 		if (priv->trans.act.semaphore != &mlc->csem)
 			goto poll;
 		else
@@ -229,7 +229,7 @@ static void hp_sdc_mlc_out(hil_mlc *mlc)
 	priv = mlc->priv;
 
 	/* Try to down the semaphore -- it should be up. */
-	BUG_ON(down_trylock(&mlc->osem));
+	BUG_ON(!down_try(&mlc->osem));
 
 	if (mlc->opacket & HIL_DO_ALTER_CTRL)
 		goto do_control;
@@ -240,7 +240,7 @@ static void hp_sdc_mlc_out(hil_mlc *mlc)
 		return;
 	}
 	/* Shouldn't be sending commands when loop may be busy */
-	BUG_ON(down_trylock(&mlc->csem));
+	BUG_ON(!down_try(&mlc->csem));
 	up(&mlc->csem);
 
 	priv->trans.actidx = 0;
@@ -296,7 +296,7 @@ static void hp_sdc_mlc_out(hil_mlc *mlc)
 	priv->tseq[3] = 0;
 	if (mlc->opacket & HIL_CTRL_APE) {
 		priv->tseq[3] |= HP_SDC_LPC_APE_IPF;
-		down_trylock(&mlc->csem);
+		down_try(&mlc->csem);
 	}
  enqueue:
 	hp_sdc_enqueue_transaction(&priv->trans);
diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
index ff05fe8..137f024 100644
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -587,7 +587,7 @@ static void rh_recovery_prepare(struct region_hash *rh)
 	/* Extra reference to avoid race with rh_stop_recovery */
 	atomic_inc(&rh->recovery_in_flight);
 
-	while (!down_trylock(&rh->recovery_count)) {
+	while (down_try(&rh->recovery_count)) {
 		atomic_inc(&rh->recovery_in_flight);
 		if (__rh_recovery_prepare(rh) <= 0) {
 			atomic_dec(&rh->recovery_in_flight);
diff --git a/drivers/net/3c527.c b/drivers/net/3c527.c
index 6aca0c6..9f57863 100644
--- a/drivers/net/3c527.c
+++ b/drivers/net/3c527.c
@@ -576,7 +576,7 @@ static int mc32_command_nowait(struct net_device *dev, u16 cmd, void *data, int
 	int ioaddr = dev->base_addr;
 	int ret = -1;
 
-	if (down_trylock(&lp->cmd_mutex) == 0)
+	if (down_try(&lp->cmd_mutex))
 	{
 		lp->cmd_nonblocking=1;
 		lp->exec_box->mbox=0;
diff --git a/drivers/net/irda/sir_dev.c b/drivers/net/irda/sir_dev.c
index 3f32909..8b9e26e 100644
--- a/drivers/net/irda/sir_dev.c
+++ b/drivers/net/irda/sir_dev.c
@@ -287,7 +287,7 @@ int sirdev_schedule_request(struct sir_dev *dev, int initial_state, unsigned par
 	IRDA_DEBUG(2, "%s - state=0x%04x / param=%u\n", __func__,
 			initial_state, param);
 
-	if (down_trylock(&fsm->sem)) {
+	if (!down_try(&fsm->sem)) {
 		if (in_interrupt()  ||  in_atomic()  ||  irqs_disabled()) {
 			IRDA_DEBUG(1, "%s(), state machine busy!\n", __func__);
 			return -EWOULDBLOCK;
diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
index b5cd850..fc36977 100644
--- a/drivers/net/wireless/airo.c
+++ b/drivers/net/wireless/airo.c
@@ -2137,7 +2137,7 @@ static int airo_start_xmit(struct sk_buff *skb, struct net_device *dev) {
 	fids[i] |= (len << 16);
 	priv->xmit.skb = skb;
 	priv->xmit.fid = i;
-	if (down_trylock(&priv->sem) != 0) {
+	if (!down_try(&priv->sem)) {
 		set_bit(FLAG_PENDING_XMIT, &priv->flags);
 		netif_stop_queue(dev);
 		set_bit(JOB_XMIT, &priv->jobs);
@@ -2208,7 +2208,7 @@ static int airo_start_xmit11(struct sk_buff *skb, struct net_device *dev) {
 	fids[i] |= (len << 16);
 	priv->xmit11.skb = skb;
 	priv->xmit11.fid = i;
-	if (down_trylock(&priv->sem) != 0) {
+	if (!down_try(&priv->sem)) {
 		set_bit(FLAG_PENDING_XMIT11, &priv->flags);
 		netif_stop_queue(dev);
 		set_bit(JOB_XMIT11, &priv->jobs);
@@ -2258,7 +2258,7 @@ static struct net_device_stats *airo_get_stats(struct net_device *dev)
 
 	if (!test_bit(JOB_STATS, &local->jobs)) {
 		/* Get stats out of the card if available */
-		if (down_trylock(&local->sem) != 0) {
+		if (!down_try(&local->sem)) {
 			set_bit(JOB_STATS, &local->jobs);
 			wake_up_interruptible(&local->thr_wait);
 		} else
@@ -2285,7 +2285,7 @@ static void airo_set_multicast_list(struct net_device *dev) {
 
 	if ((dev->flags ^ ai->flags) & IFF_PROMISC) {
 		change_bit(FLAG_PROMISC, &ai->flags);
-		if (down_trylock(&ai->sem) != 0) {
+		if (!down_try(&ai->sem)) {
 			set_bit(JOB_PROMISC, &ai->jobs);
 			wake_up_interruptible(&ai->thr_wait);
 		} else
@@ -3213,7 +3213,7 @@ static irqreturn_t airo_interrupt(int irq, void *dev_id)
 				set_bit(FLAG_UPDATE_UNI, &apriv->flags);
 				set_bit(FLAG_UPDATE_MULTI, &apriv->flags);
 
-				if (down_trylock(&apriv->sem) != 0) {
+				if (!down_try(&apriv->sem)) {
 					set_bit(JOB_EVENT, &apriv->jobs);
 					wake_up_interruptible(&apriv->thr_wait);
 				} else
@@ -7659,7 +7659,7 @@ static struct iw_statistics *airo_get_wireless_stats(struct net_device *dev)
 
 	if (!test_bit(JOB_WSTATS, &local->jobs)) {
 		/* Get stats out of the card if available */
-		if (down_trylock(&local->sem) != 0) {
+		if (!down_try(&local->sem)) {
 			set_bit(JOB_WSTATS, &local->jobs);
 			wake_up_interruptible(&local->thr_wait);
 		} else
diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 289304a..c011d05 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -490,7 +490,7 @@ int aac_fib_send(u16 command, struct fib *fibptr, unsigned long size,
 			 * hardware failure has occurred.
 			 */
 			unsigned long count = 36000000L; /* 3 minutes */
-			while (down_trylock(&fibptr->event_wait)) {
+			while (!down_try(&fibptr->event_wait)) {
 				int blink;
 				if (--count == 0) {
 					struct aac_queue * q = &dev->queues->queue[AdapNormCmdQueue];
diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 84fcaa6..f31f0c5 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -477,7 +477,7 @@ int usb_lock_device_for_reset(struct usb_device *udev,
 		}
 	}
 
-	while (usb_trylock_device(udev) != 0) {
+	while (!usb_trylock_device(udev)) {
 
 		/* If we can't acquire the lock after waiting one second,
 		 * we're probably deadlocked */
diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
index f4585d3..981275b 100644
--- a/drivers/usb/gadget/inode.c
+++ b/drivers/usb/gadget/inode.c
@@ -297,7 +297,7 @@ get_ready_ep (unsigned f_flags, struct ep_data *epdata)
 	int	val;
 
 	if (f_flags & O_NONBLOCK) {
-		if (down_trylock (&epdata->lock) != 0)
+		if (!down_try (&epdata->lock))
 			goto nonblock;
 		if (epdata->state != STATE_EP_ENABLED) {
 			up (&epdata->lock);
diff --git a/drivers/watchdog/ar7_wdt.c b/drivers/watchdog/ar7_wdt.c
index 2eb48c0..5c43dd5 100644
--- a/drivers/watchdog/ar7_wdt.c
+++ b/drivers/watchdog/ar7_wdt.c
@@ -179,7 +179,7 @@ static void ar7_wdt_disable_wdt(void)
 static int ar7_wdt_open(struct inode *inode, struct file *file)
 {
 	/* only allow one at a time */
-	if (down_trylock(&open_semaphore))
+	if (!down_try(&open_semaphore))
 		return -EBUSY;
 	ar7_wdt_enable_wdt();
 	expect_close = 0;
diff --git a/drivers/watchdog/it8712f_wdt.c b/drivers/watchdog/it8712f_wdt.c
index 445b7e8..c9a057c 100644
--- a/drivers/watchdog/it8712f_wdt.c
+++ b/drivers/watchdog/it8712f_wdt.c
@@ -306,7 +306,7 @@ static int
 it8712f_wdt_open(struct inode *inode, struct file *file)
 {
 	/* only allow one at a time */
-	if (down_trylock(&it8712f_wdt_sem))
+	if (!down_try(&it8712f_wdt_sem))
 		return -EBUSY;
 	it8712f_wdt_enable();
 
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 98532c0..23f470a 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -211,7 +211,7 @@ static int s3c2410wdt_set_heartbeat(int timeout)
 
 static int s3c2410wdt_open(struct inode *inode, struct file *file)
 {
-	if(down_trylock(&open_lock))
+	if(!down_try(&open_lock))
 		return -EBUSY;
 
 	if (nowayout)
diff --git a/drivers/watchdog/sc1200wdt.c b/drivers/watchdog/sc1200wdt.c
index 35cddff..c080e6a 100644
--- a/drivers/watchdog/sc1200wdt.c
+++ b/drivers/watchdog/sc1200wdt.c
@@ -151,7 +151,7 @@ static inline int sc1200wdt_status(void)
 static int sc1200wdt_open(struct inode *inode, struct file *file)
 {
 	/* allow one at a time */
-	if (down_trylock(&open_sem))
+	if (!down_try(&open_sem))
 		return -EBUSY;
 
 	if (timeout > MAX_TIMEOUT)
diff --git a/drivers/watchdog/scx200_wdt.c b/drivers/watchdog/scx200_wdt.c
index d55882b..e131988 100644
--- a/drivers/watchdog/scx200_wdt.c
+++ b/drivers/watchdog/scx200_wdt.c
@@ -92,7 +92,7 @@ static void scx200_wdt_disable(void)
 static int scx200_wdt_open(struct inode *inode, struct file *file)
 {
 	/* only allow one at a time */
-	if (down_trylock(&open_semaphore))
+	if (!down_try(&open_semaphore))
 		return -EBUSY;
 	scx200_wdt_enable();
 
diff --git a/drivers/watchdog/wdt_pci.c b/drivers/watchdog/wdt_pci.c
index 1355608..c20690d 100644
--- a/drivers/watchdog/wdt_pci.c
+++ b/drivers/watchdog/wdt_pci.c
@@ -426,7 +426,7 @@ static int wdtpci_ioctl(struct inode *inode, struct file *file, unsigned int cmd
 
 static int wdtpci_open(struct inode *inode, struct file *file)
 {
-	if (down_trylock(&open_sem))
+	if (!down_try(&open_sem))
 		return -EBUSY;
 
 	if (nowayout) {
diff --git a/fs/ocfs2/inode.c b/fs/ocfs2/inode.c
index 7e9e4c7..64211fc 100644
--- a/fs/ocfs2/inode.c
+++ b/fs/ocfs2/inode.c
@@ -1062,10 +1062,6 @@ void ocfs2_clear_inode(struct inode *inode)
 			(unsigned long long)oi->ip_blkno);
 	mutex_unlock(&oi->ip_io_mutex);
 
-	/*
-	 * down_trylock() returns 0, down_write_trylock() returns 1
-	 * kernel 1, world 0
-	 */
 	mlog_bug_on_msg(!down_write_trylock(&oi->ip_alloc_sem),
 			"Clear inode of %llu, alloc_sem is locked\n",
 			(unsigned long long)oi->ip_blkno);
diff --git a/fs/xfs/linux-2.6/sema.h b/fs/xfs/linux-2.6/sema.h
index 3abe7e9..7d20f04 100644
--- a/fs/xfs/linux-2.6/sema.h
+++ b/fs/xfs/linux-2.6/sema.h
@@ -36,17 +36,15 @@ typedef struct semaphore sema_t;
 
 static inline int issemalocked(sema_t *sp)
 {
-	return down_trylock(sp) || (up(sp), 0);
+	return !down_try(sp) || (up(sp), 0);
 }
 
 /*
- * Map cpsema (try to get the sema) to down_trylock. We need to switch
- * the return values since cpsema returns 1 (acquired) 0 (failed) and
- * down_trylock returns the reverse 0 (acquired) 1 (failed).
+ * Map cpsema (try to get the sema) to down_try.
  */
 static inline int cpsema(sema_t *sp)
 {
-	return down_trylock(sp) ? 0 : 1;
+	return down_try(sp);
 }
 
 #endif /* __XFS_SUPPORT_SEMA_H__ */
diff --git a/fs/xfs/linux-2.6/xfs_buf.c b/fs/xfs/linux-2.6/xfs_buf.c
index 9cc8f02..09e50cd 100644
--- a/fs/xfs/linux-2.6/xfs_buf.c
+++ b/fs/xfs/linux-2.6/xfs_buf.c
@@ -538,7 +538,7 @@ found:
 	 * if this does not work then we need to drop the
 	 * spinlock and do a hard attempt on the semaphore.
 	 */
-	if (down_trylock(&bp->b_sema)) {
+	if (!down_try(&bp->b_sema)) {
 		if (!(flags & XBF_TRYLOCK)) {
 			/* wait for buffer ownership */
 			XB_TRACE(bp, "get_lock", 0);
@@ -882,7 +882,7 @@ xfs_buf_cond_lock(
 {
 	int			locked;
 
-	locked = down_trylock(&bp->b_sema) == 0;
+	locked = down_try(&bp->b_sema);
 	if (locked) {
 		XB_SET_OWNER(bp);
 	}
diff --git a/include/linux/mutex.h b/include/linux/mutex.h
index bc6da10..c1f5b3f 100644
--- a/include/linux/mutex.h
+++ b/include/linux/mutex.h
@@ -141,10 +141,6 @@ extern int __must_check mutex_lock_killable(struct mutex *lock);
 # define mutex_lock_killable_nested(lock, subclass) mutex_lock_killable(lock)
 #endif
 
-/*
- * NOTE: mutex_trylock() follows the spin_trylock() convention,
- *       not the down_trylock() convention!
- */
 extern int mutex_trylock(struct mutex *lock);
 extern void mutex_unlock(struct mutex *lock);
 
diff --git a/include/linux/semaphore.h b/include/linux/semaphore.h
index 7415839..00af3c2 100644
--- a/include/linux/semaphore.h
+++ b/include/linux/semaphore.h
@@ -42,8 +42,12 @@ static inline void sema_init(struct semaphore *sem, int val)
 extern void down(struct semaphore *sem);
 extern int __must_check down_interruptible(struct semaphore *sem);
 extern int __must_check down_killable(struct semaphore *sem);
-extern int __must_check down_trylock(struct semaphore *sem);
+extern bool __must_check down_try(struct semaphore *sem);
+/* Old down_trylock() returned the opposite of what was expected. */
+static inline int __deprecated down_trylock(struct semaphore *sem)
+{
+	return !down_try(sem);
+}
 extern int __must_check down_timeout(struct semaphore *sem, long jiffies);
 extern void up(struct semaphore *sem);
-
 #endif /* __LINUX_SEMAPHORE_H */
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 5811c5d..2664efd 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -492,7 +492,7 @@ extern void usb_put_dev(struct usb_device *dev);
 /* USB device locking */
 #define usb_lock_device(udev)		down(&(udev)->dev.sem)
 #define usb_unlock_device(udev)		up(&(udev)->dev.sem)
-#define usb_trylock_device(udev)	down_trylock(&(udev)->dev.sem)
+#define usb_trylock_device(udev)	down_try(&(udev)->dev.sem)
 extern int usb_lock_device_for_reset(struct usb_device *udev,
 				     const struct usb_interface *iface);
 
diff --git a/kernel/mutex.c b/kernel/mutex.c
index 12c779d..38823ca 100644
--- a/kernel/mutex.c
+++ b/kernel/mutex.c
@@ -371,8 +371,8 @@ static inline int __mutex_trylock_slowpath(atomic_t *lock_count)
  * Try to acquire the mutex atomically. Returns 1 if the mutex
  * has been acquired successfully, and 0 on contention.
  *
- * NOTE: this function follows the spin_trylock() convention, so
- * it is negated to the down_trylock() return values! Be careful
+ * NOTE: this function follows the spin_trylock()/down_try() convention,
+ * so it is negated to the old down_trylock() return values! Be careful
  * about this when converting semaphore users to mutexes.
  *
  * This function must not be used in interrupt context. The
diff --git a/kernel/printk.c b/kernel/printk.c
index b51b156..ab30884 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -969,7 +969,7 @@ EXPORT_SYMBOL(acquire_console_sem);
 
 int try_acquire_console_sem(void)
 {
-	if (down_trylock(&console_sem))
+	if (!down_try(&console_sem))
 		return -1;
 	console_locked = 1;
 	console_may_schedule = 0;
@@ -1068,7 +1068,7 @@ void console_unblank(void)
 	 * oops_in_progress is set to 1..
 	 */
 	if (oops_in_progress) {
-		if (down_trylock(&console_sem) != 0)
+		if (!down_try(&console_sem))
 			return;
 	} else
 		acquire_console_sem();
diff --git a/kernel/semaphore.c b/kernel/semaphore.c
index aaaeae8..e9fdc4e 100644
--- a/kernel/semaphore.c
+++ b/kernel/semaphore.c
@@ -14,7 +14,7 @@
  * Some notes on the implementation:
  *
  * The spinlock controls access to the other members of the semaphore.
- * down_trylock() and up() can be called from interrupt context, so we
+ * down_try() and up() can be called from interrupt context, so we
  * have to disable interrupts when taking the lock.  It turns out various
  * parts of the kernel expect to be able to use down() on a semaphore in
  * interrupt context when they know it will succeed, so we have to use
@@ -115,19 +115,18 @@ int down_killable(struct semaphore *sem)
 EXPORT_SYMBOL(down_killable);
 
 /**
- * down_trylock - try to acquire the semaphore, without waiting
+ * down_try - try to acquire the semaphore, without waiting
  * @sem: the semaphore to be acquired
  *
- * Try to acquire the semaphore atomically.  Returns 0 if the mutex has
- * been acquired successfully or 1 if it it cannot be acquired.
+ * Try to acquire the semaphore atomically.  Returns true if the mutex has
+ * been acquired successfully or 0 if it it cannot be acquired.
  *
- * NOTE: This return value is inverted from both spin_trylock and
- * mutex_trylock!  Be careful about this when converting code.
+ * NOTE: This replaces down_trylock() which returned the reverse.
  *
  * Unlike mutex_trylock, this function can be used from interrupt context,
  * and the semaphore can be released by any task or interrupt.
  */
-int down_trylock(struct semaphore *sem)
+bool down_try(struct semaphore *sem)
 {
 	unsigned long flags;
 	int count;
@@ -138,9 +137,9 @@ int down_trylock(struct semaphore *sem)
 		sem->count = count;
 	spin_unlock_irqrestore(&sem->lock, flags);
 
-	return (count < 0);
+	return (count >= 0);
 }
-EXPORT_SYMBOL(down_trylock);
+EXPORT_SYMBOL(down_try);
 
 /**
  * down_timeout - acquire the semaphore within a specified time


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-08-04  7:57                 ` Rusty Russell
@ 2008-08-04  8:45                   ` Alan Cox
  2008-08-04 11:43                     ` Rusty Russell
  0 siblings, 1 reply; 17+ messages in thread
From: Alan Cox @ 2008-08-04  8:45 UTC (permalink / raw)
  To: Rusty Russell
  Cc: Linus Torvalds, Paul Menage, linux-kernel, Matthew Wilcox,
	Randy.Dunlap, Andrew Morton, Christoph Hellwig

>       down_trylock -> down_try in drivers/watchdog/ar7_wdt.c
>       down_trylock -> down_try in drivers/watchdog/it8712f_wdt.c
>       down_trylock -> down_try in drivers/watchdog/s3c2410_wdt.c
>       down_trylock -> down_try in drivers/watchdog/sc1200wdt.c
>       down_trylock -> down_try in drivers/watchdog/scx200_wdt.c
>       down_trylock -> down_try in drivers/watchdog/wdt_pci.c

I sent fixes for all of those to the watchdog maintainer in May. The
patches are still sitting with the maintainer. They all convert to proper
locking rather than sem abuse, clean up all the coding style and fix
numerous bugs plus remove BKL requirements.

Wim it appears is still trying to get real life stuff sorted out so I
think at this point we really need a new watchdog maintainer.

(See series of 57 patches "watchdog: Giant scrub" about 19th May)

Alan

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] Introduce down_try() so we can move away from down_trylock()
  2008-08-04  8:45                   ` Alan Cox
@ 2008-08-04 11:43                     ` Rusty Russell
  0 siblings, 0 replies; 17+ messages in thread
From: Rusty Russell @ 2008-08-04 11:43 UTC (permalink / raw)
  To: Alan Cox; +Cc: linux-kernel, Stephen Rothwell, Andrew Morton

On Monday 04 August 2008 18:45:07 you wrote:
> >       down_trylock -> down_try in drivers/watchdog/ar7_wdt.c
> >       down_trylock -> down_try in drivers/watchdog/it8712f_wdt.c
> >       down_trylock -> down_try in drivers/watchdog/s3c2410_wdt.c
> >       down_trylock -> down_try in drivers/watchdog/sc1200wdt.c
> >       down_trylock -> down_try in drivers/watchdog/scx200_wdt.c
> >       down_trylock -> down_try in drivers/watchdog/wdt_pci.c
>
> I sent fixes for all of those to the watchdog maintainer in May. The
> patches are still sitting with the maintainer. They all convert to proper
> locking rather than sem abuse, clean up all the coding style and fix
> numerous bugs plus remove BKL requirements.
>
> Wim it appears is still trying to get real life stuff sorted out so I
> think at this point we really need a new watchdog maintainer.

Perhaps if noone steps forward soon you should shepherd them through 
linux-next for the next merge window?

Rusty.


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2008-08-04 11:44 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-07-29  0:15 [PATCH] Introduce down_try() so we can move away from down_trylock() Rusty Russell
2008-07-29  0:27 ` Paul Menage
2008-07-29 13:01   ` Rusty Russell
2008-07-29 16:21     ` Randy Dunlap
2008-07-29 23:56       ` Rusty Russell
2008-08-01 17:26     ` Linus Torvalds
2008-08-01 17:40       ` Andrew Morton
2008-08-01 18:22         ` Linus Torvalds
2008-08-03  8:33       ` Rusty Russell
2008-08-03 13:07         ` Matthew Wilcox
2008-08-03 17:33         ` Linus Torvalds
2008-08-03 17:35           ` Linus Torvalds
2008-08-04  3:28             ` Rusty Russell
2008-08-04  5:53               ` Linus Torvalds
2008-08-04  7:57                 ` Rusty Russell
2008-08-04  8:45                   ` Alan Cox
2008-08-04 11:43                     ` Rusty Russell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).