David Hildenbrand <david@xxxxxxxxxx> writes:
> On 25.07.23 12:02, Aneesh Kumar K.V wrote:
>> Currently, memmap_on_memory feature is only supported with memory block
>> sizes that result in vmemmap pages covering full page blocks. This is
>> because memory onlining/offlining code requires applicable ranges to be
>> pageblock-aligned, for example, to set the migratetypes properly.
>>
>> This patch helps to lift that restriction by reserving more pages than
>> required for vmemmap space. This helps the start address to be page
>> block aligned with different memory block sizes. Using this facility
>> implies the kernel will be reserving some pages for every memoryblock.
>> This allows the memmap on memory feature to be widely useful with
>> different memory block size values.
>>
>> For ex: with 64K page size and 256MiB memory block size, we require 4
>> pages to map vmemmap pages, To align things correctly we end up adding a
>> reserve of 28 pages. ie, for every 4096 pages 28 pages get reserved.
>>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxx>
>> ---
>> .../admin-guide/mm/memory-hotplug.rst | 12 ++
>> mm/memory_hotplug.c | 121 ++++++++++++++++--
>> 2 files changed, 119 insertions(+), 14 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/mm/memory-hotplug.rst b/Documentation/admin-guide/mm/memory-hotplug.rst
>> index bd77841041af..2994958c7ce8 100644
>> --- a/Documentation/admin-guide/mm/memory-hotplug.rst
>> +++ b/Documentation/admin-guide/mm/memory-hotplug.rst
>> @@ -433,6 +433,18 @@ The following module parameters are currently defined:
>> memory in a way that huge pages in bigger
>> granularity cannot be formed on hotplugged
>> memory.
>> +
>> + With value "force" it could result in memory
>> + wastage due to memmap size limitations. For
>> + example, if the memmap for a memory block
>> + requires 1 MiB, but the pageblock size is 2
>> + MiB, 1 MiB of hotplugged memory will be wasted.
>> + Note that there are still cases where the
>> + feature cannot be enforced: for example, if the
>> + memmap is smaller than a single page, or if the
>> + architecture does not support the forced mode
>> + in all configurations.
>> +
>> ``online_policy`` read-write: Set the basic policy used for
>> automatic zone selection when onlining memory
>> blocks without specifying a target zone.
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index 457824a6ecb8..5b472e137898 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -41,17 +41,89 @@
>> #include "internal.h"
>> #include "shuffle.h"
>>
>> +enum {
>> + MEMMAP_ON_MEMORY_DISABLE = 0,
>> + MEMMAP_ON_MEMORY_ENABLE,
>> + MEMMAP_ON_MEMORY_FORCE,
>> +};
>> +
>> +static int memmap_mode __read_mostly = MEMMAP_ON_MEMORY_DISABLE;
>> +
>> +static inline unsigned long memory_block_memmap_pages(void)
>> +{
>> + unsigned long memmap_size;
>> +
>> + memmap_size = PHYS_PFN(memory_block_size_bytes()) * sizeof(struct page);
>> + return memmap_size >> PAGE_SHIFT;
>
> I'd really move a !page variant (memory_block_memmap_size()) to the
> previous patch and use it in mhp_supports_memmap_on_memory() and
> arch_supports_memmap_on_memory().
>
> Then, in this patch, reuse that function in
> memory_block_memmap_on_memory_pages() and ...
>
>> +}
>> +
>> +static inline unsigned long memory_block_memmap_on_memory_pages(void)
>> +{
>> + unsigned long nr_pages = memory_block_memmap_pages();
>
> ... do here a
>
> nr_pages = PHYS_PFN(memory_block_memmap_size());
>
>
> Conceptually, it would be even cleaner to have here
>
> nr_pages = PFN_UP(memory_block_memmap_size());
>
> even though one can argue that mhp_supports_memmap_on_memory() will make
> sure that the unaligned value (memory_block_memmap_size()) covers full
> pages, but at least to me it looks cleaner that way. No strong opinion.
>
>
>> +
>> + /*
>> + * In "forced" memmap_on_memory mode, we add extra pages to align the
>> + * vmemmap size to cover full pageblocks. That way, we can add memory
>> + * even if the vmemmap size is not properly aligned, however, we might waste
>> + * memory.
>> + */
>> + if (memmap_mode == MEMMAP_ON_MEMORY_FORCE)
>> + return pageblock_align(nr_pages);
>> + return nr_pages;
>> +}
>> +
>> #ifdef CONFIG_MHP_MEMMAP_ON_MEMORY
>> /*
>> * memory_hotplug.memmap_on_memory parameter
>> */
>> -static bool memmap_on_memory __ro_after_init;
>> -module_param(memmap_on_memory, bool, 0444);
>> -MODULE_PARM_DESC(memmap_on_memory, "Enable memmap on memory for memory hotplug");
>> +static int set_memmap_mode(const char *val, const struct kernel_param *kp)
>> +{
>> + int ret, mode;
>> + bool enabled;
>> +
>> + if (sysfs_streq(val, "force") || sysfs_streq(val, "FORCE")) {
>> + mode = MEMMAP_ON_MEMORY_FORCE;
>> + goto matched;
>> + }
>
> Avoid the goto + label
>
> } else {
> ret = kstrtobool(val, &enabled);
> ...
> }
>
> *((int *)kp->arg) = mode;
>
>> +
>> + ret = kstrtobool(val, &enabled);
>> + if (ret < 0)
>> + return ret;
>> + if (enabled)
>> + mode = MEMMAP_ON_MEMORY_ENABLE;
>> + else
>> + mode = MEMMAP_ON_MEMORY_DISABLE;
>> +
>> +matched:
>> + *((int *)kp->arg) = mode;
>> + if (mode == MEMMAP_ON_MEMORY_FORCE) {
>> + unsigned long memmap_pages = memory_block_memmap_on_memory_pages();
>> +
>> + pr_info("Memory hotplug will reserve %ld pages in each memory block\n",
>> + memmap_pages - memory_block_memmap_pages());
>
> pr_info_once() ?
>
>> + }
>> + return 0;
>> +}
>> +
>
> [...]
>
>> /*
>> * Besides having arch support and the feature enabled at runtime, we
>> @@ -1294,10 +1366,28 @@ static bool mhp_supports_memmap_on_memory(unsigned long size)
>> * altmap as an alternative source of memory, and we do not exactly
>> * populate a single PMD.
>> */
>> - return mhp_memmap_on_memory() &&
>> - size == memory_block_size_bytes() &&
>> - IS_ALIGNED(remaining_size, (pageblock_nr_pages << PAGE_SHIFT)) &&
>> - arch_supports_memmap_on_memory(size);
>> + if (!mhp_memmap_on_memory() || size != memory_block_size_bytes())
>> + return false;
>> +
>> + /*
>> + * Make sure the vmemmap allocation is fully contained
>> + * so that we always allocate vmemmap memory from altmap area.
>> + */
>> + if (!IS_ALIGNED(vmemmap_size, PAGE_SIZE))
>> + return false;
>> +
>> + /*
>> + * start pfn should be pageblock_nr_pages aligned for correctly
>> + * setting migrate types
>> + */
>> + if (!pageblock_aligned(memmap_pages))
>> + return false;
>> +
>> + if (memmap_pages == PHYS_PFN(memory_block_size_bytes()))
>> + /* No effective hotplugged memory doesn't make sense. */
>> + return false;
>> +
>> + return arch_supports_memmap_on_memory(size);
>> }
>>
>> /*
>> @@ -1310,7 +1400,10 @@ int __ref add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
>> {
>> struct mhp_params params = { .pgprot = pgprot_mhp(PAGE_KERNEL) };
>> enum memblock_flags memblock_flags = MEMBLOCK_NONE;
>> - struct vmem_altmap mhp_altmap = {};
>> + struct vmem_altmap mhp_altmap = {
>> + .base_pfn = PHYS_PFN(res->start),
>> + .end_pfn = PHYS_PFN(res->end),
>
> Is it required to set .end_pfn, and if so, shouldn't we also set it to
> base_pfn + memory_block_memmap_on_memory_pages()) ?
>
We use that in ppc64 for checking altmap boundary condition. As we
discussed earlier, ppc64 due to vmemmap mapping size restrictions can't
always allocate vmemmap pages from altmap area even if requested. We
fallback to regular memory alocation in that case (only used now with
pmem). We use altmap.end_pfn for that boundary check. You can refer to
altmap_cross_boundary() for more details.
>
> We also don't set it on the try_remove_memory() part,.
>
We use altmap.end_pfn only in the allocation path. This confusion is
also another motivation to embed altmap in memory block structure as
done in patch 6.
>
>
> With these things addressed, feel free to add
>
> Acked-by: David Hildenbrand <david@xxxxxxxxxx>
>
Thanks for all your help with review. Attaching the diff against
previous version taking care of the feedback above.
diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
index 7e46369d57c9..2209d66a034e 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -176,11 +176,8 @@ bool altmap_cross_boundary(struct vmem_altmap *altmap, unsigned long start,
* alignment requirement is met using altmap->reserve blocks.
*/
#define arch_supports_memmap_on_memory arch_supports_memmap_on_memory
-static inline bool arch_supports_memmap_on_memory(unsigned long size)
+static inline bool arch_supports_memmap_on_memory(unsigned long vmemmap_size)
{
- unsigned long nr_pages = size >> PAGE_SHIFT;
- unsigned long vmemmap_size = nr_pages * sizeof(struct page);
-
if (!radix_enabled())
return false;
/*
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 1c9069cc0263..cadf4209d7c4 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -49,17 +49,14 @@ enum {
static int memmap_mode __read_mostly = MEMMAP_ON_MEMORY_DISABLE;
-static inline unsigned long memory_block_memmap_pages(void)
+static inline unsigned long memory_block_memmap_size(void)
{
- unsigned long memmap_size;
-
- memmap_size = PHYS_PFN(memory_block_size_bytes()) * sizeof(struct page);
- return memmap_size >> PAGE_SHIFT;
+ return PHYS_PFN(memory_block_size_bytes()) * sizeof(struct page);
}
static inline unsigned long memory_block_memmap_on_memory_pages(void)
{
- unsigned long nr_pages = memory_block_memmap_pages();
+ unsigned long nr_pages = PFN_UP(memory_block_memmap_size());
/*
* In "forced" memmap_on_memory mode, we add extra pages to align the
@@ -83,18 +80,15 @@ static int set_memmap_mode(const char *val, const struct kernel_param *kp)
if (sysfs_streq(val, "force") || sysfs_streq(val, "FORCE")) {
mode = MEMMAP_ON_MEMORY_FORCE;
- goto matched;
+ } else {
+ ret = kstrtobool(val, &enabled);
+ if (ret < 0)
+ return ret;
+ if (enabled)
+ mode = MEMMAP_ON_MEMORY_ENABLE;
+ else
+ mode = MEMMAP_ON_MEMORY_DISABLE;
}
-
- ret = kstrtobool(val, &enabled);
- if (ret < 0)
- return ret;
- if (enabled)
- mode = MEMMAP_ON_MEMORY_ENABLE;
- else
- mode = MEMMAP_ON_MEMORY_DISABLE;
-
-matched:
/*
* Avoid changing memmap mode during hotplug.
*/
@@ -104,8 +98,8 @@ static int set_memmap_mode(const char *val, const struct kernel_param *kp)
if (mode == MEMMAP_ON_MEMORY_FORCE) {
unsigned long memmap_pages = memory_block_memmap_on_memory_pages();
- pr_info("Memory hotplug will reserve %ld pages in each memory block\n",
- memmap_pages - memory_block_memmap_pages());
+ pr_info_once("Memory hotplug will reserve %ld pages in each memory block\n",
+ memmap_pages - PFN_UP(memory_block_memmap_size()));
}
return 0;
}
@@ -1327,11 +1321,8 @@ static int online_memory_block(struct memory_block *mem, void *arg)
}
#ifndef arch_supports_memmap_on_memory
-static inline bool arch_supports_memmap_on_memory(unsigned long size)
+static inline bool arch_supports_memmap_on_memory(unsigned long vmemmap_size)
{
- unsigned long nr_vmemmap_pages = size >> PAGE_SHIFT;
- unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page);
-
/*
* As default, we want the vmemmap to span a complete PMD such that we
* can map the vmemmap using a single PMD if supported by the
@@ -1343,8 +1334,7 @@ static inline bool arch_supports_memmap_on_memory(unsigned long size)
static bool mhp_supports_memmap_on_memory(unsigned long size)
{
- unsigned long nr_vmemmap_pages = size >> PAGE_SHIFT;
- unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page);
+ unsigned long vmemmap_size = memory_block_memmap_size();
unsigned long memmap_pages = memory_block_memmap_on_memory_pages();
/*
@@ -1394,7 +1384,7 @@ static bool mhp_supports_memmap_on_memory(unsigned long size)
/* No effective hotplugged memory doesn't make sense. */
return false;
- return arch_supports_memmap_on_memory(size);
+ return arch_supports_memmap_on_memory(vmemmap_size);
}
/*
