Re: [PATCH v2 3/5] mm/hotplug: Simplify the handling of MHP_MEMMAP_ON_MEMORY flag

[David Hildenbrand <david@redhat.com>]

On 06.07.23 12:04, Aneesh Kumar K V wrote:
> On 7/6/23 2:54 PM, David Hildenbrand wrote:
>> On 06.07.23 10:50, Aneesh Kumar K.V wrote:
>>> Instead of checking for memmap on memory feature enablement within the
>>> functions checking for alignment, use the kernel parameter to control the
>>> memory hotplug flags. The generic kernel now enables memmap on memory
>>> feature if the hotplug flag request for the same.
>>>
>>> The ACPI code now can pass the flag unconditionally because the kernel will
>>> fallback to not using the feature if the alignment rules are not met.
>>>
>>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>>> ---
>>>    drivers/acpi/acpi_memhotplug.c |  3 +--
>>>    include/linux/memory_hotplug.h | 14 ++++++++++++++
>>>    mm/memory_hotplug.c            | 35 +++++++++++-----------------------
>>>    3 files changed, 26 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
>>> index 24f662d8bd39..4d0096fc4cc2 100644
>>> --- a/drivers/acpi/acpi_memhotplug.c
>>> +++ b/drivers/acpi/acpi_memhotplug.c
>>> @@ -211,8 +211,7 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
>>>            if (!info->length)
>>>                continue;
>>>    -        if (mhp_supports_memmap_on_memory(info->length))
>>> -            mhp_flags |= MHP_MEMMAP_ON_MEMORY;
>>> +        mhp_flags |= get_memmap_on_memory_flags();
>>>            result = __add_memory(mgid, info->start_addr, info->length,
>>>                          mhp_flags);
>>>    diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
>>> index a769f44b8368..af7017122506 100644
>>> --- a/include/linux/memory_hotplug.h
>>> +++ b/include/linux/memory_hotplug.h
>>> @@ -358,4 +358,18 @@ bool mhp_supports_memmap_on_memory(unsigned long size);
>>>    bool __mhp_supports_memmap_on_memory(unsigned long size);
>>>    #endif /* CONFIG_MEMORY_HOTPLUG */
>>>    +#ifdef CONFIG_MHP_MEMMAP_ON_MEMORY
>>> +extern bool memmap_on_memory;
>>> +static inline unsigned long get_memmap_on_memory_flags(void)
>>> +{
>>> +    if (memmap_on_memory)
>>> +        return MHP_MEMMAP_ON_MEMORY;
>>> +    return 0;
>>> +}
>>> +#else
>>> +static inline unsigned long get_memmap_on_memory_flags(void)
>>> +{
>>> +    return 0;
>>> +}
>>> +#endif
>>
>> That's kind-of ugly TBH.
>>
>>
>> Why do we need this change?
>>
> 
> I was trying to avoid rest of the kernel doing
> 
> if (mhp_supports_memmap_on_memory(info->length))
>           mhp_flags |= MHP_MEMMAP_ON_MEMORY;

It would look much cleaner if you would simply have:

mhp_flags |= MHP_MEMMAP_ON_MEMORY;
result = __add_memory(mgid, info->start_addr, info->length, mhp_flags);

And modify the semantics of MHP_MEMMAP_ON_MEMORY to mean "allocate the 
memmap from hotplugged memory if supported and enabled globally".

Then, we can simply ignore the flag in __add_memory() if either the 
global toggle is off or if it's not supported for the range / by the arch.

Maybe, in the future we want more fine-grained control (as asked by 
dax/kmem) and maybe not have the global toggle. But for now, that should 
be good enough I guess.

-- 
Cheers,

David / dhildenb