Re: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap

From: John Hubbard <jhubbard@nvidia.com>
To: Alistair Popple <apopple@nvidia.com>
Cc: Jason Gunthorpe <jgg@nvidia.com>, <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 <hch@lst.de>
Subject: Re: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap
Date: Tue, 30 Mar 2021 21:09:56 -0700	[thread overview]
Message-ID: <5f156670-e8d0-e62a-2286-764687a6669f@nvidia.com> (raw)
In-Reply-To: <b4b11c59-975d-26c7-043a-6acddff78dfd@nvidia.com>

On 3/30/21 8:56 PM, 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:
> 
> /**
> * 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.

Actually, re-reading your and Jason's earlier points in the thread, I see
that I'm *not* missing anything, and we are actually in agreement about how
the code operates. OK, good!

Also, as you point out above, maybe the "try_" prefix is not really accurate
either, given how this works. So maybe we have arrived at something like:

     try_to_munlock() --> page_mlock() // or mlock_page()...
     try_to_munlock_one() --> page_mlock_one()

thanks,
-- 
John Hubbard
NVIDIA