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.
