Messages in this thread | | | Date | Sun, 13 Mar 2011 17:55:45 -0700 | Subject | Re: [PATCH 1/3] futex: do not pagefault_disable in futex_atomic_cmpxchg_inatomic() | From | Darren Hart <> |
| |
I have to apologize. These landed right as I was leaving for OSTS and I didn't have time to review them properly. I wondered about the preempt vs pagefault disable and wondered how exit_robust_list() was covered in Michel's comments (perhaps somewhere higher up the call chain). I leaned on the review of others when I should have raised the questions even if I didn't have the time to dig into them myself. Linus shouldn't have had to raise those questions, I'll do better at this in the future.
I noticed that my name is the only one in futex.c with an email address in the header - that email address is no longer valid, and it delayed the patches getting to my inbox - I will submit a patch to fix that. I should catch them sooner now, regardless, with improved LKML filters.
Thanks,
Darren Hart
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! > > 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. > > Linus >
-- Darren Hart -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |