From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alistair Popple Date: Wed, 31 Mar 2021 04:15:47 +0000 Subject: Re: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap Message-Id: <23784464.epyy5R1Yul@nvdebian> List-Id: References: <20210326000805.2518-1-apopple@nvidia.com> <12636584.zsJ0Sx4KLp@nvdebian> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: John Hubbard Cc: Jason Gunthorpe , linux-mm@kvack.org, nouveau@lists.freedesktop.org, bskeggs@redhat.com, akpm@linux-foundation.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, kvm-ppc@vger.kernel.org, dri-devel@lists.freedesktop.org, rcampbell@nvidia.com, jglisse@redhat.com, hch@infradead.org, daniel@ffwll.ch, willy@infradead.org, Christoph Hellwig On Wednesday, 31 March 2021 2:56:38 PM AEDT John Hubbard wrote: > On 3/30/21 3:56 PM, Alistair Popple wrote: > ... > >> +1 for renaming "munlock*" items to "mlock*", where applicable. good grief. > > > > At least the situation was weird enough to prompt further investigation :) > > > > Renaming to mlock* doesn't feel like the right solution to me either though. I > > am not sure if you saw me responding to myself earlier but I am thinking > > renaming try_to_munlock() -> page_mlocked() and try_to_munlock_one() -> > > page_mlock_one() might be better. Thoughts? > > > > Quite confused by this naming idea. Because: try_to_munlock() returns > void, so a boolean-style name such as "page_mlocked()" is already not a > good fit. > > Even more important, though, is that try_to_munlock() is mlock-ing the > page, right? Is there some subtle point I'm missing? It really is doing > an mlock to the best of my knowledge here. Although the kerneldoc > comment for try_to_munlock() seems questionable too: It's mlocking the page if it turns out it still needs to be locked after unlocking it. But I don't think you're missing anything. > /** > * try_to_munlock - try to munlock a page > * @page: the page to be munlocked > * > * Called from munlock code. Checks all of the VMAs mapping the page > * to make sure nobody else has this page mlocked. The page will be > * returned with PG_mlocked cleared if no other vmas have it mlocked. > */ > > ...because I don't see where, in *this* routine, it clears PG_mlocked! > > Obviously we agree that a routine should be named based on what it does, > rather than on who calls it. So I think that still leads to: > > try_to_munlock() --> try_to_mlock() > try_to_munlock_one() --> try_to_mlock_one() > > Sorry if I'm missing something really obvious. Nope, I confused things somewhat by blindly quoting the documentation whilst forgetting that try_to_munlock() returns void rather than a bool. > > This is actually inspired from a suggestion in Documentation/vm/ unevictable- > > lru.rst which warns about this problem: > > > > try_to_munlock() Reverse Map Scan > > --------------------------------- > > > > .. warning:: > > [!] TODO/FIXME: a better name might be page_mlocked() - analogous to the > > page_referenced() reverse map walker. > > > > This is actually rather bad advice! page_referenced() returns an > int-that-is-really-a-boolean, whereas try_to_munlock(), at least as it > stands now, returns void. Usually when I'm writing a TODO item, I'm in a > hurry, and I think that's what probably happened here, too. :) So I think we're in agreement. The naming is bad and the advice in the documentation is also questionable :-) Thanks for the thoughts, I will re-send this with naming and documentation updates. > >> Although, it seems reasonable to tack such renaming patches onto the tail > > end > >> of this series. But whatever works. > > > > Unless anyone objects strongly I will roll the rename into this patch as there > > is only one caller of try_to_munlock. > > > > - Alistair > > > > No objections here. :) > > thanks, >