[PATCH] Introduce down_nowait()

[PATCH] Introduce down_nowait()

[Rusty Russell]

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_nowait" 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>

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_nowait - 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_nowait(struct semaphore *sem)
+{
+        return !down_trylock(sem);
+}
 #endif /* __LINUX_SEMAPHORE_H */

Re: [PATCH] Introduce down_nowait()

[Andrew Morton]

On Wed, 21 May 2008 16:00:15 +1000 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_nowait" 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>
> 
> 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_nowait - 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_nowait(struct semaphore *sem)
> +{
> +        return !down_trylock(sem);
> +}
>  #endif /* __LINUX_SEMAPHORE_H */

Actually, I don't thing down_nowait() is a terribly good name, because it
doesn't tell the reader anything about what to expect from the return
value.  Does a non-zero return mean that down_wait() acquired the lock,
or does it not?  Something like down_try() would be better, because if
it returns 1 we can say "ah, the trying succeeded".

otoh if all the kernel's "try" functions now return true on successful
acquisition then there won't be any confusion any more so the name
problably doesn't matter.

Except "down_nowait" doesn't have "try" in its name.  down_try() would
be better?

Re: [PATCH] Introduce down_nowait()

[Rusty Russell]

On Wednesday 21 May 2008 16:29:03 Andrew Morton wrote:
> On Wed, 21 May 2008 16:00:15 +1000 Rusty Russell <rusty@rustcorp.com.au> wrote:
> > Andrew suggested introducing "down_nowait" as a wrapper now, to make
> > the transition easier.
> > ...
> > +/**
> > + * down_nowait - try to down a semaphore, but don't block
> > + * @sem: the semaphore
> > + *
>
> Actually, I don't thing down_nowait() is a terribly good name, because it
> doesn't tell the reader anything about what to expect from the return
> value.  Does a non-zero return mean that down_wait() acquired the lock,
> or does it not?  Something like down_try() would be better, because if
> it returns 1 we can say "ah, the trying succeeded".'

I agree: that was my first name.  Christoph hated it.

> Except "down_nowait" doesn't have "try" in its name.  down_try() would
> be better?

What a great name!  You're a genius!

Subject: [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>

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 */

Re: [PATCH] Introduce down_nowait()

[Christoph Hellwig]

On Tue, May 20, 2008 at 11:29:03PM -0700, Andrew Morton wrote:
> Actually, I don't thing down_nowait() is a terribly good name, because it
> doesn't tell the reader anything about what to expect from the return
> value.  Does a non-zero return mean that down_wait() acquired the lock,
> or does it not?  Something like down_try() would be better, because if
> it returns 1 we can say "ah, the trying succeeded".

Actually, it does, and the kerneldoc comment explains it in every detail
for those who need to read it up.  Then again semaphores and on their
way out, and I really hate the kind of churn this thing introduces at
this moment.  Please let all the semaphore to completion/mutex/other
construct change settle for a while, and with a little chance this gem
will just go away entirely.

Re: [PATCH] Introduce down_nowait()

[Andrew Morton]

On Wed, 21 May 2008 04:04:53 -0400 Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, May 20, 2008 at 11:29:03PM -0700, Andrew Morton wrote:
> > Actually, I don't thing down_nowait() is a terribly good name, because it
> > doesn't tell the reader anything about what to expect from the return
> > value.  Does a non-zero return mean that down_wait() acquired the lock,
> > or does it not?  Something like down_try() would be better, because if
> > it returns 1 we can say "ah, the trying succeeded".
> 
> Actually, it does,

No it doesn't.  If anything, a "true" return from something called
"down_nowait()" means "I didn't wait!".  Or something.

> and the kerneldoc comment explains it in every detail
> for those who need to read it up.

Shouldn't be necessary to look it up.  By that argument we could
call it eat_at_joes() and sell the advertising space.

>  Then again semaphores and on their
> way out, and I really hate the kind of churn this thing introduces at
> this moment.  Please let all the semaphore to completion/mutex/other
> construct change settle for a while, and with a little chance this gem
> will just go away entirely.

Well, we can walk and chew gum at the same time.

The number of down_trylocks in rc3 is 51 and the number of
down_trylocks and down_nowaits in -mm is 47.  So progress is pretty
glacial.

<wonders why -mm still has seven down_trlyocks>

<oh>

./kernel/semaphore.c
./kernel/mutex.c
./include/linux/semaphore.h

Anyway, yes, I agree that the whole effort is a bit dubious and that
the time could be better directed to semaphore eliminations.

Re: [PATCH] Introduce down_nowait()

[Rusty Russell]

On Wednesday 21 May 2008 18:19:26 Andrew Morton wrote:
> The number of down_trylocks in rc3 is 51 and the number of
> down_trylocks and down_nowaits in -mm is 47.  So progress is pretty
> glacial.
...
> Anyway, yes, I agree that the whole effort is a bit dubious and that
> the time could be better directed to semaphore eliminations.

It is deeply annoying, even embarrassing, and trivial to fix.  And as you say, 
process is glacial...

Cheers,
Rusty.

Re: [PATCH] Introduce down_nowait()

[Daniel Walker]

On Wed, 2008-05-21 at 17:56 +1000, Rusty Russell wrote:


> Subject: [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.

I must be missing something critical, but what's the logjam this is
causing?

Daniel

Re: [PATCH] Introduce down_nowait()

[Rusty Russell]

On Thursday 22 May 2008 03:04:58 Daniel Walker wrote:
> On Wed, 2008-05-21 at 17:56 +1000, Rusty Russell wrote:
> > Subject: [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.
>
> I must be missing something critical, but what's the logjam this is
> causing?
>
> Daniel

The patches to change down_trylock to down_try touch a heap of files, which 
are also touched in other people's trees.  If this patch goes upstream, those 
people rewriting that code can use down_try in their rewrite, and I can throw 
mine away.

Cheers,
Rusty.

Re: [PATCH] Introduce down_nowait()

[Daniel Walker]

On Thu, 2008-05-22 at 18:56 +1000, Rusty Russell wrote:

> The patches to change down_trylock to down_try touch a heap of files, which 
> are also touched in other people's trees.  If this patch goes upstream, those 
> people rewriting that code can use down_try in their rewrite, and I can throw 
> mine away.

Seems like it's too much trouble.. I mean, we're removing semaphores
anyway and down_trylock with them. I'll agree with Andrew down_trylock
removal is pretty glacial. It's cause there aren't many of them and most
of the ones I've looked at are in strange locking schemes, which makes
them difficult to remove.. 

I'm not against your changes, but if it's going to cause problems I'd
rather people focus on mutex_trylock instead.

Daniel

Re: [PATCH] Introduce down_nowait()

[Rusty Russell]

On Friday 23 May 2008 01:48:43 Daniel Walker wrote:
> On Thu, 2008-05-22 at 18:56 +1000, Rusty Russell wrote:
> > The patches to change down_trylock to down_try touch a heap of files,
> > which are also touched in other people's trees.  If this patch goes
> > upstream, those people rewriting that code can use down_try in their
> > rewrite, and I can throw mine away.
>
> Seems like it's too much trouble.. I mean, we're removing semaphores
> anyway and down_trylock with them. I'll agree with Andrew down_trylock
> removal is pretty glacial. It's cause there aren't many of them and most
> of the ones I've looked at are in strange locking schemes, which makes
> them difficult to remove..

Frankly, I agree.  I did this patchset assuming it would be trivial, and it 
really is.  We've spent more time discussing it here than spent coding it in 
the first place and handling the conflicts.

> I'm not against your changes, but if it's going to cause problems I'd
> rather people focus on mutex_trylock instead.

Yes, but Stephen seems to have no problems with the conflicts.  He knows the 
removal patches override my patches, and if I miss any down_trylock()s, it 
simply means a deprecated warning.  The onus is on me to solve anything else.

Cheers,
Rusty.