Re: [PATCH v1 1/2] mm/memory: cleanly support zeropage in vm_insert_page*(), vm_map_pages*() and vmf_insert_mixed()

[David Hildenbrand]

On 21.05.24 11:06, Vincent Donnefort wrote:
> On Tue, May 21, 2024 at 10:25:43AM +0200, David Hildenbrand wrote:
> > On 17.05.24 17:07, Vincent Donnefort wrote:
> > > Hi David,
> > >
> > > [...]
> > >
> > > > -static int validate_page_before_insert(struct page *page)
> > > > +static bool vm_mixed_zeropage_allowed(struct vm_area_struct *vma)
> > > > +{
> > > > +	VM_WARN_ON_ONCE(vma->vm_flags & VM_PFNMAP);
> > > > +	/*
> > > > +	 * Whoever wants to forbid the zeropage after some zeropages
> > > > +	 * might already have been mapped has to scan the page tables and
> > > > +	 * bail out on any zeropages. Zeropages in COW mappings can
> > > > +	 * be unshared using FAULT_FLAG_UNSHARE faults.
> > > > +	 */
> > > > +	if (mm_forbids_zeropage(vma->vm_mm))
> > > > +		return false;
> > > > +	/* zeropages in COW mappings are common and unproblematic. */
> > > > +	if (is_cow_mapping(vma->vm_flags))
> > > > +		return true;
> > > > +	/* Mappings that do not allow for writable PTEs are unproblematic. */
> > > > +	if (!(vma->vm_flags & (VM_WRITE | VM_MAYWRITE)))
> > > > +		return false;
> > >
> > > Shouldn't we return true here?
> >
> > Indeed, thanks! I wish we would have user in the tree already that could
> > exercise that code path.
>
> I have a patch ready to use this path from the memory map tracing! I can either
> send it once this one is picked-up or you can add it to your series?

Whatever works for you! To debug, it would be good if you could send me 
the patch and simple instructions on how to test it (do we have a 
selftest as well?).

> > [...]
> >
> > > > @@ -2043,7 +2085,7 @@ static int insert_page_in_batch_locked(struct vm_area_struct *vma, pte_t *pte,
> > > >    	if (!page_count(page))
> > > >    		return -EINVAL;
> > >
> > > This test here prevents inserting the zero-page.
> >
> > You mean the existing page_count() check? or the (wrong) vma->vm_flags check
> > in vm_mixed_zeropage_allowed() ?
>
> I meant this page_count() here. As a quick test, I removed that check (also fixed
> the vm_flags above) and the zero-page was properly mapped!

That's weird and might indicate another issue.

The refcount of the shared zeropage should be initialized to 1, just 
like for any other reserved pages 
(mm/mm_init.c:__init_single_page()->init_page_count())

Hm ...

--
Cheers,

David / dhildenb