lkml.org 
[lkml]   [2021]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 1/7] mm,memory_hotplug: Allocate memmap from the added memory range
    On Thu, Feb 25, 2021 at 07:58:01PM +0100, David Hildenbrand wrote:
    > > In this way, we have:
    > >
    > > (start_pfn, buddy_start_pfn - 1] = Initialized and PageReserved
    > > (buddy_start_pfn, end_pfn] = Initialized and sent to buddy
    >
    > nit: shouldn't it be
    >
    > [start_pfn, buddy_start_pfn - 1]
    > [buddy_start_pfn, end_pfn - 1]
    >
    > or
    >
    > [start_pfn, buddy_start_pfn)
    > [buddy_start_pfn, end_pfn)
    >
    > (I remember that "[" means inclusive and "(" means exclusive, I might be wrong :) )
    >
    > I actually prefer the first variant.

    Let us go witht the first variant, I guess it is more clear.

    > > -static void online_pages_range(unsigned long start_pfn, unsigned long nr_pages)
    > > +static void online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
    > > + unsigned long buddy_start_pfn)
    > > {
    > > const unsigned long end_pfn = start_pfn + nr_pages;
    > > - unsigned long pfn;
    > > + unsigned long pfn = buddy_start_pfn;
    > > +
    > > + /*
    > > + * When using memmap_on_memory, the range might be unaligned as the
    > > + * first pfns are used for vmemmap pages. Align it in case we need to.
    > > + */
    > > + if (pfn & ((1 << (MAX_ORDER - 1)) - 1)) {
    >
    > if (!IS_ALIGNED(pfn, MAX_ORDER_NR_PAGES))

    Will change

    >
    > > + (*online_page_callback)(pfn_to_page(pfn), pageblock_order);
    > > + pfn += 1 << pageblock_order;
    >
    > pfn += pageblock_nr_pages;
    >
    > Can you add a comment why we can be sure that we are off by a single pageblock? What about s390x where a MAX_ORDER_NR_PAGES == 4 * pageblock_nr_pages?
    >
    > Would it make thing simpler to just do a
    >
    > while (!IS_ALIGNED(pfn, MAX_ORDER_NR_PAGES)) {
    > (*online_page_callback)(pfn_to_page(pfn), 0);
    > pfn++;
    > }

    Honestly, I did not spend much time thinking on other platforms other
    than arm64/x86_64.
    But I think that that would be the universal solution as we do not make
    any assumptions.

    I will replace it.

    > > +bool mhp_supports_memmap_on_memory(unsigned long size)
    > > +{
    > > + return memmap_on_memory_enabled &&
    > > + size == memory_block_size_bytes();
    >
    > Regarding my other comments as reply to the other patches, I'd move all magic you have when trying to enable right here.

    Ok, will do.

    > > @@ -1613,7 +1658,7 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages)
    > > zone_pcp_disable(zone);
    > > /* set above range as isolated */
    > > - ret = start_isolate_page_range(start_pfn, end_pfn,
    > > + ret = start_isolate_page_range(buddy_start_pfn, end_pfn,
    > > MIGRATE_MOVABLE,
    > > MEMORY_OFFLINE | REPORT_FAILURE);
    >
    > Did you take care to properly adjust undo_isolate_page_range() as well? I can't spot it.

    No, I did not. Good that you noticed :-)

    Will fix it up in the next version.

    > > +static int get_nr_vmemmap_pages_cb(struct memory_block *mem, void *arg)
    > > +{
    > > + unsigned long *nr_vmemmap_pages = (unsigned long *)arg;
    > > +
    > > + *nr_vmemmap_pages += mem->nr_vmemmap_pages;
    > > + return mem->nr_vmemmap_pages;
    > > +}
    > > +
    >
    > I think you can do this easier, all you want to know is if there
    > is any block that has nr_vmemmap_pages set - and return the value.
    >
    > static int first_set_nr_vmemmap_pages_cb(struct memory_block *mem, void *arg)
    > {
    > /* If not set, continue with the next block. */
    > return mem->nr_vmemmap_pages;
    > }

    Yeah, less code.
    Will fix it.

    >
    > ...
    > > + walk_memory_blocks(start, size, &nr_vmemmap_pages,
    > > + get_nr_vmemmap_pages_cb);
    >
    > ...
    >
    > mem->nr_vmemmap_pages = walk_memory_blocks(start ...)
    >
    >
    >
    > Looks quite promising, only a couple of things to fine-tune :) Thanks!

    Thanks for having a look, that is highly appreciated!

    Let us see if we can polish the minor things that are missing and target
    this for the next release.


    --
    Oscar Salvador
    SUSE L3

    \
     
     \ /
      Last update: 2021-02-28 19:52    [W:7.874 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site