Re: [2/4] consolidate bit waiting code patterns

William Lee Irwin III
2004-08-28T10:21:38Z

On Sat, Aug 28, 2004 at 02:34:36AM -0700, Andrew Morton wrote:
> Some comments over this thing would be nice.
> The `wait' argument seems to be misnamed. It typically points at
> sync_page(), yes? Maybe it should be called `action' or `kick' or
> something.
> If (*wait)() returns non-zero then it looks to me like the callers will get
> confused, thinking that the page did come unlocked?
> If (*wait)() returns non-zero then we need to run finish_wait(), no?

You're right, that should be a break; and return ret; with ret
initialized to 0. Mysteriously, I never hit this even in combination
testing with aio.

Index: mm1-2.6.9-rc1/kernel/fork.c
===================================================================
--- mm1-2.6.9-rc1.orig/kernel/fork.c 2004-08-28 01:23:05.410362856 -0700
+++ mm1-2.6.9-rc1/kernel/fork.c 2004-08-28 02:47:05.712120520 -0700
@@ -261,37 +261,40 @@
}
EXPORT_SYMBOL(__wake_up_bit);

+/*
+ * To allow interruptible waiting and asynchronous (i.e. nonblocking)
+ * waiting, the actions of __wait_on_bit() and __wait_on_bit_lock() are
+ * permitted return codes. Nonzero return codes halt waiting and return.
+ */
int __sched __wait_on_bit(wait_queue_head_t *wq, struct wait_bit_queue *q,
void *word,
- int bit, int (*wait)(void *), unsigned mode)
+ int bit, int (*action)(void *), unsigned mode)
{
- int ret;
+ int ret = 0;

prepare_to_wait(wq, &q->wait, mode);
- if (test_bit(bit, word)) {
- if ((ret = (*wait)(word)))
- return ret;
- }
+ if (test_bit(bit, word))
+ ret = (*action)(word);
finish_wait(wq, &q->wait);
- return 0;
+ return ret;
}
EXPORT_SYMBOL(__wait_on_bit);

int __sched __wait_on_bit_lock(wait_queue_head_t *wq, struct wait_bit_queue *q,
void *word, int bit,
- int (*wait)(void *), unsigned mode)
+ int (*action)(void *), unsigned mode)
{
- int ret;
+ int ret = 0;

while (test_and_set_bit(bit, word)) {
prepare_to_wait_exclusive(wq, &q->wait, mode);
if (test_bit(bit, word)) {
- if ((ret = (*wait)(word)))
- return ret;
+ if ((ret = (*action)(word)))
+ break;
}
}
finish_wait(wq, &q->wait);
- return 0;
+ return ret;
}
EXPORT_SYMBOL(__wait_on_bit_lock);

Index: mm1-2.6.9-rc1/include/linux/wait.h
===================================================================
--- mm1-2.6.9-rc1.orig/include/linux/wait.h 2004-08-28 01:44:13.448591744 -0700
+++ mm1-2.6.9-rc1/include/linux/wait.h 2004-08-28 02:45:22.458817408 -0700
@@ -343,7 +343,7 @@
* wait_on_bit - wait for a bit to be cleared
* @word: the word being waited on, a kernel virtual address
* @bit: the bit of the word being waited on
- * @wait: the function used to sleep, which may take special actions
+ * @action: the function used to sleep, which may take special actions
* @mode: the task state to sleep in
*
* There is a standard hashed waitqueue table for generic use. This
@@ -354,19 +354,19 @@
* but has no intention of setting it.
*/
static inline int wait_on_bit(void *word, int bit,
- int (*wait)(void *), unsigned mode)
+ int (*action)(void *), unsigned mode)
{
DEFINE_WAIT_BIT(q, word, bit);
wait_queue_head_t *wqh = bit_waitqueue(word, bit);

- return __wait_on_bit(wqh, &q, word, bit, wait, mode);
+ return __wait_on_bit(wqh, &q, word, bit, action, mode);
}

/**
* wait_on_bit_lock - wait for a bit to be cleared, when wanting to set it
* @word: the word being waited on, a kernel virtual address
* @bit: the bit of the word being waited on
- * @wait: the function used to sleep, which may take special actions
+ * @action: the function used to sleep, which may take special actions
* @mode: the task state to sleep in
*
* There is a standard hashed waitqueue table for generic use. This
@@ -379,12 +379,12 @@
* clear with the intention of setting it, and when done, clearing it.
*/
static inline int wait_on_bit_lock(void *word, int bit,
- int (*wait)(void *), unsigned mode)
+ int (*action)(void *), unsigned mode)
{
DEFINE_WAIT_BIT(q, word, bit);
wait_queue_head_t *wqh = bit_waitqueue(word, bit);

- return __wait_on_bit_lock(wqh, &q, word, bit, wait, mode);
+ return __wait_on_bit_lock(wqh, &q, word, bit, action, mode);
}

#endif /* __KERNEL__ */