Messages in this thread | | | Date | Tue, 30 Mar 2021 19:24:40 -0300 | From | Jason Gunthorpe <> | Subject | Re: [PATCH v7 3/8] mm/rmap: Split try_to_munlock from try_to_unmap |
| |
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
| |