[patch 2/6] epoll keyed wakeups v2 - introduce new *_poll() wakeup macros

[patch 2/6] epoll keyed wakeups v2 - introduce new *_poll() wakeup macros

[Davide Libenzi]

This patch introduces new wakeup macros that allow passing an event
mask to the wakeup targets. They exactly mimic their non-_poll()
counterpart, with the added event mask passing capability.
I did add only the ones currently requested, avoiding the _nr() and
_all() for the moment.



Signed-off-by: Davide Libenzi <davidel@xmailserver.org>


- Davide


---
 include/linux/wait.h |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Index: linux-2.6.mod/include/linux/wait.h
===================================================================
--- linux-2.6.mod.orig/include/linux/wait.h	2009-01-31 15:24:50.000000000 -0800
+++ linux-2.6.mod/include/linux/wait.h	2009-01-31 15:33:40.000000000 -0800
@@ -156,6 +156,18 @@ wait_queue_head_t *bit_waitqueue(void *,
 #define wake_up_interruptible_all(x)	__wake_up(x, TASK_INTERRUPTIBLE, 0, NULL)
 #define wake_up_interruptible_sync(x)	__wake_up_sync((x), TASK_INTERRUPTIBLE, 1)
 
+/*
+ * Wakeup macros to be used to report events to the targets.
+ */
+#define wake_up_poll(x, m)				\
+	__wake_up(x, TASK_NORMAL, 1, (void *) (m))
+#define wake_up_locked_poll(x, m)				\
+	__wake_up_locked_key((x), TASK_NORMAL, (void *) (m))
+#define wake_up_interruptible_poll(x, m)			\
+	__wake_up(x, TASK_INTERRUPTIBLE, 1, (void *) (m))
+#define wake_up_interruptible_sync_poll(x, m)				\
+	__wake_up_sync_key((x), TASK_INTERRUPTIBLE, 1, (void *) (m))
+
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 /*
  * macro to avoid include hell
@@ -168,8 +180,17 @@ do {									\
 	wake_up_locked(x); 						\
 	spin_unlock_irqrestore(&(x)->lock, flags);			\
 } while (0)
+#define wake_up_nested_poll(x, m, s)					\
+do {									\
+	unsigned long flags;						\
+									\
+	spin_lock_irqsave_nested(&(x)->lock, flags, (s));		\
+	wake_up_locked_poll(x, m);					\
+	spin_unlock_irqrestore(&(x)->lock, flags);			\
+} while (0)
 #else
 #define wake_up_nested(x, s)		wake_up(x)
+#define wake_up_nested_poll(x, m, s)	wake_up_poll(x, m)
 #endif
 
 #define __wait_event(wq, condition) 					\

Re: [patch 2/6] epoll keyed wakeups v2 - introduce new *_poll() wakeup macros

[Andrew Morton]

On Sun, 01 Feb 2009 12:04:23 -0800 Davide Libenzi <davidel@xmailserver.org> wrote:

> +#define wake_up_nested_poll(x, m, s)					\
> +do {									\
> +	unsigned long flags;						\
> +									\
> +	spin_lock_irqsave_nested(&(x)->lock, flags, (s));		\
> +	wake_up_locked_poll(x, m);					\
> +	spin_unlock_irqrestore(&(x)->lock, flags);			\
> +} while (0)

I had to go and find the callsite to work out the type of `x' :(

- this macro can be passed the address of any structure which has a
  `spinlock_t lock;' in it, which seems strange.

- It references its first arg three times.

Is there any reason why we can't implement this in C?  

Re: [patch 2/6] epoll keyed wakeups v2 - introduce new *_poll() wakeup macros

[Davide Libenzi]

On Tue, 3 Feb 2009, Andrew Morton wrote:

> On Sun, 01 Feb 2009 12:04:23 -0800 Davide Libenzi <davidel@xmailserver.org> wrote:
> 
> > +#define wake_up_nested_poll(x, m, s)					\
> > +do {									\
> > +	unsigned long flags;						\
> > +									\
> > +	spin_lock_irqsave_nested(&(x)->lock, flags, (s));		\
> > +	wake_up_locked_poll(x, m);					\
> > +	spin_unlock_irqrestore(&(x)->lock, flags);			\
> > +} while (0)
> 
> I had to go and find the callsite to work out the type of `x' :(
> 
> - this macro can be passed the address of any structure which has a
>   `spinlock_t lock;' in it, which seems strange.
> 
> - It references its first arg three times.
> 
> Is there any reason why we can't implement this in C?  

I don't see any reason why these two couldn't be normal functions (I 
just referenced wake_up_nested(), that was a macro in the first place). 
Probably even un-inlined?



- Davide

Re: [patch 2/6] epoll keyed wakeups v2 - introduce new *_poll() wakeup macros

[Davide Libenzi]

On Tue, 3 Feb 2009, Davide Libenzi wrote:

> On Tue, 3 Feb 2009, Andrew Morton wrote:
> 
> > On Sun, 01 Feb 2009 12:04:23 -0800 Davide Libenzi <davidel@xmailserver.org> wrote:
> > 
> > > +#define wake_up_nested_poll(x, m, s)					\
> > > +do {									\
> > > +	unsigned long flags;						\
> > > +									\
> > > +	spin_lock_irqsave_nested(&(x)->lock, flags, (s));		\
> > > +	wake_up_locked_poll(x, m);					\
> > > +	spin_unlock_irqrestore(&(x)->lock, flags);			\
> > > +} while (0)
> > 
> > I had to go and find the callsite to work out the type of `x' :(
> > 
> > - this macro can be passed the address of any structure which has a
> >   `spinlock_t lock;' in it, which seems strange.
> > 
> > - It references its first arg three times.
> > 
> > Is there any reason why we can't implement this in C?  
> 
> I don't see any reason why these two couldn't be normal functions (I 
> just referenced wake_up_nested(), that was a macro in the first place). 

Actually reading the comments helps :) It triggers an include-hell, if you 
make them inline. Since they're lockdep debug thingies, I think it's kinda 
wasted turn them into non-inline real functions, so they'd better remain 
macros IMO.



- Davide

Re: [patch 2/6] epoll keyed wakeups v2 - introduce new *_poll() wakeup macros

[Andrew Morton]

On Tue, 3 Feb 2009 11:20:46 -0800 (PST)
Davide Libenzi <davidel@xmailserver.org> wrote:

> On Tue, 3 Feb 2009, Davide Libenzi wrote:
> 
> > On Tue, 3 Feb 2009, Andrew Morton wrote:
> > 
> > > On Sun, 01 Feb 2009 12:04:23 -0800 Davide Libenzi <davidel@xmailserver.org> wrote:
> > > 
> > > > +#define wake_up_nested_poll(x, m, s)					\
> > > > +do {									\
> > > > +	unsigned long flags;						\
> > > > +									\
> > > > +	spin_lock_irqsave_nested(&(x)->lock, flags, (s));		\
> > > > +	wake_up_locked_poll(x, m);					\
> > > > +	spin_unlock_irqrestore(&(x)->lock, flags);			\
> > > > +} while (0)
> > > 
> > > I had to go and find the callsite to work out the type of `x' :(
> > > 
> > > - this macro can be passed the address of any structure which has a
> > >   `spinlock_t lock;' in it, which seems strange.
> > > 
> > > - It references its first arg three times.
> > > 
> > > Is there any reason why we can't implement this in C?  
> > 
> > I don't see any reason why these two couldn't be normal functions (I 
> > just referenced wake_up_nested(), that was a macro in the first place). 
> 
> Actually reading the comments helps :) It triggers an include-hell, if you 
> make them inline. Since they're lockdep debug thingies, I think it's kinda 
> wasted turn them into non-inline real functions, so they'd better remain 
> macros IMO.
> 

ho hum.  I think it'd be worth at least renaming the arguments to
something less daft, for readability reasons.

One could also do

do {
	wait_queue_head_t *__wqh = x;
	<use __wqh>
}

which would provide typechecking of the first arg (so people can no
longer "pass the address of any structure which has a `spinlock_t
lock;' in it") and which fixes the multiple-references-to-an-argument
issue.

Re: [patch 2/6] epoll keyed wakeups v2 - introduce new *_poll() wakeup macros

[Davide Libenzi]

On Tue, 3 Feb 2009, Andrew Morton wrote:

> On Tue, 3 Feb 2009 11:20:46 -0800 (PST)
> Davide Libenzi <davidel@xmailserver.org> wrote:
> 
> > On Tue, 3 Feb 2009, Davide Libenzi wrote:
> > 
> > > On Tue, 3 Feb 2009, Andrew Morton wrote:
> > > 
> > > > On Sun, 01 Feb 2009 12:04:23 -0800 Davide Libenzi <davidel@xmailserver.org> wrote:
> > > > 
> > > > > +#define wake_up_nested_poll(x, m, s)					\
> > > > > +do {									\
> > > > > +	unsigned long flags;						\
> > > > > +									\
> > > > > +	spin_lock_irqsave_nested(&(x)->lock, flags, (s));		\
> > > > > +	wake_up_locked_poll(x, m);					\
> > > > > +	spin_unlock_irqrestore(&(x)->lock, flags);			\
> > > > > +} while (0)
> > > > 
> > > > I had to go and find the callsite to work out the type of `x' :(
> > > > 
> > > > - this macro can be passed the address of any structure which has a
> > > >   `spinlock_t lock;' in it, which seems strange.
> > > > 
> > > > - It references its first arg three times.
> > > > 
> > > > Is there any reason why we can't implement this in C?  
> > > 
> > > I don't see any reason why these two couldn't be normal functions (I 
> > > just referenced wake_up_nested(), that was a macro in the first place). 
> > 
> > Actually reading the comments helps :) It triggers an include-hell, if you 
> > make them inline. Since they're lockdep debug thingies, I think it's kinda 
> > wasted turn them into non-inline real functions, so they'd better remain 
> > macros IMO.
> > 
> 
> ho hum.  I think it'd be worth at least renaming the arguments to
> something less daft, for readability reasons.
> 
> One could also do
> 
> do {
> 	wait_queue_head_t *__wqh = x;
> 	<use __wqh>
> }
> 
> which would provide typechecking of the first arg (so people can no
> longer "pass the address of any structure which has a `spinlock_t
> lock;' in it") and which fixes the multiple-references-to-an-argument
> issue.

OK, you've got my arg-rename bits already. Another version using a 
typecheck() in there? Or you prefer the explicit instantiation?


- Davide

Re: [patch 2/6] epoll keyed wakeups v2 - introduce new *_poll() wakeup macros

[Linus Torvalds]

On Tue, 3 Feb 2009, Andrew Morton wrote:
> > 
> > Actually reading the comments helps :) It triggers an include-hell, if you 
> > make them inline. Since they're lockdep debug thingies, I think it's kinda 
> > wasted turn them into non-inline real functions, so they'd better remain 
> > macros IMO.
> 
> ho hum.  I think it'd be worth at least renaming the arguments to
> something less daft, for readability reasons.

Also, maybe we can make it an out-of-line thing rather than an inline?

Why not just make it

	extern void wake_up_nested[_poll](wait_queue_head_t *,
		[unsigned long pollflags, ] unsigned int nesting);

and then just move it out of a header file entirely by writing it as a 
real function in some *.c file that only gets compiled with 
DEBUG_LOCK_ALLOC.

Hmm? No include hell.

			Linus

Re: [patch 2/6] epoll keyed wakeups v2 - introduce new *_poll() wakeup macros

[Davide Libenzi]

On Tue, 3 Feb 2009, Linus Torvalds wrote:

> 
> 
> On Tue, 3 Feb 2009, Andrew Morton wrote:
> > > 
> > > Actually reading the comments helps :) It triggers an include-hell, if you 
> > > make them inline. Since they're lockdep debug thingies, I think it's kinda 
> > > wasted turn them into non-inline real functions, so they'd better remain 
> > > macros IMO.
> > 
> > ho hum.  I think it'd be worth at least renaming the arguments to
> > something less daft, for readability reasons.
> 
> Also, maybe we can make it an out-of-line thing rather than an inline?
> 
> Why not just make it
> 
> 	extern void wake_up_nested[_poll](wait_queue_head_t *,
> 		[unsigned long pollflags, ] unsigned int nesting);
> 
> and then just move it out of a header file entirely by writing it as a 
> real function in some *.c file that only gets compiled with 
> DEBUG_LOCK_ALLOC.
> 
> Hmm? No include hell.

There's another thing. Epoll is the *only* user of such thing. I could 
suck it in there if you prefer. wake_up_nested(), once 
wake_up_nested_poll() goes in, has no more users and IMO can go.
Otherwise yes, we could define:

void wake_up_nested_poll(wait_queue_head_t *q, unsigned long events,
                         int subclass);

in wait.h, and define it under DEBUG_LOCK_ALLOC in sched.c.
Let me know what you'd like best ...



- Davide

Re: [patch 2/6] epoll keyed wakeups v2 - introduce new *_poll() wakeup macros

[Linus Torvalds]

On Tue, 3 Feb 2009, Davide Libenzi wrote:
> 
> There's another thing. Epoll is the *only* user of such thing. I could 
> suck it in there if you prefer. wake_up_nested(), once 
> wake_up_nested_poll() goes in, has no more users and IMO can go.

Ahh. Even better. Make it so.

		Linus

Re: [patch 2/6] epoll keyed wakeups v2 - introduce new *_poll() wakeup macros

[Davide Libenzi]

On Tue, 3 Feb 2009, Andrew Morton wrote:

> On Sun, 01 Feb 2009 12:04:23 -0800 Davide Libenzi <davidel@xmailserver.org> wrote:
> 
> > +#define wake_up_nested_poll(x, m, s)					\
> > +do {									\
> > +	unsigned long flags;						\
> > +									\
> > +	spin_lock_irqsave_nested(&(x)->lock, flags, (s));		\
> > +	wake_up_locked_poll(x, m);					\
> > +	spin_unlock_irqrestore(&(x)->lock, flags);			\
> > +} while (0)
> 
> I had to go and find the callsite to work out the type of `x' :(
> 
> - this macro can be passed the address of any structure which has a
>   `spinlock_t lock;' in it, which seems strange.

That'd be a wait_queue_head_t*. Maybe better turn this into functions, I 
agree.


- Davide