From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41826 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725875AbgIJV7Y (ORCPT ); Thu, 10 Sep 2020 17:59:24 -0400 Received: from mail-qk1-x741.google.com (mail-qk1-x741.google.com [IPv6:2607:f8b0:4864:20::741]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0D681C061756 for ; Thu, 10 Sep 2020 14:59:24 -0700 (PDT) Received: by mail-qk1-x741.google.com with SMTP id n133so7708814qkn.11 for ; Thu, 10 Sep 2020 14:59:24 -0700 (PDT) Date: Thu, 10 Sep 2020 18:59:21 -0300 From: Jason Gunthorpe Subject: Re: [RFC PATCH v2 1/3] mm/gup: fix gup_fast with dynamic page table folding Message-ID: <20200910215921.GP87483@ziepe.ca> References: <20200909142904.00b72921@thinkpad> <20200909192534.442f8984@thinkpad> <20200909180324.GI87483@ziepe.ca> <20200910093925.GB29166@oc3871087118.ibm.com> <20200910181319.GO87483@ziepe.ca> <20200910211010.46d064a7@thinkpad> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-s390-owner@vger.kernel.org List-ID: To: Linus Torvalds Cc: Gerald Schaefer , Alexander Gordeev , Dave Hansen , John Hubbard , LKML , linux-mm , linux-arch , Andrew Morton , Russell King , Mike Rapoport , Catalin Marinas , Will Deacon , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , Jeff Dike , Richard Weinberger , Dave Hansen , Andy Lutomirski , Peter Zijlstra , Thomas Gleixner , Ingo Molnar , Borislav Petkov , Arnd Bergmann , Andrey Ryabinin , linux-x86 , linux-arm , linux-power , linux-sparc , linux-um , linux-s390 , Vasily Gorbik , Heiko Carstens , Christian Borntraeger , Claudio Imbrenda On Thu, Sep 10, 2020 at 12:32:05PM -0700, Linus Torvalds wrote: > Yeah, I get hung up on naming sometimes. I don't tend to care much > about private local variables ("i" is a perfectly fine variable name), > but these kinds of somewhat subtle cross-architecture definitions I > feel matter. One of the first replys to this patch was to ask "when would I use _orig vs normal", so you are not alone. The name should convey it.. So, I suggest pXX_offset_unlocked() Since it is safe to call without the page table lock, while pXX_offset() requires the page table lock to be held as the internal *pXX is a data race otherwise. Patch 1 might be OK for a stable backport, but to get to a clear pXX_offset_unlocked() all the arches would want to be changed to implement that API and the generic code would provide the wrapper: #define pXX_offset(pXXp, address) pXX_offset_unlocked(pXXp, *(pXXp), address) Arches would not have a *pXX inside their code. Then we can talk about auditing call sites of pXX_offset and think about using the _unlocked version in places where the page table lock is not held. For instance mm/pagewalk.c should be changed. So should huge_pte_offset() and probably other places. These places might already be exsting data-race bugs. It is code-as-documentation indicating an unlocked page table walk. Now it is not just a S390 story but a change that makes the data concurrency much clearer, so I think I prefer this version to the addr_end one too. Regards, Jason