diff mbox series

[v20,8/9] mm: memory_hotplug: disable memmap_on_memory when hugetlb_free_vmemmap enabled

Message ID 20210415084005.25049-9-songmuchun@bytedance.com (mailing list archive)
State New, archived
Headers show
Series Free some vmemmap pages of HugeTLB page | expand

Commit Message

Muchun Song April 15, 2021, 8:40 a.m. UTC
The parameter of memory_hotplug.memmap_on_memory is not compatible with
hugetlb_free_vmemmap. So disable it when hugetlb_free_vmemmap is
enabled.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  4 ++++
 drivers/acpi/acpi_memhotplug.c                  |  1 +
 mm/memory_hotplug.c                             | 18 +++++++++++++-----
 3 files changed, 18 insertions(+), 5 deletions(-)

Comments

Oscar Salvador April 20, 2021, 10:35 a.m. UTC | #1
On Thu, Apr 15, 2021 at 04:40:04PM +0800, Muchun Song wrote:
>  bool mhp_supports_memmap_on_memory(unsigned long size)
>  {
> +	bool supported;
>  	unsigned long nr_vmemmap_pages = size / PAGE_SIZE;
>  	unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page);
>  	unsigned long remaining_size = size - vmemmap_size;
> @@ -1011,11 +1012,18 @@ 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 memmap_on_memory &&
> -	       IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
> -	       size == memory_block_size_bytes() &&
> -	       IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
> -	       IS_ALIGNED(remaining_size, pageblock_nr_pages << PAGE_SHIFT);
> +	supported = memmap_on_memory &&
> +		    IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
> +		    size == memory_block_size_bytes() &&
> +		    IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
> +		    IS_ALIGNED(remaining_size, pageblock_nr_pages << PAGE_SHIFT);
> +
> +	if (supported && is_hugetlb_free_vmemmap_enabled()) {
> +		pr_info("Cannot enable memory_hotplug.memmap_on_memory, it is not compatible with hugetlb_free_vmemmap\n");
> +		supported = false;
> +	}

I would not print anything and rather have

return memmap_on_memory &&
       !is_hugetlb_free_vmemmap_enabled &&
       IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
       size == memory_block_size_bytes() &&
       IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
       IS_ALIGNED(remaining_size, pageblock_nr_pages << PAGE_SHIFT);

Documentation/admin-guide/kernel-parameters.txt already provides an
explanation on memory_hotplug.memmap_on_memory parameter that states
that the feature cannot be enabled when using hugetlb-vmemmap
optimization.

Users can always check whether the feature is enabled via
/sys/modules/memory_hotplug/parameters/memmap_on_memory.

Also, I did not check if it is, but if not, the fact about hugetlb-vmemmmap vs
hotplug-vmemmap should also be called out in the hugetlb-vmemmap kernel
parameter.

Thanks
Muchun Song April 21, 2021, 3:41 a.m. UTC | #2
On Tue, Apr 20, 2021 at 6:35 PM Oscar Salvador <osalvador@suse.de> wrote:
>
> On Thu, Apr 15, 2021 at 04:40:04PM +0800, Muchun Song wrote:
> >  bool mhp_supports_memmap_on_memory(unsigned long size)
> >  {
> > +     bool supported;
> >       unsigned long nr_vmemmap_pages = size / PAGE_SIZE;
> >       unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page);
> >       unsigned long remaining_size = size - vmemmap_size;
> > @@ -1011,11 +1012,18 @@ 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 memmap_on_memory &&
> > -            IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
> > -            size == memory_block_size_bytes() &&
> > -            IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
> > -            IS_ALIGNED(remaining_size, pageblock_nr_pages << PAGE_SHIFT);
> > +     supported = memmap_on_memory &&
> > +                 IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
> > +                 size == memory_block_size_bytes() &&
> > +                 IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
> > +                 IS_ALIGNED(remaining_size, pageblock_nr_pages << PAGE_SHIFT);
> > +
> > +     if (supported && is_hugetlb_free_vmemmap_enabled()) {
> > +             pr_info("Cannot enable memory_hotplug.memmap_on_memory, it is not compatible with hugetlb_free_vmemmap\n");
> > +             supported = false;
> > +     }
>
> I would not print anything and rather have
>
> return memmap_on_memory &&
>        !is_hugetlb_free_vmemmap_enabled &&
>        IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
>        size == memory_block_size_bytes() &&
>        IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
>        IS_ALIGNED(remaining_size, pageblock_nr_pages << PAGE_SHIFT);

OK. Will do.

>
> Documentation/admin-guide/kernel-parameters.txt already provides an
> explanation on memory_hotplug.memmap_on_memory parameter that states
> that the feature cannot be enabled when using hugetlb-vmemmap
> optimization.
>
> Users can always check whether the feature is enabled via
> /sys/modules/memory_hotplug/parameters/memmap_on_memory.

If memory_hotplug.memmap_on_memory is enabled

    $ cat /sys/module/memory_hotplug/parameters/memmap_on_memory
    $ Y

If memory_hotplug.memmap_on_memory is disabled

    $ cat /sys/module/memory_hotplug/parameters/memmap_on_memory
    $ N

>
> Also, I did not check if it is, but if not, the fact about hugetlb-vmemmmap vs
> hotplug-vmemmap should also be called out in the hugetlb-vmemmap kernel
> parameter.

Make sense. I will update the doc.

Thanks.

>
> Thanks
>
> --
> Oscar Salvador
> SUSE L3
Oscar Salvador April 21, 2021, 7:33 a.m. UTC | #3
On Wed, Apr 21, 2021 at 11:41:24AM +0800, Muchun Song wrote:
> > Documentation/admin-guide/kernel-parameters.txt already provides an
> > explanation on memory_hotplug.memmap_on_memory parameter that states
> > that the feature cannot be enabled when using hugetlb-vmemmap
> > optimization.
> >
> > Users can always check whether the feature is enabled via
> > /sys/modules/memory_hotplug/parameters/memmap_on_memory.

Heh, I realized this is not completely true.
Users can check whether the feature is __enabled__ by checking the sys fs,
but although it is enabled, it might not be effective.

This might be due to a different number of reasons, vmemmap does not fully
span a PMD, the size we want to add spans more than a single memory block, etc.

That is what

"Note that even when enabled, there are a few cases where the feature is not
 effective."

is supposed to mean.

Anyway, I did not change my opionion on this.

Thanks
Muchun Song April 21, 2021, 9:06 a.m. UTC | #4
On Wed, Apr 21, 2021 at 3:33 PM Oscar Salvador <osalvador@suse.de> wrote:
>
> On Wed, Apr 21, 2021 at 11:41:24AM +0800, Muchun Song wrote:
> > > Documentation/admin-guide/kernel-parameters.txt already provides an
> > > explanation on memory_hotplug.memmap_on_memory parameter that states
> > > that the feature cannot be enabled when using hugetlb-vmemmap
> > > optimization.
> > >
> > > Users can always check whether the feature is enabled via
> > > /sys/modules/memory_hotplug/parameters/memmap_on_memory.
>
> Heh, I realized this is not completely true.
> Users can check whether the feature is __enabled__ by checking the sys fs,
> but although it is enabled, it might not be effective.

Right. I have done the test.

>
> This might be due to a different number of reasons, vmemmap does not fully
> span a PMD, the size we want to add spans more than a single memory block, etc.

Agree. Thanks for your explanations.

>
> That is what
>
> "Note that even when enabled, there are a few cases where the feature is not
>  effective."
>
> is supposed to mean.
>
> Anyway, I did not change my opionion on this.
>
> Thanks
>
> --
> Oscar Salvador
> SUSE L3
diff mbox series

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 9e655f5206ac..1f648b3e6120 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2893,6 +2893,10 @@ 
 			Note that even when enabled, there are a few cases where
 			the feature is not effective.
 
+			This is not compatible with hugetlb_free_vmemmap. If
+			both parameters are enabled, hugetlb_free_vmemmap takes
+			precedence over memory_hotplug.memmap_on_memory.
+
 	memtest=	[KNL,X86,ARM,PPC,RISCV] Enable memtest
 			Format: <integer>
 			default : 0 <disable>
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index 8cc195c4c861..0d7f595ee441 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -15,6 +15,7 @@ 
 #include <linux/acpi.h>
 #include <linux/memory.h>
 #include <linux/memory_hotplug.h>
+#include <linux/hugetlb.h>
 
 #include "internal.h"
 
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 68923c19bdea..c45ed6c0cd9f 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -981,6 +981,7 @@  static int online_memory_block(struct memory_block *mem, void *arg)
 
 bool mhp_supports_memmap_on_memory(unsigned long size)
 {
+	bool supported;
 	unsigned long nr_vmemmap_pages = size / PAGE_SIZE;
 	unsigned long vmemmap_size = nr_vmemmap_pages * sizeof(struct page);
 	unsigned long remaining_size = size - vmemmap_size;
@@ -1011,11 +1012,18 @@  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 memmap_on_memory &&
-	       IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
-	       size == memory_block_size_bytes() &&
-	       IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
-	       IS_ALIGNED(remaining_size, pageblock_nr_pages << PAGE_SHIFT);
+	supported = memmap_on_memory &&
+		    IS_ENABLED(CONFIG_MHP_MEMMAP_ON_MEMORY) &&
+		    size == memory_block_size_bytes() &&
+		    IS_ALIGNED(vmemmap_size, PMD_SIZE) &&
+		    IS_ALIGNED(remaining_size, pageblock_nr_pages << PAGE_SHIFT);
+
+	if (supported && is_hugetlb_free_vmemmap_enabled()) {
+		pr_info("Cannot enable memory_hotplug.memmap_on_memory, it is not compatible with hugetlb_free_vmemmap\n");
+		supported = false;
+	}
+
+	return supported;
 }
 
 /*