Re: Linux 3.19-rc3

[Kent Overstreet]

On Tue, Jan 06, 2015 at 12:42:15PM +0100, Peter Zijlstra wrote:
> On Tue, Jan 06, 2015 at 03:07:30AM -0800, Kent Overstreet wrote:
> > > No, the root cause is nesting sleep primitives, this is not fixable in
> > > the one place, both prepare_to_wait and mutex_lock are using
> > > task_struct::state, they have to, no way around it.
> > 
> > No, it's completely possible to construct a prepare_to_wait() that doesn't
> > require messing with the task state. Had it for years.
> > 
> > http://evilpiepirate.org/git/linux-bcache.git/log/?h=aio_ring_fix
> 
> Your closures are cute but not the same. And sure you can do a wait
> queue like interface -- my wait_woken thing is an example -- that
> doesn't require task state.
> 
> The point remains that you then have to fix every instance to conform to
> the new interface.

Possibly I missed your point because I've been overly crotchety lately.

I do want to make the point that it's not really the callers that are broken;
especially those that are using prepare_to_wait() via wait_event(). Using
wait_event() is exactly what people _should_ be doing, instead of open coding
this stuff and/or coming up with hacks to work around the fact that
prepare_to_wait() is implemented via messing with the task state.

This is a sore point for me because I've seen other experienced, _skilled_
programmers screw this kind of code up too many times and I hate debugging lost
wakeups. I can write this kind of code in my sleep because I've spent too much
of time time implementing these kinds of primitives and I'm sure you can too,
but most people can't.

Anyways, my point is either wait_event() should be fixed to not muck with the
task state, or since that's not really practical we should at least provide a
standard drop in replacement that doesn't.

And the drop in replacement more or less exists, closure_wait_event() has the
same semantics as wait_event, similarly with the lower level primitives I just
listed the conversions for.

I don't even really care if it's closures, we could make something new that uses
the existing wait_queue_head_t but IMO it's gonna look a lot like closures with
an embedded refcount and closure_sync() instead of schedule().
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/