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

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

On 7/15/21 1:52 PM, Joao Martins wrote:
> On 7/15/21 2:08 AM, Dan Williams wrote:
>> On Wed, Jul 14, 2021 at 12:36 PM Joao Martins <joao.m.martins@oracle.com> wrote:
>>> instead of
>>> base pages. When a compound page geometry is requested, all but the first
>>> page are initialised as tail pages instead of order-0 pages.
>>>
>>> For certain ZONE_DEVICE users like device-dax which have a fixed page size,
>>> this creates an opportunity to optimize GUP and GUP-fast walkers, treating
>>> it the same way as THP or hugetlb pages.
>>>
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> ---
>>>  include/linux/memremap.h | 17 +++++++++++++++++
>>>  mm/memremap.c            |  8 ++++++--
>>>  mm/page_alloc.c          | 34 +++++++++++++++++++++++++++++++++-
>>>  3 files changed, 56 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
>>> index 119f130ef8f1..e5ab6d4525c1 100644
>>> --- a/include/linux/memremap.h
>>> +++ b/include/linux/memremap.h
>>> @@ -99,6 +99,10 @@ struct dev_pagemap_ops {
>>>   * @done: completion for @internal_ref
>>>   * @type: memory type: see MEMORY_* in memory_hotplug.h
>>>   * @flags: PGMAP_* flags to specify defailed behavior
>>> + * @geometry: structural definition of how the vmemmap metadata is populated.
>>> + *     A zero or PAGE_SIZE defaults to using base pages as the memmap metadata
>>> + *     representation. A bigger value but also multiple of PAGE_SIZE 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;
>>>         const struct dev_pagemap_ops *ops;
>>>         void *owner;
>>>         int nr_range;
>>> @@ -130,6 +135,18 @@ static inline struct vmem_altmap *pgmap_altmap(struct dev_pagemap *pgmap)
>>>         return NULL;
>>>  }
>>>
>>> +static inline unsigned long pgmap_geometry(struct dev_pagemap *pgmap)
>>> +{
>>> +       if (!pgmap || !pgmap->geometry)
>>> +               return PAGE_SIZE;
>>> +       return pgmap->geometry;
>>> +}
>>> +
>>> +static inline unsigned long pgmap_pfn_geometry(struct dev_pagemap *pgmap)
>>> +{
>>> +       return PHYS_PFN(pgmap_geometry(pgmap));
>>> +}
>>
>> Are both needed? Maybe just have ->geometry natively be in nr_pages
>> units directly, because pgmap_pfn_geometry() makes it confusing
>> whether it's a geometry of the pfn or the geometry of the pgmap.
>>
> I use pgmap_geometry() largelly when we manipulate memmap in sparse-vmemmap code, as we
> deal with addresses/offsets/subsection-size. While using pgmap_pfn_geometry for code that
> deals with PFN initialization. For this patch I could remove the confusion.
> 
> And actually maybe I can just store the pgmap_geometry() value in bytes locally in
> vmemmap_populate_compound_pages() and we can remove this extra helper.
> 
But one nice property of pgmap_geometry() is the @pgmap check and not needing that the
driver initializes a pgmap->geometry property. So a zeroed value would still support pgmap
users on the old case where there's no @geometry (or the user doesn't care). So departing
from this helper might mean that either memremap_pages() sets the right @geometry if a
zeroed value is passed in. and __populate_section_memmap() makes sures pgmap is associated
when trying to figure out if there's a geometry to consider in the section mapping.