Hello Linus,
This patch against 2.5.22 adds support for wait queue function
callbacks, which are used by aio to build async read / write
operations on top of existing wait queues at points that would
normally block a process. Comments?
-ben
--
"You will be reincarnated as a toad; and you will be much happier."
:r ~/patches/v2.5/v2.5.22-waitqueue-func.diff
diff -urN v2.5.22/include/linux/wait.h wq-func-v2.5.22.diff/include/linux/wait.h
--- v2.5.22/include/linux/wait.h Mon Jun 10 21:41:10 2002
+++ wq-func-v2.5.22.diff/include/linux/wait.h Mon Jun 17 15:53:25 2002
@@ -19,13 +19,16 @@
#include <asm/page.h>
#include <asm/processor.h>
+typedef struct __wait_queue wait_queue_t;
+typedef void (*wait_queue_func_t)(wait_queue_t *wait);
+
struct __wait_queue {
unsigned int flags;
#define WQ_FLAG_EXCLUSIVE 0x01
struct task_struct * task;
+ wait_queue_func_t func;
struct list_head task_list;
};
-typedef struct __wait_queue wait_queue_t;
struct __wait_queue_head {
spinlock_t lock;
@@ -40,6 +43,7 @@
#define __WAITQUEUE_INITIALIZER(name, tsk) { \
task: tsk, \
+ func: NULL, \
task_list: { NULL, NULL } }
#define DECLARE_WAITQUEUE(name, tsk) \
@@ -62,6 +66,15 @@
{
q->flags = 0;
q->task = p;
+ q->func = NULL;
+}
+
+static inline void init_waitqueue_func_entry(wait_queue_t *q,
+ wait_queue_func_t func)
+{
+ q->flags = 0;
+ q->task = NULL;
+ q->func = func;
}
static inline int waitqueue_active(wait_queue_head_t *q)
@@ -89,6 +102,22 @@
list_del(&old->task_list);
}
+#define add_wait_queue_cond(q, wait, cond) \
+ ({ \
+ unsigned long flags; \
+ int _raced = 0; \
+ wq_write_lock_irqsave(&(q)->lock, flags); \
+ (wait)->flags = 0; \
+ __add_wait_queue((q), (wait)); \
+ rmb(); \
+ if (!(cond)) { \
+ _raced = 1; \
+ __remove_wait_queue((q), (wait)); \
+ } \
+ wq_write_unlock_irqrestore(&(q)->lock, flags); \
+ _raced; \
+ })
+
#endif /* __KERNEL__ */
#endif
diff -urN v2.5.22/kernel/sched.c wq-func-v2.5.22.diff/kernel/sched.c
--- v2.5.22/kernel/sched.c Mon Jun 17 15:41:42 2002
+++ wq-func-v2.5.22.diff/kernel/sched.c Mon Jun 17 16:00:03 2002
@@ -916,13 +916,22 @@
*/
static inline void __wake_up_common(wait_queue_head_t *q, unsigned int mode, int nr_exclusive, int sync)
{
- struct list_head *tmp;
+ struct list_head *tmp, *next;
unsigned int state;
wait_queue_t *curr;
task_t *p;
- list_for_each(tmp, &q->task_list) {
+ list_for_each_safe(tmp, next, &q->task_list) {
+ wait_queue_func_t func;
curr = list_entry(tmp, wait_queue_t, task_list);
+ func = curr->func;
+ if (func) {
+ unsigned flags = curr->flags;
+ func(curr);
+ if ((flags & WQ_FLAG_EXCLUSIVE) && !--nr_exclusive)
+ break;
+ continue;
+ }
p = curr->task;
state = p->state;
if ((state & mode) && try_to_wake_up(p, sync) &&
On Mon, Jun 17, 2002 at 04:14:34PM -0400, Benjamin LaHaise wrote:
> +#define add_wait_queue_cond(q, wait, cond) \
> + ({ \
> + unsigned long flags; \
> + int _raced = 0; \
> + wq_write_lock_irqsave(&(q)->lock, flags); \
I thought we killed off wq_write_lock_irqsave 1-2 kernels ago ?
Dave
--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs
On Mon, Jun 17, 2002 at 10:28:12PM +0200, Dave Jones wrote:
> On Mon, Jun 17, 2002 at 04:14:34PM -0400, Benjamin LaHaise wrote:
> > +#define add_wait_queue_cond(q, wait, cond) \
> > + ({ \
> > + unsigned long flags; \
> > + int _raced = 0; \
> > + wq_write_lock_irqsave(&(q)->lock, flags); \
>
> I thought we killed off wq_write_lock_irqsave 1-2 kernels ago ?
Ah, I didn't notice that as I've not reached the point of merging
everything into a working build -- I'd like to get some feedback
and comments on the mergable units as they become available, since
I'm rewriting a few portions to fix problems that experience with
the code revealed. Anyways, the patch below changes add_wait_queue_cond
to use spin_locks directly.
-ben
diff -urN wq-func-v2.5.22.diff/include/linux/wait.h wq-func-v2.5.22-b.diff/include/linux/wait.h
--- wq-func-v2.5.22.diff/include/linux/wait.h Mon Jun 17 15:53:25 2002
+++ wq-func-v2.5.22-b.diff/include/linux/wait.h Mon Jun 17 16:46:23 2002
@@ -106,7 +106,7 @@
({ \
unsigned long flags; \
int _raced = 0; \
- wq_write_lock_irqsave(&(q)->lock, flags); \
+ spin_lock_irqsave(&(q)->lock, flags); \
(wait)->flags = 0; \
__add_wait_queue((q), (wait)); \
rmb(); \
@@ -114,7 +114,7 @@
_raced = 1; \
__remove_wait_queue((q), (wait)); \
} \
- wq_write_unlock_irqrestore(&(q)->lock, flags); \
+ spin_lock_irqrestore(&(q)->lock, flags); \
_raced; \
})
On Mon, Jun 17, 2002 at 10:28:12PM +0200, Dave Jones wrote:
> On Mon, Jun 17, 2002 at 04:14:34PM -0400, Benjamin LaHaise wrote:
> > +#define add_wait_queue_cond(q, wait, cond) \
> > + ({ \
> > + unsigned long flags; \
> > + int _raced = 0; \
> > + wq_write_lock_irqsave(&(q)->lock, flags); \
>
> I thought we killed off wq_write_lock_irqsave 1-2 kernels ago ?
>
> Dave
>
> --
> | Dave Jones. http://www.codemonkey.org.uk
> | SuSE Labs
> -
Dave,
It depends on what you mean by killed off. I submitted a patch to Linus back
at 2.5.3 to clean up the way the completion code called the wait queue
interface. This interface got added then. You picked up those changes at
that time (and still have them in your kernel tree) but the changes have
never made it into Linus' tree.
So, Linus has never had the code to 'kill' and you've never dropped it
after picking it up.
--
Bob Miller Email: [email protected]
Open Source Development Lab Phone: 503.626.2455 Ext. 17
On Mon, 17 Jun 2002, Benjamin LaHaise wrote:
>
> Anyways, the patch below changes add_wait_queue_cond
> to use spin_locks directly.
Can you do one more thing for me: change the "NULL" in the wake function
to be a "default_wake_function()" that looks something like
int default_wake_function(task_t *p, unsigned int mode, int sync)
{
return (p->state & mode) && try_to_wake_up(p, sunc);
}
and then you just make the code in __wake_up_common() do
list_for_each(tmp, &q->task_list) {
curr = list_entry(tmp, wait_queue_t, task_list);
p = curr->task;
if (curr->func(p, mode, sync) &&
((curr->flags & WQ_FLAG_EXCLUSIVE) && !--nr_exclusive))
break;
}
instead of having separate code-paths for the "func" and the default
wake-up action.
In short, the event "func" could never be NULL, and would always return
whether the wakeup/event was "successful" from an exclusivity standpoint.
Linus
On Mon, Jun 17, 2002 at 01:57:44PM -0700, Bob Miller wrote:
> > I thought we killed off wq_write_lock_irqsave 1-2 kernels ago ?
> It depends on what you mean by killed off. I submitted a patch to Linus back
> at 2.5.3 to clean up the way the completion code called the wait queue
> interface. This interface got added then. You picked up those changes at
> that time (and still have them in your kernel tree) but the changes have
> never made it into Linus' tree.
>
> So, Linus has never had the code to 'kill' and you've never dropped it
> after picking it up.
Your patch was to use wq_write_lock and friends in sched.c iirc.
That change is now removed from my tree (though I've not put up a
version containing that change yet).
Since 2.5.20 or so, the wq_write_lock functions are dead as in gone.
Not around, Extinct. They are ex-functions.
Dave
--
| Dave Jones. http://www.codemonkey.org.uk
| SuSE Labs
On Mon, 2002-06-17 at 13:57, Bob Miller wrote:
> It depends on what you mean by killed off. I submitted a patch to Linus back
> at 2.5.3 to clean up the way the completion code called the wait queue
> interface. This interface got added then. You picked up those changes at
> that time (and still have them in your kernel tree) but the changes have
> never made it into Linus' tree.
>
> So, Linus has never had the code to 'kill' and you've never dropped it
> after picking it up.
Work has gone in since this.
During 2.5.20, Linus asked for and I submitted a patch to remove the
whole wq_lock_t mess altogether. It was merged into 2.5.21.
Subsequently, there is no wq_lock_t abstraction in current 2.5 kernels
and code should use a standard spinlock.
Robert Love
On Mon, Jun 17, 2002 at 11:08:32PM +0200, Dave Jones wrote:
> Your patch was to use wq_write_lock and friends in sched.c iirc.
> That change is now removed from my tree (though I've not put up a
> version containing that change yet).
>
> Since 2.5.20 or so, the wq_write_lock functions are dead as in gone.
> Not around, Extinct. They are ex-functions.
>
> Dave
Always look before you leap. Just looked a 2.5.20 kernel and these
are indeed gone. Sorry for the fire drill.
--
Bob Miller Email: [email protected]
Open Source Development Lab Phone: 503.626.2455 Ext. 17
On Mon, Jun 17, 2002 at 02:02:22PM -0700, Linus Torvalds wrote:
> In short, the event "func" could never be NULL, and would always return
> whether the wakeup/event was "successful" from an exclusivity standpoint.
How's the patch below? The main reason for passing in the pointer to
the wait queue structure is that the aio functions need to remove
themselves from the wait list if the event they were waiting for occurs.
It seems to boot for me, how about others?
-ben
--
"You will be reincarnated as a toad; and you will be much happier."
diff -urN v2.5.22/include/linux/wait.h wq-func-v2.5.22-c.diff/include/linux/wait.h
--- v2.5.22/include/linux/wait.h Mon Jun 10 21:41:10 2002
+++ wq-func-v2.5.22-c.diff/include/linux/wait.h Mon Jun 17 17:34:09 2002
@@ -19,13 +19,17 @@
#include <asm/page.h>
#include <asm/processor.h>
+typedef struct __wait_queue wait_queue_t;
+typedef int (*wait_queue_func_t)(wait_queue_t *wait, unsigned mode, int sync);
+extern int default_wake_function(wait_queue_t *wait, unsigned mode, int sync);
+
struct __wait_queue {
unsigned int flags;
#define WQ_FLAG_EXCLUSIVE 0x01
struct task_struct * task;
+ wait_queue_func_t func;
struct list_head task_list;
};
-typedef struct __wait_queue wait_queue_t;
struct __wait_queue_head {
spinlock_t lock;
@@ -40,13 +44,14 @@
#define __WAITQUEUE_INITIALIZER(name, tsk) { \
task: tsk, \
+ func: default_wake_function, \
task_list: { NULL, NULL } }
#define DECLARE_WAITQUEUE(name, tsk) \
wait_queue_t name = __WAITQUEUE_INITIALIZER(name, tsk)
#define __WAIT_QUEUE_HEAD_INITIALIZER(name) { \
- lock: SPIN_LOCK_UNLOCKED, \
+ lock: SPIN_LOCK_UNLOCKED, \
task_list: { &(name).task_list, &(name).task_list } }
#define DECLARE_WAIT_QUEUE_HEAD(name) \
@@ -62,6 +67,15 @@
{
q->flags = 0;
q->task = p;
+ q->func = default_wake_function;
+}
+
+static inline void init_waitqueue_func_entry(wait_queue_t *q,
+ wait_queue_func_t func)
+{
+ q->flags = 0;
+ q->task = NULL;
+ q->func = func;
}
static inline int waitqueue_active(wait_queue_head_t *q)
@@ -89,6 +103,22 @@
list_del(&old->task_list);
}
+#define add_wait_queue_cond(q, wait, cond) \
+ ({ \
+ unsigned long flags; \
+ int _raced = 0; \
+ spin_lock_irqsave(&(q)->lock, flags); \
+ (wait)->flags = 0; \
+ __add_wait_queue((q), (wait)); \
+ rmb(); \
+ if (!(cond)) { \
+ _raced = 1; \
+ __remove_wait_queue((q), (wait)); \
+ } \
+ spin_lock_irqrestore(&(q)->lock, flags); \
+ _raced; \
+ })
+
#endif /* __KERNEL__ */
#endif
diff -urN v2.5.22/kernel/sched.c wq-func-v2.5.22-c.diff/kernel/sched.c
--- v2.5.22/kernel/sched.c Mon Jun 17 15:41:42 2002
+++ wq-func-v2.5.22-c.diff/kernel/sched.c Mon Jun 17 17:33:38 2002
@@ -905,6 +905,12 @@
}
#endif /* CONFIG_PREEMPT */
+int default_wake_function(wait_queue_t *curr, unsigned mode, int sync)
+{
+ task_t *p = curr->task;
+ return ((p->state & mode) && try_to_wake_up(p, sync));
+}
+
/*
* The core wakeup function. Non-exclusive wakeups (nr_exclusive == 0) just
* wake everything up. If it's an exclusive wakeup (nr_exclusive == small +ve
@@ -916,18 +922,17 @@
*/
static inline void __wake_up_common(wait_queue_head_t *q, unsigned int mode, int nr_exclusive, int sync)
{
- struct list_head *tmp;
- unsigned int state;
- wait_queue_t *curr;
- task_t *p;
+ struct list_head *tmp, *next;
- list_for_each(tmp, &q->task_list) {
+ list_for_each_safe(tmp, next, &q->task_list) {
+ wait_queue_t *curr;
+ unsigned flags;
curr = list_entry(tmp, wait_queue_t, task_list);
- p = curr->task;
- state = p->state;
- if ((state & mode) && try_to_wake_up(p, sync) &&
- ((curr->flags & WQ_FLAG_EXCLUSIVE) && !--nr_exclusive))
- break;
+ flags = curr->flags;
+ if (curr->func(curr, mode, sync) &&
+ (flags & WQ_FLAG_EXCLUSIVE) &&
+ !--nr_exclusive)
+ break;
}
}
On Mon, 17 Jun 2002, Benjamin LaHaise wrote:
>
> How's the patch below? The main reason for passing in the pointer to
> the wait queue structure is that the aio functions need to remove
> themselves from the wait list if the event they were waiting for occurs.
> It seems to boot for me, how about others?
Looks ok at first glance, although I haven't booted yet.
One thing strikes me: we could move the "flags & WQ_FLAGS_EXCLUSIVE" test
also into the wakeup function - making the "exclusivity" depend on which
wakeup function you use. Does that make any sense? I'm not 100% convinced,
but it would mean that the normal non-exclusive stuff would never even
have to test the thing at run-time.
Linus