[patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[Davide Libenzi]

The following patch introduces new kwake_* macros that accepts an
extra key parameter to be specified in the wakeup.
I chose to add an initial 'k' to the original names, instead of adding
a whole "_key", since the name of some of those macros is becoming
awfully long. No problem in using the "_key" naming, if others feel it.
Comments?



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


- Davide


---
 include/linux/wait.h |   41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

Index: linux-2.6.mod/include/linux/wait.h
===================================================================
--- linux-2.6.mod.orig/include/linux/wait.h	2009-01-30 12:11:42.000000000 -0800
+++ linux-2.6.mod/include/linux/wait.h	2009-01-30 12:11:44.000000000 -0800
@@ -144,31 +144,48 @@ int out_of_line_wait_on_bit(void *, int,
 int out_of_line_wait_on_bit_lock(void *, int, int (*)(void *), unsigned);
 wait_queue_head_t *bit_waitqueue(void *, int);
 
-#define wake_up(x)			__wake_up(x, TASK_NORMAL, 1, NULL)
-#define wake_up_nr(x, nr)		__wake_up(x, TASK_NORMAL, nr, NULL)
-#define wake_up_all(x)			__wake_up(x, TASK_NORMAL, 0, NULL)
-#define wake_up_locked(x)		__wake_up_locked((x), TASK_NORMAL, NULL)
-
-#define wake_up_interruptible(x)	__wake_up(x, TASK_INTERRUPTIBLE, 1, NULL)
-#define wake_up_interruptible_nr(x, nr)	__wake_up(x, TASK_INTERRUPTIBLE, nr, NULL)
-#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, \
-						       NULL)
+#define kwake_up(x, k)			__wake_up(x, TASK_NORMAL, 1, (void *) (k))
+#define kwake_up_nr(x, nr, k)		__wake_up(x, TASK_NORMAL, nr, (void *) (k))
+#define kwake_up_all(x, k)		__wake_up(x, TASK_NORMAL, 0, (void *) (k))
+#define kwake_up_locked(x, k)		__wake_up_locked((x), TASK_NORMAL, \
+						(void *) (k))
+
+#define kwake_up_interruptible(x, k)	__wake_up(x, TASK_INTERRUPTIBLE, 1, \
+						(void *) (k))
+#define kwake_up_interruptible_nr(x, nr, k)	__wake_up(x, TASK_INTERRUPTIBLE, nr, \
+						(void *) (k))
+#define kwake_up_interruptible_all(x, k)	__wake_up(x, TASK_INTERRUPTIBLE, 0, \
+						(void *) (k))
+#define kwake_up_interruptible_sync(x, k)	__wake_up_sync((x), TASK_INTERRUPTIBLE, \
+						1, (void *) (k))
+
+#define wake_up(x)			kwake_up(x, NULL)
+#define wake_up_nr(x, nr)		kwake_up_nr(x, nr, NULL)
+#define wake_up_all(x)			kwake_up_all(x, NULL)
+#define wake_up_locked(x)		kwake_up_locked(x, NULL)
+
+#define wake_up_interruptible(x)	kwake_up_interruptible(x, NULL)
+#define wake_up_interruptible_nr(x, nr)	kwake_up_interruptible_nr(x, nr, NULL)
+#define wake_up_interruptible_all(x)	kwake_up_interruptible_all(x, NULL)
+#define wake_up_interruptible_sync(x)	kwake_up_interruptible_sync(x, NULL)
+
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 /*
  * macro to avoid include hell
  */
-#define wake_up_nested(x, s)						\
+#define kwake_up_nested(x, s, k)					\
 do {									\
 	unsigned long flags;						\
 									\
 	spin_lock_irqsave_nested(&(x)->lock, flags, (s));		\
-	wake_up_locked(x); 						\
+	kwake_up_locked(x, k); 						\
 	spin_unlock_irqrestore(&(x)->lock, flags);			\
 } while (0)
+#define wake_up_nested(x, s)		kwake_up_nested(x, s, NULL)
 #else
 #define wake_up_nested(x, s)		wake_up(x)
+#define kwake_up_nested(x, k, s)	kwake_up(x, k)
 #endif
 
 #define __wait_event(wq, condition) 					\

Re: [patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[Ingo Molnar]

* Davide Libenzi <davidel@xmailserver.org> wrote:

> +#define wake_up(x)			kwake_up(x, NULL)
> +#define wake_up_nr(x, nr)		kwake_up_nr(x, nr, NULL)
> +#define wake_up_all(x)			kwake_up_all(x, NULL)
> +#define wake_up_locked(x)		kwake_up_locked(x, NULL)
> +
> +#define wake_up_interruptible(x)	kwake_up_interruptible(x, NULL)
> +#define wake_up_interruptible_nr(x, nr)	kwake_up_interruptible_nr(x, nr, NULL)
> +#define wake_up_interruptible_all(x)	kwake_up_interruptible_all(x, NULL)
> +#define wake_up_interruptible_sync(x)	kwake_up_interruptible_sync(x, NULL)

i like the patchset - nice work!

One minor worry i have: these wakeup calls are _very_ common in the 
kernel, and this patch adds an extra parameter to it that is unused (NULL) 
in 99% of the cases.

Would be nice to see the kernel image size increase due to this change 
(which gives a good measure about how much of an issue this is).

If it's of any worrying level, it might make sense to keep the original 
functions untouched, and introduce a second entry point that has one more 
parameter. Ok?

	Ingo

Re: [patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[Davide Libenzi]

On Sat, 31 Jan 2009, Ingo Molnar wrote:

> 
> * Davide Libenzi <davidel@xmailserver.org> wrote:
> 
> > +#define wake_up(x)			kwake_up(x, NULL)
> > +#define wake_up_nr(x, nr)		kwake_up_nr(x, nr, NULL)
> > +#define wake_up_all(x)			kwake_up_all(x, NULL)
> > +#define wake_up_locked(x)		kwake_up_locked(x, NULL)
> > +
> > +#define wake_up_interruptible(x)	kwake_up_interruptible(x, NULL)
> > +#define wake_up_interruptible_nr(x, nr)	kwake_up_interruptible_nr(x, nr, NULL)
> > +#define wake_up_interruptible_all(x)	kwake_up_interruptible_all(x, NULL)
> > +#define wake_up_interruptible_sync(x)	kwake_up_interruptible_sync(x, NULL)
> 
> i like the patchset - nice work!
> 
> One minor worry i have: these wakeup calls are _very_ common in the 
> kernel, and this patch adds an extra parameter to it that is unused (NULL) 
> in 99% of the cases.
> 
> Would be nice to see the kernel image size increase due to this change 
> (which gives a good measure about how much of an issue this is).
> 
> If it's of any worrying level, it might make sense to keep the original 
> functions untouched, and introduce a second entry point that has one more 
> parameter. Ok?

Fine by me. Any preference for names?



- Davide

Re: [patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[Linus Torvalds]

On Sat, 31 Jan 2009, Ingo Molnar wrote:
> 
> Would be nice to see the kernel image size increase due to this change 
> (which gives a good measure about how much of an issue this is).

Ingo, I don't think you have looked at that header file for a while.

It's already doing that, Davide just changed the names a bit:

	#define wake_up_interruptible(x)        __wake_up(x, TASK_INTERRUPTIBLE, 1, NULL)

and the extra parameter is already there in the caller.

(Yeah, Davide did add it to __wake_up_locked and __wake_up_sync, but those 
are really not the common cases).

Sure, we can change those #define's to be actual functions (and perhaps 
not export the low-level __wake_up() functions at all), since it's true 
that it would probably shrink the kernel size, but that is really a 
totally independent issue from the whole epoll wakeups thing.

		Linus

Re: [patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Sat, 31 Jan 2009, Ingo Molnar wrote:
> > 
> > Would be nice to see the kernel image size increase due to this change 
> > (which gives a good measure about how much of an issue this is).
> 
> Ingo, I don't think you have looked at that header file for a while.
> 
> It's already doing that, Davide just changed the names a bit:
> 
> 	#define wake_up_interruptible(x)        __wake_up(x, TASK_INTERRUPTIBLE, 1, NULL)
> 
> and the extra parameter is already there in the caller.

Yeah, indeed - i should have noticed the absense of new function 
prototypes in the patch ... and in any case i shouldnt post at 4am ;)

> (Yeah, Davide did add it to __wake_up_locked and __wake_up_sync, but 
> those are really not the common cases).
> 
> Sure, we can change those #define's to be actual functions (and perhaps 
> not export the low-level __wake_up() functions at all), since it's true 
> that it would probably shrink the kernel size, but that is really a 
> totally independent issue from the whole epoll wakeups thing.

Yeah. Will have a look at that independently of Davide's patch.

	Ingo

Re: [patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[Davide Libenzi]

On Sat, 31 Jan 2009, Ingo Molnar wrote:

> 
> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> 
> > On Sat, 31 Jan 2009, Ingo Molnar wrote:
> > > 
> > > Would be nice to see the kernel image size increase due to this change 
> > > (which gives a good measure about how much of an issue this is).
> > 
> > Ingo, I don't think you have looked at that header file for a while.
> > 
> > It's already doing that, Davide just changed the names a bit:
> > 
> > 	#define wake_up_interruptible(x)        __wake_up(x, TASK_INTERRUPTIBLE, 1, NULL)
> > 
> > and the extra parameter is already there in the caller.
> 
> Yeah, indeed - i should have noticed the absense of new function 
> prototypes in the patch ... and in any case i shouldnt post at 4am ;)
> 
> > (Yeah, Davide did add it to __wake_up_locked and __wake_up_sync, but 
> > those are really not the common cases).
> > 
> > Sure, we can change those #define's to be actual functions (and perhaps 
> > not export the low-level __wake_up() functions at all), since it's true 
> > that it would probably shrink the kernel size, but that is really a 
> > totally independent issue from the whole epoll wakeups thing.
> 
> Yeah. Will have a look at that independently of Davide's patch.

The ones with an extra parameter are "sync" and "locked". The 
simple __wake_up() ones, already had the parameter.
So your idea would be to have __wake_up_locked_XXX and __wake_up_sync_XXX 
instead of adding an extra parameter, no?
If yes, what about XXX? "key"?



- Davide

Re: [patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[Alan Cox]

> One minor worry i have: these wakeup calls are _very_ common in the 
> kernel, and this patch adds an extra parameter to it that is unused (NULL) 
> in 99% of the cases.

And in most of the cases it is used is constant for the waitqueue anyway
or appears to be (eg the tty patch). I don't think we should be stacking
extra parameters on all those zillions of calls made all over the kernel
and for no purpose - so the existing wakeup functions should stay as is
for performance as well as for size (which due to cache pressure is
performance).

Alan

Re: [patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[Davide Libenzi]

On Sat, 31 Jan 2009, Alan Cox wrote:

> > One minor worry i have: these wakeup calls are _very_ common in the 
> > kernel, and this patch adds an extra parameter to it that is unused (NULL) 
> > in 99% of the cases.
> 
> And in most of the cases it is used is constant for the waitqueue anyway
> or appears to be (eg the tty patch). I don't think we should be stacking
> extra parameters on all those zillions of calls made all over the kernel
> and for no purpose - so the existing wakeup functions should stay as is
> for performance as well as for size (which due to cache pressure is
> performance).

Besides for that, did the TTY patch look sane to you? Did I get all the 
wakeup points? TTY code always gave me the willies :)


- Davide

Re: [patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[Linus Torvalds]

On Fri, 30 Jan 2009, Davide Libenzi wrote:
>
> The following patch introduces new kwake_* macros that accepts an
> extra key parameter to be specified in the wakeup.

I really hate the naming.

> I chose to add an initial 'k' to the original names, instead of adding
> a whole "_key", since the name of some of those macros is becoming
> awfully long. No problem in using the "_key" naming, if others feel it.
> Comments?

That whole "kwake" thing makes me just think mis-spelling, so it does need 
to change. 

But even more I dislike the notion of this being a "key". It's not. It's 
about poll events, nothing more. So renaming it to "_key()" in no way 
helps.

Yes, _internally_ we send that "void *key" around, and then leave it to 
lower levels to agree about how it is used, but at the level _you_ then 
use it, that is no longer the case. When you do a 

	kwake_up_interruptible(&tty->write_wait, POLLOUT);

that has _nothing_ to do with "keys" any more. So the 'k' prefix is wrong 
and really odd-looking, but a '_key' postfix wouldn't be much better 
either. Because when you pass in POLLOUT, you're not using it as a key, 
you are very much using it as a poll-specific thing.

So the naming should match that. I suspect a '_poll' postfix (or a 'poll_' 
prefix would work and make sense.

So apart from that hating, I think the internal implementation and the use 
of the existing 'key' parameter is fairly sane. The only downside is that 
we've now really used up that key thing for something very epoll-specific, 
but I don't see any better use for it, so I guess that's not a big 
downside.

Oh, and numbers, please. How big of a win is this, really? Preferably with 
something that really uses epoll for something real.

		Linus

Re: [patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[Davide Libenzi]

On Fri, 30 Jan 2009, Linus Torvalds wrote:

> > I chose to add an initial 'k' to the original names, instead of adding
> > a whole "_key", since the name of some of those macros is becoming
> > awfully long. No problem in using the "_key" naming, if others feel it.
> > Comments?
> 
> That whole "kwake" thing makes me just think mis-spelling, so it does need 
> to change. 
> 
> But even more I dislike the notion of this being a "key". It's not. It's 
> about poll events, nothing more. So renaming it to "_key()" in no way 
> helps.
> 
> Yes, _internally_ we send that "void *key" around, and then leave it to 
> lower levels to agree about how it is used, but at the level _you_ then 
> use it, that is no longer the case. When you do a 
> 
> 	kwake_up_interruptible(&tty->write_wait, POLLOUT);
> 
> that has _nothing_ to do with "keys" any more. So the 'k' prefix is wrong 
> and really odd-looking, but a '_key' postfix wouldn't be much better 
> either. Because when you pass in POLLOUT, you're not using it as a key, 
> you are very much using it as a poll-specific thing.
> 
> So the naming should match that. I suspect a '_poll' postfix (or a 'poll_' 
> prefix would work and make sense.

As I said in the patch, I'm really ok with any naming. Adding another 
pre/suf-fix seemed to render names a little on the long side, hence the 
'k' choice.




> So apart from that hating, I think the internal implementation and the use 
> of the existing 'key' parameter is fairly sane. The only downside is that 
> we've now really used up that key thing for something very epoll-specific, 
> but I don't see any better use for it, so I guess that's not a big 
> downside.
> 
> Oh, and numbers, please. How big of a win is this, really? Preferably with 
> something that really uses epoll for something real.

Will try to see if I can come up with something, although is not really 
easy to simulate what can happen in a real life scenario.
What happens today is that if you're waiting for POLLOUT, say in a socket, 
a new incoming packet will trigger a wakeup, epoll has to check the 
ready-list, call f_op->poll, verify that the event doesn't match the 
interest, and go back to sleep. Same if it was waiting for POLLIN, and 
write-space was triggered.



- Davide

Re: [patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[wli]

On Fri, Jan 30, 2009 at 07:49:26PM -0800, Linus Torvalds wrote:
> But even more I dislike the notion of this being a "key". It's not. It's 
> about poll events, nothing more. So renaming it to "_key()" in no way 
> helps.
> Yes, _internally_ we send that "void *key" around, and then leave it to 
> lower levels to agree about how it is used, but at the level _you_ then 
> use it, that is no longer the case. When you do a 
> 	kwake_up_interruptible(&tty->write_wait, POLLOUT);
> that has _nothing_ to do with "keys" any more. So the 'k' prefix is wrong 
> and really odd-looking, but a '_key' postfix wouldn't be much better 
> either. Because when you pass in POLLOUT, you're not using it as a key, 
> you are very much using it as a poll-specific thing.
> So the naming should match that. I suspect a '_poll' postfix (or a 'poll_' 
> prefix would work and make sense.

The filtered wakeup code uses some notion of a key already. There, the
key is to route wakeups to recipients associated with the correct
object. Here, the objects are known in advance. The wakeup is to give
them some particular message in which they may not necessarily be
interested. Names indicative of that (e.g. *_msg/msg_*) would clarify
the distinction between it and the filtered wakeup code used for pages.

-- wli

Re: [patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[Davide Libenzi]

On Fri, 30 Jan 2009, wli@movementarian.org wrote:

> On Fri, Jan 30, 2009 at 07:49:26PM -0800, Linus Torvalds wrote:
> > But even more I dislike the notion of this being a "key". It's not. It's 
> > about poll events, nothing more. So renaming it to "_key()" in no way 
> > helps.
> > Yes, _internally_ we send that "void *key" around, and then leave it to 
> > lower levels to agree about how it is used, but at the level _you_ then 
> > use it, that is no longer the case. When you do a 
> > 	kwake_up_interruptible(&tty->write_wait, POLLOUT);
> > that has _nothing_ to do with "keys" any more. So the 'k' prefix is wrong 
> > and really odd-looking, but a '_key' postfix wouldn't be much better 
> > either. Because when you pass in POLLOUT, you're not using it as a key, 
> > you are very much using it as a poll-specific thing.
> > So the naming should match that. I suspect a '_poll' postfix (or a 'poll_' 
> > prefix would work and make sense.
> 
> The filtered wakeup code uses some notion of a key already. There, the
> key is to route wakeups to recipients associated with the correct
> object. Here, the objects are known in advance. The wakeup is to give
> them some particular message in which they may not necessarily be
> interested. Names indicative of that (e.g. *_msg/msg_*) would clarify
> the distinction between it and the filtered wakeup code used for pages.

I had thought of giving the void* some structure, besides being a cast 
from an event mask, so that later on we'd be able to eventually embed 
more information in the wakeup. Dunno if worth it.



- Davide

Re: [patch 3/7] epoll keyed wakeups - introduce key-aware wakeup macros

[wli]

On Fri, 30 Jan 2009, wli@movementarian.org wrote:
>> The filtered wakeup code uses some notion of a key already. There, the
>> key is to route wakeups to recipients associated with the correct
>> object. Here, the objects are known in advance. The wakeup is to give
>> them some particular message in which they may not necessarily be
>> interested. Names indicative of that (e.g. *_msg/msg_*) would clarify
>> the distinction between it and the filtered wakeup code used for pages.

On Sat, Jan 31, 2009 at 11:08:40AM -0800, Davide Libenzi wrote:
> I had thought of giving the void* some structure, besides being a cast 
> from an event mask, so that later on we'd be able to eventually embed 
> more information in the wakeup. Dunno if worth it.

kernel/wait.c uses struct wait_bit_key; void * was merely for keeping it
as private as possible, though it is exposed for the sake of macros like
DEFINE_WAIT_BIT et al. It would make little difference to the pagecache
waitqueue code if the structure were augmented, the function prototypes
changed to reflect the argument type, etc. There are a priori guarantees
based on the objects that the lists of tasks considered won't overlap,
so unrelated structures are even possible.


-- wli