lkml.org 
[lkml]   [2021]   [Mar]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 1/5] mm,memory_hotplug: Allocate memmap from the added memory range
From
Date
On 16.03.21 17:46, David Hildenbrand wrote:
> On 15.03.21 11:22, Oscar Salvador wrote:
>> On Thu, Mar 11, 2021 at 08:06:53PM +0100, David Hildenbrand wrote:
>>> This looks essentially good to me, except some parts in
>>> mhp_supports_memmap_on_memory()
>>>
>>>> +bool mhp_supports_memmap_on_memory(unsigned long size)
>>>> +{
>>>> + unsigned long pageblock_size = PFN_PHYS(pageblock_nr_pages);
>>>> + unsigned long remaining_mem = size - PMD_SIZE;
>>
>> Hi David, thanks for the review!
>>
>>> This looks weird. I think what you want to test is that
>>>
>>>
>>> a) "nr_vmemmap_pages * sizeof(struct page)" spans complete PMDs (IOW, we
>>> won't map too much via the altmap when populating a PMD in the vmemmap)
>>>
>>> b) "remaining = size - nr_vmemmap_pages * sizeof(struct page)" spans
>>> complete pageblock.
>>
>> We do not know the nr_vmemmap_pages at this point in time, although it is
>> easy to pre-compute.
>>
>> For every section we populate, we use PMD_SIZE. So, PMD_SIZE/PAGE_SIZE lays
>> the nr_vmemmap_pages that are used for populating a single section.
>
> I find that cross reference to vmemmap code a little hard to digest.
> I would have assume that we don't have to care about PMDs in this
> code here at all. The vmemmap population code should handle that.
>
> I think I already mentioned that somewhere, I think it should be like this:
>
> a) vmemmap code should *never* populate more memory than requested for
> a single memory section when we are populating from the altmap.
> If that cannot be guaranteed for PMDs, then we have to fallback
> to populating base pages. Populating PMDs from an altmap with
> sizeof(struct page) == 64 is highly dangerous.
>
> Assume we have sizeof(struct page) == 56. A 128 MiB section
> spans 32768 pages - we need 32768 * sizeof(struct page)
> space for the vmemmap.
>
> With 64k pages we *can* use exactly one PMD. With 56k pages
> we need 448 individual (full!) pages for the vmemmap.
>
> IOW, we don't care how vmemmap code will do the mapping.
> vmemmap code has to get it right. IMHO, asserting it in
> this code is wrong.
>
>
> b) In this code, we really should only care about what
> memory onlining/offlining code can or can't do.
> We really only care that
>
> 1) size == memory_block_size_bytes()
> 2) remaining_size
> 3) IS_ALIGNED(remaining_size, pageblock_size);
>
>
> I think a) is a logical consequence of b) for x86-64,
> whereby the pageblock size corresponds to PMD, so we
> might not have to care about a) right now.
>
> See below for my take.
>
>
>>
>> But let me explain the reasoning I use in the current code:
>>
>> I will enumarate the assumptions that must hold true in order to support the
>> feature together with their check:
>>
>> - We span a single memory block
>>
>> size == memory_block_size_bytes()
>>
>> - The vmemmap pages span a complete PMD and no more than a PMD.
>>
>> !(PMD_SIZE % sizeof(struct page))
>>
>> - The vmemmap pages and the pages exposed to the buddy have to cover full
>> pageblocks
>>
>> remaining_mem = size - PMD_SIZE;
>> IS_ALIGNED(remaining_mem, pageblock_size)
>>
>> Although this check only covers the range without the vmemmap pages, one could
>> argue that since we use only a PMD_SIZE at a time, we know that PMD_SIZE is
>> pageblock aligned, so the vmemmap range is PMD_SIZE as well.
>>
>> Now, I see how this might be confusing and rather incomplete.
>> So I guess a better and more clear way to write it would be:
>>
>> bool mhp_supports_memmap_on_memory(unsigned long size)
>> {
>> unsigned long nr_vmemmap_pages = PMD_SIZE / PAGE_SIZE;
>> unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page);
>> unsigned long remaining_size = size - vmemmap_size;
>>
>> return memmap_on_memory &&
>> IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
>> size == memory_block_size_bytes() &&
>> !(PMD_SIZE % vmemmap_size) &&
>> IS_ALIGNED(vmemmap_size, pageblock_size) &&
>> remaining_size &&
>> IS_ALIGNED(remaining_size, pageblock_size);
>> }
>>
>> Note that above check is only for a single section, but if assumptions hold true
>> for a single section, it will for many as well.
>
> Okay, please document the statement about single sections, that's
> important to understand what's happening.
>
> My take would be
>
> bool mhp_supports_memmap_on_memory(unsigned long size)
> {
> /*
> * Note: We calculate for a single memory section. The calculation
> * implicitly covers memory blocks that span multiple sections.
> */
> unsigned long nr_vmemmap_pages = SECTION_SIZE / PAGE_SIZE;
> unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page);
> unsigned long remaining_size = size - vmemmap_size;
>
> /*
> * Note: vmemmap code has to make sure to not populate more memory
> * via the altmap than absolutely necessary for a single section.
> * This always holds when we allocate pageblock-sized chunks from
> * the altmap, as we require pageblock alignment here.
> *
> * TODO, other doc
> */
> return memmap_on_memory &&
> IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
> size == memory_block_size_bytes() &&
> remaining_size &&
> IS_ALIGNED(remaining_size, pageblock_size);

Oh, I forgot, we can also drop the check for "remaining_size" in that
code. If we ever have PAGE_SIZE == sizeof(struct page) we'll be in
bigger trouble :D


--
Thanks,

David / dhildenb

\
 
 \ /
  Last update: 2021-03-16 18:46    [W:0.080 / U:0.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site