lkml.org 
[lkml]   [2020]   [Nov]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [RFC PATCH 3/3] mm,memory_hotplug: Allocate memmap from the added memory range
From
Date
On 22.10.20 14:58, Oscar Salvador wrote:
> Physical memory hotadd has to allocate a memmap (struct page array) for
> the newly added memory section. Currently, alloc_pages_node() is used
> for those allocations.
>
> This has some disadvantages:
> a) an existing memory is consumed for that purpose
> (eg: ~2MB per 128MB memory section on x86_64)
> b) if the whole node is movable then we have off-node struct pages
> which has performance drawbacks.
> c) It might be there are no PMD_ALIGNED chunks so memmap array gets
> populated with base pages.
>
> This can be improved when CONFIG_SPARSEMEM_VMEMMAP is enabled.
>
> Vmemap page tables can map arbitrary memory.
> That means that we can simply use the beginning of each memory section and
> map struct pages there.
> struct pages which back the allocated space then just need to be treated
> carefully.
>
> Implementation wise we will reuse vmem_altmap infrastructure to override
> the default allocator used by __populate_section_memmap. Once the memmap is
> allocated, we are going to need a way to mark altmap pfns used for the allocation.
> If MHP_MEMMAP_ON_MEMORY flag was passed, we will set up the layout of the
> altmap structure in add_memory_resouce(), and then we will call
> mhp_mark_vmemmap_pages() to properly mark those pages.
>
> Online/Offline:
>
> In the memory_block structure, a new field is created in order to
> store the number of vmemmap_pages.
> Having that around simplifies things a lot since in {online/offline}_pages
> we can know how much we have to skip forward until we have the first non-
> vmemmap page, for operations like isolation/migration/initialization.
>
> Hot-remove:
>
> If the range was using memmap on memory (aka vmemmap pages),
> we construct an altmap structure so free_hugepage_table does
> the right thing and calls vmem_altmap_free instead of
> free_pagetable.
>

Sorry for the late replay, fairly busy with all kinds of things.

[...]

> diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
> index b02fd51e5589..6b57bf90ca72 100644
> --- a/drivers/acpi/acpi_memhotplug.c
> +++ b/drivers/acpi/acpi_memhotplug.c
> @@ -195,7 +195,7 @@ static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
> node = memory_add_physaddr_to_nid(info->start_addr);
>
> result = __add_memory(node, info->start_addr, info->length,
> - MHP_NONE);
> + MEMHP_MEMMAP_ON_MEMORY);

I'd suggest moving that into a separate patch.

[...]

> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index 439a89e758d8..7cc93de5856c 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -30,6 +30,7 @@ struct memory_block {
> int phys_device; /* to which fru does this belong? */
> struct device dev;
> int nid; /* NID for this memory block */
> + unsigned long nr_vmemmap_pages; /* Number for vmemmap pages */

Maybe also document that these pages are directly at the beginning of
the memory block.


>
> -static inline int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
> +static inline int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
> + unsigned long nr_vmemmap_pages)
> {
> return -EINVAL;
> }
> @@ -369,10 +372,12 @@ extern int add_memory_driver_managed(int nid, u64 start, u64 size,
> mhp_t mhp_flags);
> extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
> unsigned long nr_pages,
> + unsigned long nr_vmemmap_pages,
> struct vmem_altmap *altmap, int migratetype);
> extern void remove_pfn_range_from_zone(struct zone *zone,
> unsigned long start_pfn,
> - unsigned long nr_pages);
> + unsigned long nr_pages,
> + unsigned long nr_vmemmap_pages);

I think we should not pass nr_vmemmap_pages down here but instead do two
separate calls to move_pfn_range_to_zone()/remove_pfn_range_from_zone()
from online_pages()/offline_pages()

1. for vmemmap pages, migratetype = MIGRATE_UNMOVABLE
2. for remaining pages, migratetype = MIGRATE_ISOLATE

[...]

>
> /* Select all remaining pages up to the next section boundary */
> @@ -629,6 +630,7 @@ static void online_pages_range(unsigned long start_pfn, unsigned long nr_pages)
> {
> const unsigned long end_pfn = start_pfn + nr_pages;
> unsigned long pfn;
> + unsigned int order;
>
> /*
> * Online the pages in MAX_ORDER - 1 aligned chunks. The callback might
> @@ -636,8 +638,12 @@ static void online_pages_range(unsigned long start_pfn, unsigned long nr_pages)
> * later). We account all pages as being online and belonging to this
> * zone ("present").
> */
> - for (pfn = start_pfn; pfn < end_pfn; pfn += MAX_ORDER_NR_PAGES)
> - (*online_page_callback)(pfn_to_page(pfn), MAX_ORDER - 1);
> + for (pfn = start_pfn; pfn < end_pfn; pfn += (1 << order)) {
> + order = MAX_ORDER - 1;
> + while (pfn & ((1 << order) - 1))
> + order--;
> + (*online_page_callback)(pfn_to_page(pfn), order);
> + }
>
> /* mark all involved sections as online */
> online_mem_sections(start_pfn, end_pfn);
> @@ -697,6 +703,18 @@ static void __meminit resize_pgdat_range(struct pglist_data *pgdat, unsigned lon
> pgdat->node_spanned_pages = max(start_pfn + nr_pages, old_end_pfn) - pgdat->node_start_pfn;
>
> }
> +
> +static void link_vmemmap_pages(unsigned long pfn, unsigned long nr_pages,
> + struct zone *zone, int nid)
> +{
> + struct page *p = pfn_to_page(pfn);
> + enum zone_type z = zone_idx(zone);
> + unsigned long i;
> +
> + for (i = 0; i < nr_pages; i++)
> + set_page_links(p + i, z, nid, pfn);
> +}
> +
> /*
> * Associate the pfn range with the given zone, initializing the memmaps
> * and resizing the pgdat/zone data to span the added pages. After this
> @@ -708,6 +726,7 @@ static void __meminit resize_pgdat_range(struct pglist_data *pgdat, unsigned lon
> */
> void __ref move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
> unsigned long nr_pages,
> + unsigned long nr_vmemmap_pages,
> struct vmem_altmap *altmap, int migratetype)
> {
> struct pglist_data *pgdat = zone->zone_pgdat;
> @@ -726,14 +745,22 @@ void __ref move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
> resize_pgdat_range(pgdat, start_pfn, nr_pages);
> pgdat_resize_unlock(pgdat, &flags);
>
> + if (nr_vmemmap_pages)
> + /*
> + * Vmemmap pages are already initialized at hot-add state.
> + * What is left is to link them to a node and a zone.
> + */
> + link_vmemmap_pages(start_pfn, nr_vmemmap_pages, zone, nid);
> +
> /*
> * TODO now we have a visible range of pages which are not associated
> * with their zone properly. Not nice but set_pfnblock_flags_mask
> * expects the zone spans the pfn range. All the pages in the range
> * are reserved so nobody should be touching them so we should be safe
> */
> - memmap_init_zone(nr_pages, nid, zone_idx(zone), start_pfn,
> - MEMINIT_HOTPLUG, altmap, migratetype);
> + memmap_init_zone(nr_pages - nr_vmemmap_pages, nid, zone_idx(zone),
> + start_pfn + nr_vmemmap_pages, MEMINIT_HOTPLUG, altmap,
> + migratetype);
>
> set_zone_contiguous(zone);
> }
> @@ -796,9 +823,9 @@ struct zone * zone_for_pfn_range(int online_type, int nid, unsigned start_pfn,
> }
>
> int __ref online_pages(unsigned long pfn, unsigned long nr_pages,
> - int online_type, int nid)
> + unsigned long nr_vmemmap_pages, int online_type, int nid)
> {
> - unsigned long flags;
> + unsigned long flags, valid_start_pfn, valid_nr_pages;
> struct zone *zone;
> int need_zonelists_rebuild = 0;
> int ret;
> @@ -809,11 +836,15 @@ int __ref online_pages(unsigned long pfn, unsigned long nr_pages,
> !IS_ALIGNED(pfn | nr_pages, PAGES_PER_SECTION)))
> return -EINVAL;
>
> + valid_start_pfn = pfn + nr_vmemmap_pages;
> + valid_nr_pages = nr_pages - nr_vmemmap_pages;

Hm, valid sounds strange. More like "free_start_pfn" or "buddy_start_pfn".

> +
> mem_hotplug_begin();
>
> /* associate pfn range with the zone */
> zone = zone_for_pfn_range(online_type, nid, pfn, nr_pages);
> - move_pfn_range_to_zone(zone, pfn, nr_pages, NULL, MIGRATE_ISOLATE);
> + move_pfn_range_to_zone(zone, pfn, nr_pages, nr_vmemmap_pages, NULL,
> + MIGRATE_ISOLATE);

As mentioned, I'd suggest properly initializing the memmap here

if (nr_vmemmap_pages) {
move_pfn_range_to_zone(zone, pfn, nr_vmemmap_pages, NULL,
MIGRATE_UNMOVABLE);
}
move_pfn_range_to_zone(zone, valid_start_pfn, valid_nr_pages, NULL,
MIGRATE_ISOLATE);

[...]

> /*
> * NOTE: The caller must call lock_device_hotplug() to serialize hotplug
> * and online/offline operations (triggered e.g. by sysfs).
> @@ -1039,6 +1080,7 @@ static int online_memory_block(struct memory_block *mem, void *arg)
> int __ref add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
> {
> struct mhp_params params = { .pgprot = PAGE_KERNEL };
> + struct vmem_altmap mhp_altmap = {};
> u64 start, size;
> bool new_node = false;
> int ret;
> @@ -1065,18 +1107,36 @@ int __ref add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
> goto error;
> new_node = ret;
>
> + /*
> + * Check whether we support memmap_on_memory
> + */
> + if (!support_memmap_on_memory(size))
> + mhp_flags &= ~MEMHP_MEMMAP_ON_MEMORY;

Callers (e.g., virtio-mem) might rely on this. We should reject this
with -EINVAL and provide a way for callers to test whether this flag is
possible.

> +
> + /*
> + * Self hosted memmap array
> + */
> + if (mhp_flags & MEMHP_MEMMAP_ON_MEMORY) {
> + mhp_altmap.free = size >> PAGE_SHIFT;
> + mhp_altmap.base_pfn = start >> PAGE_SHIFT;
> + params.altmap = &mhp_altmap;
> + }
> +
> /* call arch's memory hotadd */
> ret = arch_add_memory(nid, start, size, &params);
> if (ret < 0)
> goto error;
>
> /* create memory block devices after memory was added */
> - ret = create_memory_block_devices(start, size);
> + ret = create_memory_block_devices(start, size, mhp_altmap.alloc);
> if (ret) {
> arch_remove_memory(nid, start, size, NULL);
> goto error;
> }
>
> + if (mhp_flags & MEMHP_MEMMAP_ON_MEMORY)
> + mhp_mark_vmemmap_pages(params.altmap);

Do we really still need that? Pages are offline, so we're messing with
an invalid memmap. online_pages() should handle initializing the memmap
of these pages.

[...]

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index e74ca22baaa1..043503fb8c6e 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8761,6 +8761,13 @@ void __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
> spin_lock_irqsave(&zone->lock, flags);
> while (pfn < end_pfn) {
> page = pfn_to_page(pfn);
> + /*
> + * Skip vmemmap pages
> + */
> + if (PageVmemmap(page)) {
> + pfn += vmemmap_nr_pages(page);
> + continue;
> + }

I'd assume calling code can handle that and exclude isolating such pages.


--
Thanks,

David / dhildenb
\
 
 \ /
  Last update: 2020-11-17 16:41    [W:0.138 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site