Re: [PATCH v4 04/14] mm/memremap: add ZONE_DEVICE support for compound pages

[Joao Martins <joao.m.martins@oracle.com>]

On 8/27/21 4:33 PM, Christoph Hellwig wrote:
> On Fri, Aug 27, 2021 at 03:58:09PM +0100, Joao Martins wrote:
>> + * @geometry: structural definition of how the vmemmap metadata is populated.
>> + *	A zero or 1 defaults to using base pages as the memmap metadata
>> + *	representation. A bigger value will set up compound struct pages
>> + *	representative of the requested geometry size.
>>   * @ops: method table
>>   * @owner: an opaque pointer identifying the entity that manages this
>>   *	instance.  Used by various helpers to make sure that no
>> @@ -114,6 +118,7 @@ struct dev_pagemap {
>>  	struct completion done;
>>  	enum memory_type type;
>>  	unsigned int flags;
>> +	unsigned long geometry;
> 
> So why not make this a shift as I suggested somewhere deep in the
> last thread? 

So the previous you suggested had the check for pgmap_geometry() > PAGE_SIZE,
but because pgmap_geometry() return 1 by default, then the pfn_end() - pfn
computation wouldn't change for those that don't request a geometry.

So rather than this that you initially suggested:

	refs = pfn_end(pgmap, range_id) - pfn_first(pgmap, range_id);
	if (pgmap_geometry(pgmap) > 1)
		refs /= pgmap_geometry(pgmap);

I would turn into this, because now for users which don't select geometry it's always a
division by 1:

	refs = pfn_end(pgmap, range_id) - pfn_first(pgmap, range_id);
	refs /= pgmap_geometry(pgmap);

So felt like doing it inline straight away inline when calling percpu_ref_get_many():
	
	(pfn_end(pgmap, range_id) - pfn_first(pgmap, range_id)) / pgmap_geometry(pgmap);

I can switch to a shift if you prefer:

	(pfn_end(pgmap, range_id) - pfn_first(pgmap, range_id))
		<< pgmap_geometry_order(pgmap);

> Also geometry sounds a bit strange, even if I can't really
> offer anything better offhand.
> 
We started with @align (like in device dax core), and then we switched
to @geometry because these are slightly different things (one relates
to vmemmap metadata structure (number of pages) and the other is how
the mmap is aligned to a page size. I couldn't suggest anything else,
besides a more verbose name like vmemmap_align maybe.

>> +static inline unsigned long pgmap_geometry(struct dev_pagemap *pgmap)
>> +{
>> +	if (pgmap && pgmap->geometry)
>> +		return pgmap->geometry;
> 
> Why does this need to support a NULL pgmap?
> 
This was mainly a defensive choice, similar to put_dev_pagemap(). There's
only one caller, which is populate_section_memmap with an altmap but without
pgmap.

>> +static void __ref memmap_init_compound(struct page *head, unsigned long head_pfn,
> 
> Overly long line.
> 
Fixed.

Interesting that checkpatch doesn't complain with one character past 80 lines.