linux-kernel - Re: [PATCH] thp: reduce khugepaged freezing latency

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111109183447.GG1260@google.com>
Date:	Wed, 9 Nov 2011 10:34:47 -0800
From:	Tejun Heo <tj@...nel.org>
To:	Andrea Arcangeli <aarcange@...hat.com>
Cc:	Oleg Nesterov <oleg@...hat.com>,
	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>,
	"Rafael J. Wysocki" <rjw@...e.com>, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org, Jiri Slaby <jirislaby@...il.com>,
	linux-mm@...ck.org, Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] thp: reduce khugepaged freezing latency

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/