From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Gunthorpe Date: Tue, 30 Mar 2021 22:24:40 +0000 Subject: Re: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap Message-Id: <20210330222440.GC2356281@nvidia.com> List-Id: References: <20210326000805.2518-1-apopple@nvidia.com> <20210326000805.2518-4-apopple@nvidia.com> <20210330184903.GZ2356281@nvidia.com> <12442194.rtmf8Ope3M@nvdebian> In-Reply-To: <12442194.rtmf8Ope3M@nvdebian> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Alistair Popple Cc: 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, jhubbard@nvidia.com, rcampbell@nvidia.com, jglisse@redhat.com, hch@infradead.org, daniel@ffwll.ch, willy@infradead.org, Christoph Hellwig On Wed, Mar 31, 2021 at 09:09:30AM +1100, Alistair Popple wrote: > > > @@ -1796,8 +1821,7 @@ bool try_to_unmap(struct page *page, enum ttu_flags > flags) > > > void try_to_munlock(struct page *page) > > > { > > > > But this is also called try_to_munlock ?? > > As far as I can tell this has always been called try_to_munlock() even though > it appears to do the opposite. Maybe we should change it then? > > /** > > * 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. > > */ > > In other words it sets PG_mlocked if one or more vmas has it mlocked. So > try_to_mlock() might be a better name, except that seems to have the potential > for confusion as well because it's only called from the munlock code path and > never for mlock. That explanation makes more sense.. This function looks like it is 'set PG_mlocked of the page if any vm->flags has VM_LOCKED' Maybe call it check_vm_locked or something then and reword the above comment? (and why is it OK to read vm->flags for this without any locking?) > > Something needs attention here.. > > I think the code is correct, but perhaps the naming could be better. Would be > interested hearing any thoughts on renaming try_to_munlock() to try_to_mlock() > as the current name appears based on the context it is called from (munlock) > rather than what it does (mlock). The point of this patch is to make it clearer, after all, so I'd change something and maybe slightly clarify the comment. Jason