Re: [PATCH v6 05/13] mm/page_vma_mapped: Add flag to page_vma_mapped_walk::flags to track device private pages

["David Hildenbrand (Arm)" <david@kernel.org>]

>>>  #define DEFINE_FOLIO_VMA_WALK(name, _folio, _vma, _address, _flags)	\
>>>  	struct page_vma_mapped_walk name = {				\
>>> -		.pfn = folio_pfn(_folio),				\
>>> +		.pfn = folio_page_vma_walk_pfn(_folio),			\
>>>  		.nr_pages = folio_nr_pages(_folio),			\
>>>  		.pgoff = folio_pgoff(_folio),				\
>>>  		.vma = _vma,						\
>>>  		.address = _address,					\
>>> -		.flags = _flags,					\
>>> +		.flags = page_vma_walk_flags(_folio, _flags),		\
>>>  	}
>>
>> That's all rather horrible ...
>>
>>
>> I was asking myself recently, why something that is called
>> "page_vma_mapped_walk" consume a pfn. It's just a horrible interface.
> 
> I don't disagree, and in fact it used to consume a page until 2aff7a4755be ("mm:
> Convert page_vma_mapped_walk to work on PFNs"). If this was a page it would
> basically resolve all the hackiness of this patch because we would no longer
> have to pass PFN context around. So I wonder if there would be any opposition to
> changing this back to taking a page?

Are there cases where we pass a struct-page-less PFN?

The offenders are pfn_mkclean_range() and mapping_wrprotect_range().

pfn_mkclean_range() is only used by fs/dax.c

At least the caller immediately does a

	dax_flush(dax_dev, page_address(pfn_to_page(pfn)), count * PAGE_SIZE);

Afterwards where, apparently, a page for the PFN exists? :)


mapping_wrprotect_range() is only used by drivers/video/fbdev/core/fb_defio.c

fb_deferred_io_work() just calls page_to_pfn(page). Huh?

Best to explore the history what's going on there.


My best guess is that they don't want to modify page state. But that can
be expressed by the helper functions and doesn't require us to have such
a weird implementation.

> 
>>
>>
>> * DEFINE_FOLIO_VMA_WALK() users obviously receive a folio.
>> * mm/migrate_device.c just abuses page_vma_mapped_walk() to make
>>   set_pmd_migration_entry() work. But we have a folio.
>> * page_mapped_in_vma() has a page/folio.
>>
>> mapping_wrprotect_range_one() and pfn_mkclean_range() are the real
>> issues. They all end up calling page_vma_mkclean_one(), which does not
>> operate on pages/folios.
>>
>> Ideally, the odd pfn case would use it's own simplified infrastructure.
>>
>>
>> So, could we simply add a folio+page pointer in case we have one, and
>> use that one if set, leaving leaving the pfn unset?
>>
>> Then, the pfn would only be set for the
>> mapping_wrprotect_range_one/pfn_mkclean_range case. I don't think
>> device-private folios would ever have to mess with that.
>>
>>
>> Then, you just always have a folio+page and don't even have to worry
>> about the pfn?
> 
> That sounds reasonable to me. We were hesitant to add a page back to the
> interface given it had been removed previously but lets try implementing this to
> see what it looks like.

Thanks!

-- 
Cheers,

David