Re: [PATCH] thp: reduce khugepaged freezing latency

[Tejun Heo <tj@kernel.org>]

Hello, Andrea.

On Wed, Nov 09, 2011 at 07:19:25PM +0100, Andrea Arcangeli wrote:
> On Wed, Nov 09, 2011 at 10:09:00AM -0800, Tejun Heo wrote:
> > I'm confused.  You're doing add_wait_queue() before
> > schedule_timeout_interruptible().  prepare_to_wait() is essentially
> > add_wait_queue() + set_current_state().  What am I missing?  ie. why
> > not do the following?
> 
> Ah the reason of the waitqueue is the sysfs store, to get out of there
> if somebody decreases the wait time from 1min to 10sec or
> similar. It's not really needed for other things, in theory it could
> be a separate waitqueue just for sysfs but probably not worth it.

Oh I see.

> I have no "event" to wait other than the wakeup itself, this in the
> end is the only reason it isn't already using
> wait_event_freezable_timeout. Of course I can pass "false" as the
> event.

I think, for this specific case, wait_event_freezable_timeout() w/
false is the simplest thing to do.

> > Hmmm... I don't know.  I really hope all freezable tasks stick to
> > higher level interface.  It's way too easy to get things wrong and eat
> > either freezing or actual wakeup condition.
> 
> Well you've just to tell me if I have to pass "false" and if
> add_wait_queue+schedule_timeout_interruptible is obsoleted. If it's
> not obsoleted the patch I posted should already be ok. It also will be
> useful if others need to wait for a long time (> the freezer max wait)
> without a waitqueue which I don't think is necessarily impossible. It
> wasn't the case here just because I need to promptly react to the
> sysfs writes (or setting the wait time to 1 day would then require 1
> day before sysfs new value becomes meaningful, well unless somebody
> doess killall khugepaged.. :)

I agree that there can be use cases where freezable interruptible
sleep is useful.  Thanks to the the inherently racy nature of
schedule_interruptible_timeout() w.r.t. non-persistent interruptible
wakeups (ie. everything other than signal), race conditions introduced
by try_to_freeze() should be okay

The biggest problem I have with schedule_timeout_freezable() is that
it doesn't advertise that it's racy - ie. it doesn't have sleep
condition in the function name.  Its wait counterpart
wait_event_freezable() isn't racy thanks to the explicit wait
condition and doesn't have such problem.

Maybe my concern is just paraonia and people wouldn't assume it's
schedule_timeout() with magic freezer support.  Or we can name it
schedule_timeout_interruptible_freezable() (urgh........).  I don't
know.  My instinct tells me to strongly recommend use of
wait_event_freezable_timeout() and run away.  :)

Thanks.

-- 
tejun