Messages in this thread | | | Date | Mon, 14 Mar 2011 02:15:29 -0700 | Subject | Re: [PATCH 1/3] futex: do not pagefault_disable in futex_atomic_cmpxchg_inatomic() | From | Michel Lespinasse <> |
| |
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.
| |