Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755599Ab1CNJPh (ORCPT ); Mon, 14 Mar 2011 05:15:37 -0400 Received: from smtp-out.google.com ([74.125.121.67]:10679 "EHLO smtp-out.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755376Ab1CNJPd (ORCPT ); Mon, 14 Mar 2011 05:15:33 -0400 DomainKey-Signature: a=rsa-sha1; c=nofws; d=google.com; s=beta; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; b=c00NdR1TJtLWEYI7qV/F9WLeLwMCAI3M+ID5L8UkoyqZ3Pxk64/WFh7EI4mUm13wBq klMLBPsUqLdUdGrpFcdQ== MIME-Version: 1.0 In-Reply-To: References: <20110307021127.GB31188@google.com> <20110309112550.GA3050@google.com> <20110311021654.GA26122@google.com> <20110311024731.GB26122@google.com> Date: Mon, 14 Mar 2011 02:15:29 -0700 Message-ID: Subject: Re: [PATCH 1/3] futex: do not pagefault_disable in futex_atomic_cmpxchg_inatomic() From: Michel Lespinasse To: Linus Torvalds Cc: Thomas Gleixner , Darren Hart , Ingo Molnar , Peter Zijlstra , Matt Turner , Russell King , David Howells , Tony Luck , Michal Simek , Ralf Baechle , "James E.J. Bottomley" , Benjamin Herrenschmidt , Martin Schwidefsky , Paul Mundt , "David S. Miller" , Chris Metcalf , Andrew Morton , LKML Content-Type: text/plain; charset=ISO-8859-1 X-System-Of-Record: true Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2029 Lines: 45 On Sun, Mar 13, 2011 at 3:49 PM, Linus Torvalds wrote: > On Thu, Mar 10, 2011 at 6:47 PM, Michel Lespinasse 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. -- 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/