Re: [PATCH 1/3] futex: do not pagefault_disable in futex_atomic_cmpxchg_inatomic()

[Michel Lespinasse <walken@google.com>]

On Sun, Mar 13, 2011 at 3:49 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Mar 10, 2011 at 6:47 PM, Michel Lespinasse <walken@google.com> wrote:
>> kernel/futex.c disables page faults before calling
>> futex_atomic_cmpxchg_inatomic(), so there is no need to do it again
>> within that function.
>
> This seems totally bogus.
>
> Even the comment is crap.
>
> Sure, the callers may disable preemption, but that has NOTHING to do
> with "pagefault_disable()". Th epagefault_[en/dis]able functions will
> touch the preempt count EVEN IF PREEMPTION ISN'T EVEN ENABLED!

I understand pagefault_disable() and preempt_disable() are different
concepts, but their implementations have a lot in common...

> So what the f*ck does that "Note that preemption is disabled.." crap even mean?

I was thinking that if cmpxchg_futex_value_locked() already raised the
preempt count, it seemed redundant to do it again (and then decrement
it back) in a nested way within futex_atomic_cmpxchg_inatomic on arm.

> The thing is made even worse by the fact that as far as I can tell,
> the comment simply isn't true at all (even if you were to ignore the
> fundamental confusion about preemption vs the pagefault
> disable/enable). Not all callers of futex_atomic_cmpxchg_inatomic() do
> anything of the sort, whether it's preemptibility _or_ the proper
> pagefault_disable/enable(). Just look at the exit_robust_list() ->
> handle_futex_death(), for example.

You got me there - clearly I f*cked up. What really irks me is that I
have a retrospectively bogus memory of actually looking at that call
site and seeing a pagefault_disable there... But as is now obvious,
it's never been there :/

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.