Messages in this thread | | | Date | Mon, 14 Mar 2011 10:13:30 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [PATCH 1/3] futex: do not pagefault_disable in futex_atomic_cmpxchg_inatomic() |
| |
On Sun, 13 Mar 2011, Linus Torvalds 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! > > So what the f*ck does that "Note that preemption is disabled.." crap even mean? > > 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. > > This kind of patch is the kind that personally makes me want to put > you on a spam-list. Misleading commit messages with bogus and > fundamentally incorrect added comments in the code. WTF? > > Did I miss some patch that changed that, or is this really as horribly > bad as I think it is? I see it already made it into -tip.
That's my fault.
I really checked the call sites of futex_atomic_cmpxchg_inatomic() and totally failed to see the one in handle_futex_death() which does not use the helper function cmpxchg_futex_value_locked(). That helper function is safe and does the right thing:
pagefault_disable(); curval = futex_atomic_cmpxchg_inatomic(uaddr, uval, newval); pagefault_enable();
So, that means we have all call sites covered except one, which needs to be fixed _AND_ also pushed into stable as all arch implementations except ARM rely on the caller doing the pagefault_disable().
And I missed the bogus comment as well. Sigh.
Thanks,
tglx
| |