[PATCH v1] mm/memory_hotplug: drop "online" parameter from add_memory_resource()

[PATCH v1] mm/memory_hotplug: drop "online" parameter from add_memory_resource()

[David Hildenbrand]

User space should always be in charge of how to online memory and
if memory should be onlined automatically in the kernel. Let's drop the
parameter to overwrite this - XEN passes memhp_auto_online, just like
add_memory(), so we can directly use that instead internally.

Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Pavel Tatashin <pasha.tatashin@oracle.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: Arun KS <arunks@codeaurora.org>
Cc: Mathieu Malaterre <malat@debian.org>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 drivers/xen/balloon.c          | 2 +-
 include/linux/memory_hotplug.h | 2 +-
 mm/memory_hotplug.c            | 6 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index 12148289debd..81ba448166cd 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -397,7 +397,7 @@ static enum bp_state reserve_additional_memory(void)
 	mutex_unlock(&balloon_mutex);
 	/* add_memory_resource() requires the device_hotplug lock */
 	lock_device_hotplug();
-	rc = add_memory_resource(nid, resource, memhp_auto_online);
+	rc = add_memory_resource(nid, resource);
 	unlock_device_hotplug();
 	mutex_lock(&balloon_mutex);
 
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 84e9ae205930..ebc99f29aeae 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -326,7 +326,7 @@ extern int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,
 		void *arg, int (*func)(struct memory_block *, void *));
 extern int __add_memory(int nid, u64 start, u64 size);
 extern int add_memory(int nid, u64 start, u64 size);
-extern int add_memory_resource(int nid, struct resource *resource, bool online);
+extern int add_memory_resource(int nid, struct resource *resource);
 extern int arch_add_memory(int nid, u64 start, u64 size,
 		struct vmem_altmap *altmap, bool want_memblock);
 extern void move_pfn_range_to_zone(struct zone *zone, unsigned long start_pfn,
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 7b4317ae8318..7b64bbf645c3 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1096,7 +1096,7 @@ static int online_memory_block(struct memory_block *mem, void *arg)
  *
  * we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG
  */
-int __ref add_memory_resource(int nid, struct resource *res, bool online)
+int __ref add_memory_resource(int nid, struct resource *res)
 {
 	u64 start, size;
 	bool new_node = false;
@@ -1151,7 +1151,7 @@ int __ref add_memory_resource(int nid, struct resource *res, bool online)
 	mem_hotplug_done();
 
 	/* online pages if requested */
-	if (online)
+	if (memhp_auto_online)
 		walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1),
 				  NULL, online_memory_block);
 
@@ -1175,7 +1175,7 @@ int __ref __add_memory(int nid, u64 start, u64 size)
 	if (IS_ERR(res))
 		return PTR_ERR(res);
 
-	ret = add_memory_resource(nid, res, memhp_auto_online);
+	ret = add_memory_resource(nid, res);
 	if (ret < 0)
 		release_memory_resource(res);
 	return ret;
-- 
2.17.2

Re: [PATCH v1] mm/memory_hotplug: drop "online" parameter from add_memory_resource()

[Michal Hocko]

On Fri 23-11-18 13:37:40, David Hildenbrand wrote:
> User space should always be in charge of how to online memory and
> if memory should be onlined automatically in the kernel. Let's drop the
> parameter to overwrite this - XEN passes memhp_auto_online, just like
> add_memory(), so we can directly use that instead internally.

Heh, I wanted to get rid of memhp_auto_online so much and now we have it
in the core memory_hotplug. Not a win on my side I would say :/
On the other hand this can be seen as a cleanup because it removes that
ambiguity that some callers might be unaware of the memhp_auto_online
leading to a different behavior.

> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Pavel Tatashin <pasha.tatashin@oracle.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Arun KS <arunks@codeaurora.org>
> Cc: Mathieu Malaterre <malat@debian.org>
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: Michal Hocko <mhocko@suse.com>
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm/memory_hotplug: drop "online" parameter from add_memory_resource()

[David Hildenbrand]

On 23.11.18 13:54, Michal Hocko wrote:
> On Fri 23-11-18 13:37:40, David Hildenbrand wrote:
>> User space should always be in charge of how to online memory and
>> if memory should be onlined automatically in the kernel. Let's drop the
>> parameter to overwrite this - XEN passes memhp_auto_online, just like
>> add_memory(), so we can directly use that instead internally.
> 
> Heh, I wanted to get rid of memhp_auto_online so much and now we have it
> in the core memory_hotplug. Not a win on my side I would say :/

That is actually a good point: Can we remove memhp_auto_online or is it
already some sort of kernel ABI?

(as it is exported via /sys/devices/system/memory/auto_online_blocks)

> On the other hand this can be seen as a cleanup because it removes that
> ambiguity that some callers might be unaware of the memhp_auto_online
> leading to a different behavior.

I would say this patch is a step into the right direction - remove the
flag from the interfaces, then drop it (eventually, as stated not sure
if that train has left the station).

> 
>> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Cc: Juergen Gross <jgross@suse.com>
>> Cc: Stefano Stabellini <sstabellini@kernel.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Dan Williams <dan.j.williams@intel.com>
>> Cc: Oscar Salvador <osalvador@suse.de>
>> Cc: Pavel Tatashin <pasha.tatashin@oracle.com>
>> Cc: Michal Hocko <mhocko@suse.com>
>> Cc: David Hildenbrand <david@redhat.com>
>> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>> Cc: Arun KS <arunks@codeaurora.org>
>> Cc: Mathieu Malaterre <malat@debian.org>
>> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v1] mm/memory_hotplug: drop "online" parameter from add_memory_resource()

[Michal Hocko]

On Fri 23-11-18 13:58:16, David Hildenbrand wrote:
> On 23.11.18 13:54, Michal Hocko wrote:
> > On Fri 23-11-18 13:37:40, David Hildenbrand wrote:
> >> User space should always be in charge of how to online memory and
> >> if memory should be onlined automatically in the kernel. Let's drop the
> >> parameter to overwrite this - XEN passes memhp_auto_online, just like
> >> add_memory(), so we can directly use that instead internally.
> > 
> > Heh, I wanted to get rid of memhp_auto_online so much and now we have it
> > in the core memory_hotplug. Not a win on my side I would say :/
> 
> That is actually a good point: Can we remove memhp_auto_online or is it
> already some sort of kernel ABI?
> 
> (as it is exported via /sys/devices/system/memory/auto_online_blocks)

I have tried and there was a pushback [1]. That led to a rework of the
sysfs semantic of onlining btw. The biggest objection against removing was
that the userspace might be too slow to online memory and memmaps could
eat the available memory and trigger OOM. That is why I've started
working on the self hosted memmpas but failed to finish it. Fortunatelly
Oscar is brave enough to continue in that work.

[1] http://lkml.kernel.org/r/20170227092817.23571-1-mhocko@kernel.org
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v1] mm/memory_hotplug: drop "online" parameter from add_memory_resource()

[Oscar Salvador]

On Fri, 2018-11-23 at 13:37 +0100, David Hildenbrand wrote:
> Signed-off-by: David Hildenbrand <david@redhat.com>

Thanks ;-)

Reviewed-by: Oscar Salvador <osalvador@suse.de>

-- 
Oscar Salvador
SUSE L3

Re: [PATCH v1] mm/memory_hotplug: drop "online" parameter from add_memory_resource()

[David Hildenbrand]

On 23.11.18 14:05, Michal Hocko wrote:
> On Fri 23-11-18 13:58:16, David Hildenbrand wrote:
>> On 23.11.18 13:54, Michal Hocko wrote:
>>> On Fri 23-11-18 13:37:40, David Hildenbrand wrote:
>>>> User space should always be in charge of how to online memory and
>>>> if memory should be onlined automatically in the kernel. Let's drop the
>>>> parameter to overwrite this - XEN passes memhp_auto_online, just like
>>>> add_memory(), so we can directly use that instead internally.
>>>
>>> Heh, I wanted to get rid of memhp_auto_online so much and now we have it
>>> in the core memory_hotplug. Not a win on my side I would say :/
>>
>> That is actually a good point: Can we remove memhp_auto_online or is it
>> already some sort of kernel ABI?
>>
>> (as it is exported via /sys/devices/system/memory/auto_online_blocks)
> 
> I have tried and there was a pushback [1]. That led to a rework of the
> sysfs semantic of onlining btw. The biggest objection against removing was
> that the userspace might be too slow to online memory and memmaps could
> eat the available memory and trigger OOM. That is why I've started
> working on the self hosted memmpas but failed to finish it. Fortunatelly
> Oscar is brave enough to continue in that work.

Yes I saw that work :) . I wonder if it is really an issue or can we
worked around. At least for paravirtualized devices (a.k.a. balloon
devices) - even without the memmaps rework.

E.g. only add a new memory block in case the old one was onlined. And we
get that information via register_memory_notifier(). So we would always
only have one memory block "pending to be onlined".

That's at least my plan for virtio-mem (add one block at a time). The
problem is if some external entity triggers memory hotplug of actual
devices you cannot simply control. Like adding a bunch of ACPI DIMMs in
one shot without userspace being able to keep up.

But the memmaps thingy still is very valuable when wanting to add memory
in an environment where we are already low on memory.

> 
> [1] http://lkml.kernel.org/r/20170227092817.23571-1-mhocko@kernel.org
> 


-- 

Thanks,

David / dhildenb

Re: [PATCH v1] mm/memory_hotplug: drop "online" parameter from add_memory_resource()

[Juergen Gross]

On 23/11/2018 13:37, David Hildenbrand wrote:
> User space should always be in charge of how to online memory and
> if memory should be onlined automatically in the kernel. Let's drop the
> parameter to overwrite this - XEN passes memhp_auto_online, just like
> add_memory(), so we can directly use that instead internally.
> 
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: Pavel Tatashin <pasha.tatashin@oracle.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Arun KS <arunks@codeaurora.org>
> Cc: Mathieu Malaterre <malat@debian.org>
> Cc: Stephen Rothwell <sfr@canb.auug.org.au>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: Juergen Gross <jgross@suse.com>


Juergen