Skip to contentSkip to conversation

Introduce down_nowait()

PATCH In review Rusty Russell 4 participants 10 messages May 21, 2008 – May 23, 2008

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Introduce down_nowait()

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.

Cc Andrew Morton <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Stephen Rothwell <[email protected]>
include/linux/semaphore.h +14 -0
diff -r 92664ae4130b include/linux/semaphore.h
@@ -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 */
#1 lore raw
Quoted 25 lines · On Wed, 21 May 2008 16:00:15 +1000 Rusty Russell <[email protected]> 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.

Cc Andrew Morton <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Stephen Rothwell <[email protected]>

diff -r 92664ae4130b include/linux/semaphore.h

include/linux/semaphore.h +5 -0
@@ -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

include/linux/semaphore.h extern int __must_check down_timeout(str · line 65

Quoted 10 lines
include/linux/semaphore.h +9 -0
+ * 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?

#2 lore raw
Quoted 9 lines · On Wednesday 21 May 2008 16:29:03 Andrew Morton wrote:
Quoted 7 lines · On Wed, 21 May 2008 16:00:15 +1000 Rusty Russell <[email protected]> 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
+ *

Quoted 5 lines

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.

Quoted 2 lines

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.

Cc Andrew Morton <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc Matthew Wilcox <[email protected]>
Cc Stephen Rothwell <[email protected]>
include/linux/semaphore.h +14 -0
diff -r 92664ae4130b include/linux/semaphore.h
@@ -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 */
#3 lore raw
Quoted 8 lines · Rusty Russell

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

#4 lore raw
Quoted 7 lines · On Thursday 22 May 2008 03:04:58 Daniel Walker wrote:
Quoted 6 lines · Rusty Russell

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.

Quoted 7 lines
Quoted 2 lines

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.

#5 lore raw
Quoted 4 lines · Rusty Russell

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

#6 lore raw
Quoted 11 lines · On Friday 23 May 2008 01:48:43 Daniel Walker wrote:
Quoted 4 lines · Rusty Russell

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.

Quoted 2 lines

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.

#7 lore raw
In reply to Andrew Morton
Quoted 5 lines · Andrew Morton

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.

#8 lore raw
Quoted 8 lines · On Wed, 21 May 2008 04:04:53 -0400 Christoph Hellwig <[email protected]> wrote:
Quoted 5 lines · Andrew Morton

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.

Quoted 2 lines

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.

Quoted 5 lines

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.

#9 lore raw
Quoted 3 lines · 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.

...

Quoted 2 lines

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.