Re: [PATCH v3] mm: memory: fix /proc/meminfo reporting for MLOCK_ONFAULT

[Lucian Grijincu]

ï> On 9/16/19, 04:35, "Michal Hocko" <mhocko@xxxxxxxxxx> wrote:
>     > diff --git a/mm/memory.c b/mm/memory.c
>     > index e0c232fe81d9..55da24f33bc4 100644
>     > --- a/mm/memory.c
>     > +++ b/mm/memory.c
>     > @@ -3311,6 +3311,8 @@ vm_fault_t alloc_set_pte(struct vm_fault *vmf, struct mem_cgroup *memcg,
>     >  	} else {
>     >  		inc_mm_counter_fast(vma->vm_mm, mm_counter_file(page));
>     >  		page_add_file_rmap(page, false);
>     > +		if (vma->vm_flags & VM_LOCKED && !PageTransCompound(page))
>     > +			mlock_vma_page(page);
>     >  	}
>     >  	set_pte_at(vma->vm_mm, vmf->address, vmf->pte, entry);
    
>     I dunno. Handling it here in alloc_set_pte sounds a bit weird to me.
>     Altough we already do mlock for CoW pages there, I thought this was more
>     of an exception.
>     Is there any real reason why this cannot be done in the standard #PF
>     path? finish_fault for example?

alloc_set_pte is called from finish_fault https://github.com/torvalds/linux/blob/v5.2/mm/memory.c#L3400

   vm_fault_t finish_fault(struct vm_fault *vmf)
     ...
	if (!ret)
		ret = alloc_set_pte(vmf, vmf->memcg, page);

and inside alloc_set_pte one of the branches of the if-clause already handled mlocked pages:
https://github.com/torvalds/linux/blob/v5.2/mm/memory.c#L3348-L3356

I added it to the else-branch as that seemed like the least intrusive change, but I will move this to finish_fault, probably like this (after I'm done testing):

   vm_fault_t finish_fault(struct vm_fault *vmf)
    ...
        if (!ret)
                ret = alloc_set_pte(vmf, vmf->memcg, page);
+       if (!ret && (vmf->vma->vm_flags & VM_LOCKED) && !PageTransCompound(page))
+                       mlock_vma_page(page);

Thanks for the review and suggestions!

--
Lucian