Re: [PATCH v2 1/5] mm/hotplug: Embed vmem_altmap details in memory block

[David Hildenbrand <david@redhat.com>]

On 07.07.23 15:30, Aneesh Kumar K V wrote:
> On 7/7/23 5:47 PM, David Hildenbrand wrote:
>> On 06.07.23 18:06, Aneesh Kumar K V wrote:
>>> On 7/6/23 6:29 PM, David Hildenbrand wrote:
>>>> On 06.07.23 14:32, Aneesh Kumar K V wrote:
>>>>> On 7/6/23 4:44 PM, David Hildenbrand wrote:
>>>>>> On 06.07.23 11:36, Aneesh Kumar K V wrote:
>>>>>>> On 7/6/23 2:48 PM, David Hildenbrand wrote:
>>>>>>>> On 06.07.23 10:50, Aneesh Kumar K.V wrote:
>>>>>>>>> With memmap on memory, some architecture needs more details w.r.t altmap
>>>>>>>>> such as base_pfn, end_pfn, etc to unmap vmemmap memory.
>>>>>>>>
>>>>>>>> Can you elaborate why ppc64 needs that and x86-64 + aarch64 don't?
>>>>>>>>
>>>>>>>> IOW, why can't ppc64 simply allocate the vmemmap from the start of the memblock (-> base_pfn) and use the stored number of vmemmap pages to calculate the end_pfn?
>>>>>>>>
>>>>>>>> To rephrase: if the vmemmap is not at the beginning and doesn't cover full apgeblocks, memory onlining/offlining would be broken.
>>>>>>>>
>>>>>>>> [...]
>>>>>>>
>>>>>>>
>>>>>>> With ppc64 and 64K pagesize and different memory block sizes, we can end up allocating vmemmap backing memory from outside altmap because
>>>>>>> a single page vmemmap can cover 1024 pages (64 *1024/sizeof(struct page)). and that can point to pages outside the dev_pagemap range.
>>>>>>> So on free we  check
>>>>>>
>>>>>> So you end up with a mixture of altmap and ordinarily-allocated vmemmap pages? That sound wrong (and is counter-intuitive to the feature in general, where we *don't* want to allocate the vmemmap from outside the altmap).
>>>>>>
>>>>>> (64 * 1024) / sizeof(struct page) -> 1024 pages
>>>>>>
>>>>>> 1024 pages * 64k = 64 MiB.
>>>>>>
>>>>>> What's the memory block size on these systems? If it's >= 64 MiB the vmemmap of a single memory block fits into a single page and we should be fine.
>>>>>>
>>>>>> Smells like you want to disable the feature on a 64k system.
>>>>>>
>>>>>
>>>>> But that part of vmemmap_free is common for both dax,dax kmem and the new memmap on memory feature. ie, ppc64 vmemmap_free have checks which require
>>>>> a full altmap structure with all the details in. So for memmap on memmory to work on ppc64 we do require similar altmap struct. Hence the idea
>>>>> of adding vmemmap_altmap to  struct memory_block
>>>>
>>>> I'd suggest making sure that for the memmap_on_memory case your really *always* allocate from the altmap (that's what the feature is about after all), and otherwise block the feature (i.e., arch_mhp_supports_... should reject it).
>>>>
>>>
>>> Sure. How about?
>>>
>>> bool mhp_supports_memmap_on_memory(unsigned long size)
>>> {
>>>
>>>      unsigned long nr_pages = size >> PAGE_SHIFT;
>>>      unsigned long vmemmap_size = nr_pages * sizeof(struct page);
>>>
>>>      if (!radix_enabled())
>>>          return false;
>>>      /*
>>>       * memmap on memory only supported with memory block size add/remove
>>>       */
>>>      if (size != memory_block_size_bytes())
>>>          return false;
>>>      /*
>>>       * Also make sure the vmemmap allocation is fully contianed
>>>       * so that we always allocate vmemmap memory from altmap area.
>>>       */
>>>      if (!IS_ALIGNED(vmemmap_size,  PAGE_SIZE))
>>>          return false;
>>>      /*
>>>       * The pageblock alignment requirement is met by using
>>>       * reserve blocks in altmap.
>>>       */
>>>      return true;
>>> }
>>
>> Better, but the PAGE_SIZE that could be added to common code as well.
>>
>> ... but, the pageblock check in common code implies a PAGE_SIZE check, so why do we need any other check besides the radix_enabled() check for arm64 and just keep all the other checks in common code as they are?
>>
>> If your vmemmap does not cover full pageblocks (which implies full pages), the feature cannot be used *unless* we'd waste altmap space in the vmemmap to cover one pageblock.
>>
>> Wasting hotplugged memory certainly sounds wrong?
>>
>>
>> So I appreciate if you could explain why the pageblock check should not be had for ppc64?
>>
> 
> If we want things to be aligned to pageblock (2M) we will have to use 2M vmemmap space and that implies a memory block of 2G with 64K page size. That requirements makes the feature not useful at all
> on power. The compromise i came to was what i mentioned in the commit message for enabling the feature on ppc64.

As we'll always handle a 2M pageblock, you'll end up wasting memory.

Assume a 64MiB memory block:

With 64k: 1024 pages -> 64k vmemmap, almost 2 MiB wasted. ~3.1 %
With 4k: 16384 pages -> 1 MiB vmemmap, 1 MiB wasted. ~1.5%

It gets worse with smaller memory block sizes.


> 
> We  use altmap.reserve feature to align things correctly at pageblock granularity. We can end up loosing some pages in memory with this. For ex: with 256MB memory block
> size, we require 4 pages to map vmemmap pages, In order to align things correctly we end up adding a reserve of 28 pages. ie, for every 4096 pages
> 28 pages get reserved.


You can simply align-up the nr_vmemmap_pages up to pageblocks in the 
memory hotplug code (e.g., depending on a config/arch knob whether 
wasting memory is supported).

Because the pageblock granularity is a memory onlining/offlining 
limitation and should be checked+handled exactly there.

-- 
Cheers,

David / dhildenb