[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20100106111830.9E34.A69D9226@jp.fujitsu.com>
Date: Wed, 6 Jan 2010 11:27:08 +0900 (JST)
From: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To: Darren Hart <dvhltc@...ibm.com>
Cc: kosaki.motohiro@...fujitsu.com,
Hugh Dickins <hugh.dickins@...cali.co.uk>,
Peter Zijlstra <peterz@...radead.org>,
KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
Nick Piggin <npiggin@...e.de>, Ingo Molnar <mingo@...e.hu>,
LKML <linux-kernel@...r.kernel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Ulrich Drepper <drepper@...il.com>,
"Hansen, Dave" <haveblue@...ibm.com>
Subject: Re: [PATCH v2] futex: remove rw parameter from get_futex_key()
> Hugh Dickins wrote:
> > On Tue, 5 Jan 2010, KOSAKI Motohiro wrote:
>
> >> From c3e2dfdff84b9b720e646fd6dd3c38fff293c7e6 Mon Sep 17 00:00:00 2001
> >> From: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
> >> Date: Tue, 5 Jan 2010 11:33:00 +0900
> >> Subject: [PATCH] futex: remove rw parameter from get_futex_key()
> >>
> >> Currently, futex have two problem.
> >>
> >> A) current futex doesn't handle private file mappings properly.
> >>
> >> get_futex_key() use PageAnon() to distinguish file and anon. it can
> >> makes following bad scenario.
> >>
> >> 1) thread-A call futex(private-mapping, FUTEX_WAIT). it makes to
> >> sleep on file mapping object.
> >> 2) thread-B write a variable and it makes cow.
> >> 3) thread-B call futex(private-mapping, FUTEX_WAKE). it wake up
> >> sleeped thread on the anonymous page. (but it's nothing)
> >>
>
> Excellent test case, thank you! Would you consider preparing a patch to
> futextest?
>
> http://git.kernel.org/?p=linux/kernel/git/dvhart/futextest.git;a=summary
Patch attached. you can feel free any modify such file. thanks.
> I did some experimentation here and found that:
>
> o The test works if the *_PRIVATE op codes are used.
> This is because the futex keys are generated using only the virtual
> address of the page, which doesn't change on a COW.
> o If the waiter writes to the val first, it works.
> This forces the COW before the waiter generates it's futex key.
True.
> So the waiter's key is generated based on the page cache page address
> for shared futexes when the value hasn't been written to prior to wait.
>
> The only scenario where I could think of wanting this behavior is if
> another process were to try and wake the waiter via the same file backed
> page. However, as I understand it, the re-use of the same page for
> unwritten-to private pages is an optimization and can't be relied upon.
> So this scenario is out. Another would be to use the futex as a very
> simple wait queue where the value is never changed. In this case
> however, the implementation is racy as the value check is effectively
> negated, so this use case is also out.
>
> As such, I see no reason not to always use VERIFY_WRITE and force a COW
> prior to generating the futex_key for shared futexes. It is not
> necessary for private futexes however as they use only the virtual address.
>
> I am not sure on whether or not it makes sense to avoid the VERIFY_WRITE
> on the private futexes. Could be it is just more code for negligible
> benefit. Thoughts?
I think it's no problem. because,
as performance view:
access_ok() of almost arch (included x86) ignore VERIFY_WRITE.
then, this change doesn't cause performance loss of course.
(current futex mainly handle ro-mapping problem by fault_in_user_writeable)
as consistency view:
This patch have better consistency without FUTEX_PRIVATE_FLAG case.
as usability view:
Nobody want to use ro-mappings for private futex. it's obviously meaningless
and useless.
Download attachment "0001-futextest-Add-two-testcase.patch" of type "application/octet-stream" (7607 bytes)
Powered by blists - more mailing lists