semaphore documentation

PATCH In review Matthew Wilcox 4 participants 6 messages Apr 11, 2008 – Apr 15, 2008 3 messages not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Matthew Wilcox

author

2008-04-11 19:22 UTC #3 lore raw

PATCH 1/1 semaphore documentation

Quoted 7 lines · Andrew Morton

Quoted 3 lines · On Thu, 10 Apr 2008 16:08:16 -0600 Matthew Wilcox <[email protected]> wrote:
It seems very strange to me to document the API with the implementation rather than with the declaration. It's almost as if we expect people to have to read the implementation to figure out how stuff works.
That approach makes sense for C++. But for C, the code is .c-centric.

I've never programmed in C++ ... I just expect to find API documentation in header files.

Quoted 4 lines

That's particularly the case with the kernel, where we explicitly work to make the .c files the things which people look at, while not caring about the .h files. Look at how much we say "get that ifdef out of there and hide it in the header file".

I see that as being "move the complexity around" and "get the interfaces right", not "hide it in the header files where nobody ever looks".

Quoted 4 lines

Quoted 2 lines
How about a note in semaphore.c that says "refer to semaphore.h for usage information"?
No, please document it in the C file, where people expect to find it.

Fine, I've done it the other way round.

Please review this doc-patch. Without comments, I'll commit it to the semaphore git tree tomorrow.

include/linux/semaphore.h +2 ~~-37~~

index a7125da..9cae64b 100644
@@ -4,8 +4,7 @@
  *
  * Distributed under the terms of the GNU GPL, version 2
  *
- * Counting semaphores allow up to <n> tasks to acquire the semaphore
- * simultaneously.
+ * Please see kernel/semaphore.c for documentation of these functions
  */
 #ifndef __LINUX_SEMAPHORE_H
 #define __LINUX_SEMAPHORE_H

@@ -13,11 +12,7 @@
 #include <linux/list.h>
 #include <linux/spinlock.h>
 
-/*
- * The spinlock controls access to the other members of the semaphore.
- * 'count' represents how many more tasks can acquire this semaphore.
- * Tasks waiting for the lock are kept on the wait_list.
- */
+/* Please don't access any members of this structure directly */
 struct semaphore {
 	spinlock_t		lock;
 	unsigned int		count;

@@ -46,41 +41,11 @@ static inline void sema_init(struct semaphore *sem, int val)
 #define init_MUTEX(sem)		sema_init(sem, 1)
 #define init_MUTEX_LOCKED(sem)	sema_init(sem, 0)
 
-/*
- * Attempt to acquire the semaphore.  If another task is already holding the
- * semaphore, sleep until the semaphore is released.
- */
 extern void down(struct semaphore *sem);
-
-/*
- * As down(), except the sleep may be interrupted by a signal.  If it is,
- * this function will return -EINTR.
- */
 extern int __must_check down_interruptible(struct semaphore *sem);
-
-/*
- * As down_interruptible(), except the sleep may only be interrupted by
- * signals which are fatal to this process.
- */
 extern int __must_check down_killable(struct semaphore *sem);
-
-/*
- * As down(), except this function will not sleep.  It will return 0 if it
- * acquired the semaphore and 1 if the semaphore was contended.  This
- * function may be called from any context, including interrupt and softirq.
- */
 extern int __must_check down_trylock(struct semaphore *sem);
-
-/*
- * As down(), except this function will return -ETIME if it fails to
- * acquire the semaphore within the specified number of jiffies.
- */
 extern int __must_check down_timeout(struct semaphore *sem, long jiffies);
-
-/*
- * Release the semaphore.  Unlike mutexes, up() may be called from any
- * context and even by tasks which have never called down().
- */
 extern void up(struct semaphore *sem);
 
 #endif /* __LINUX_SEMAPHORE_H */

kernel/semaphore.c +68 ~~-11~~

127 lines not on this page · raw message

Signature · 4 lines

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

Randy Dunlap

2008-04-11 20:29 UTC #4 lore raw

Quoted 23 lines · On Fri, 11 Apr 2008 13:21:54 -0600 Matthew Wilcox wrote:

Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Thu, 10 Apr 2008 16:08:16 -0600 Matthew Wilcox <[email protected]> wrote:
It seems very strange to me to document the API with the implementation rather than with the declaration. It's almost as if we expect people to have to read the implementation to figure out how stuff works.
That approach makes sense for C++. But for C, the code is .c-centric.
I've never programmed in C++ ... I just expect to find API documentation in header files.
Quoted 4 lines
That's particularly the case with the kernel, where we explicitly work to make the .c files the things which people look at, while not caring about the .h files. Look at how much we say "get that ifdef out of there and hide it in the header file".
I see that as being "move the complexity around" and "get the interfaces right", not "hide it in the header files where nobody ever looks".
Quoted 3 lines
Quoted 2 lines
How about a note in semaphore.c that says "refer to semaphore.h for usage information"?

Quoted 6 lines

Quoted 1 line
No, please document it in the C file, where people expect to find it.
Fine, I've done it the other way round.
Please review this doc-patch. Without comments, I'll commit it to the semaphore git tree tomorrow.

Looks good to me. Thanks.

---
~Randy

Andrew Morton

2008-04-12 05:10 UTC #5 lore raw

Quoted 25 lines · On Fri, 11 Apr 2008 13:27:54 -0700 Randy Dunlap <[email protected]> wrote:

Quoted 23 lines · On Fri, 11 Apr 2008 13:21:54 -0600 Matthew Wilcox wrote:
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Thu, 10 Apr 2008 16:08:16 -0600 Matthew Wilcox <[email protected]> wrote:
It seems very strange to me to document the API with the implementation rather than with the declaration. It's almost as if we expect people to have to read the implementation to figure out how stuff works.
That approach makes sense for C++. But for C, the code is .c-centric.
I've never programmed in C++ ... I just expect to find API documentation in header files.
Quoted 4 lines
That's particularly the case with the kernel, where we explicitly work to make the .c files the things which people look at, while not caring about the .h files. Look at how much we say "get that ifdef out of there and hide it in the header file".
I see that as being "move the complexity around" and "get the interfaces right", not "hide it in the header files where nobody ever looks".
Quoted 3 lines
Quoted 2 lines
How about a note in semaphore.c that says "refer to semaphore.h for usage information"?

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
No, please document it in the C file, where people expect to find it.
Fine, I've done it the other way round.
Please review this doc-patch. Without comments, I'll commit it to the semaphore git tree tomorrow.
Looks good to me. Thanks.

Yup, most excellent.

btw, down() and friends should have might_sleep() checks in them, shouldn't they? They don't seem to be in there, nor in mainline lib/semaphore-sleepers.c. Confused.

Matthew Wilcox

author

2008-04-12 14:13 UTC #6 lore raw

Quoted 4 lines · Andrew Morton

Quoted 1 line · On Fri, 11 Apr 2008 13:27:54 -0700 Randy Dunlap <[email protected]> wrote:
Looks good to me. Thanks.
Yup, most excellent.

Thanks for the review.

Quoted 3 lines

btw, down() and friends should have might_sleep() checks in them, shouldn't they? They don't seem to be in there, nor in mainline lib/semaphore-sleepers.c. Confused.

Mmm. Ingo gets annoyed when I add additional checks to semaphores -- he wants them to maintain their current semantics and to get better checking by migrating more users to mutexes. I've already exposed at least one problem (in aacraid) by adding the __must_check to down_interruptible().

As I wrote in one of the comments, we have places in the kernel which know that even though they're in a non-sleeping context, there is at least one more token left in the semaphore. One place this bit me was in start_kernel(). We disable interrupts and then call lock_kernel() which calls down(). Since we're in start_kernel(), we know there's nothing else running and this is perfectly safe. But a might_sleep() would warn bogusly.

I'd be open to putting a might_sleep() in __down(). We definitely are going to sleep at that point, so getting a warning out of it would be good. I thought that schedule() would warn itself in that case, but I can't see the code that would do that now I check.

Signature · 4 lines

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

Andrew Morton

2008-04-12 19:28 UTC #7 lore raw

Quoted 17 lines · On Sat, 12 Apr 2008 08:12:51 -0600 Matthew Wilcox <[email protected]> wrote:

Quoted 4 lines · Andrew Morton
Quoted 1 line · On Fri, 11 Apr 2008 13:27:54 -0700 Randy Dunlap <[email protected]> wrote:
Looks good to me. Thanks.
Yup, most excellent.
Thanks for the review.
Quoted 3 lines
btw, down() and friends should have might_sleep() checks in them, shouldn't they? They don't seem to be in there, nor in mainline lib/semaphore-sleepers.c. Confused.
Mmm. Ingo gets annoyed when I add additional checks to semaphores -- he wants them to maintain their current semantics and to get better checking by migrating more users to mutexes. I've already exposed at least one problem (in aacraid) by adding the __must_check to down_interruptible().

Quoted 7 lines

As I wrote in one of the comments, we have places in the kernel which know that even though they're in a non-sleeping context, there is at least one more token left in the semaphore. One place this bit me was in start_kernel(). We disable interrupts and then call lock_kernel() which calls down(). Since we're in start_kernel(), we know there's nothing else running and this is perfectly safe. But a might_sleep() would warn bogusly.

urgh, yes, I'd forgotten about that mess.

I suppose that if might_sleep() checking in down() is useful (and surely it is) we could provide a separate down_im_stupid() (and lock_kernel_im_stupid()) which omits the check, and call that from the problematic sites.

Quoted 3 lines

I'd be open to putting a might_sleep() in __down(). We definitely are going to sleep at that point, so getting a warning out of it would be good.

I think it'd be worth playing with some time, but it's off-topic for this current work.

Quoted 2 lines

I thought that schedule() would warn itself in that case,
but I can't see the code that would do that now I check.

schedule() will warn ("scheduling while atomic"), but only if we happened to hit contention.

David Woodhouse

2008-04-15 08:25 UTC #8 lore raw

In reply to Matthew Wilcox

Quoted 7 lines · Matthew Wilcox

As I wrote in one of the comments, we have places in the kernel which know that even though they're in a non-sleeping context, there is at least one more token left in the semaphore. One place this bit me was in start_kernel(). We disable interrupts and then call lock_kernel() which calls down(). Since we're in start_kernel(), we know there's nothing else running and this is perfectly safe. But a might_sleep() would warn bogusly.

I would have thought they'd use down_trylock() in that case.

Signature · 1 line

dwmw2