Re: [PATCH v3] memory-hotplug: add automatic onlining policy for the newly added memory

[Daniel Kiper]

On Fri, Jan 08, 2016 at 05:55:07PM +0100, Vitaly Kuznetsov wrote:
> Daniel Kiper <daniel.kiper@xxxxxxxxxx> writes:

[...]

> >> diff --git a/Documentation/memory-hotplug.txt b/Documentation/memory-hotplug.txt
> >> index ce2cfcf..ceaf40c 100644
> >> --- a/Documentation/memory-hotplug.txt
> >> +++ b/Documentation/memory-hotplug.txt
> >> @@ -254,12 +254,23 @@ If the memory block is online, you'll read "online".
> >>  If the memory block is offline, you'll read "offline".
> >>
> >>
> >> -5.2. How to online memory
> >> +5.2. Memory onlining
> >>  ------------
> >> -Even if the memory is hot-added, it is not at ready-to-use state.
> >> -For using newly added memory, you have to "online" the memory block.
> >> +When the memory is hot-added, the kernel decides whether or not to "online"
> >> +it according to the policy which can be read from "auto_online_blocks" file:
> >>
> >> -For onlining, you have to write "online" to the memory block's state file as:
> >> +% cat /sys/devices/system/memory/auto_online_blocks
> >> +
> >> +The default is "offline" which means the newly added memory is not in a
> >> +ready-to-use state and you have to "online" the newly added memory blocks
> >> +manually. Automatic onlining can be requested by writing "online" to
> >> +"auto_online_blocks" file:
> >> +
> >> +% echo online > /sys/devices/system/memory/auto_online_blocks
> >> +
> >> +If the automatic onlining wasn't requested or some memory block was offlined
> >> +it is possible to change the individual block's state by writing to the "state"
> >> +file:
> >>
> >>  % echo online > /sys/devices/system/memory/memoryXXX/state
> >
> > Please say clearly that offlined blocks are not onlined automatically
> > when /sys/devices/system/memory/auto_online_blocks is set to online.
> >
>
> You mean the blocks which were manually offlined won't magically come
> back, right? Ok, I'll try.

Yep, but AIUI it works in that way for all offlined blocks not only for
earlier manually offlined ones.

> >> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> >> index 25425d3..44a618d 100644
> >> --- a/drivers/base/memory.c
> >> +++ b/drivers/base/memory.c
> >> @@ -439,6 +439,37 @@ print_block_size(struct device *dev, struct device_attribute *attr,
> >>  static DEVICE_ATTR(block_size_bytes, 0444, print_block_size, NULL);
> >>
> >>  /*
> >> + * Memory auto online policy.
> >> + */
> >> +
> >> +static ssize_t
> >> +show_auto_online_blocks(struct device *dev, struct device_attribute *attr,
> >> +			char *buf)
> >> +{
> >> +	if (memhp_auto_online)
> >> +		return sprintf(buf, "online\n");
> >> +	else
> >> +		return sprintf(buf, "offline\n");
> >> +}
> >> +
> >> +static ssize_t
> >> +store_auto_online_blocks(struct device *dev, struct device_attribute *attr,
> >> +			 const char *buf, size_t count)
> >> +{
> >> +	if (sysfs_streq(buf, "online"))
> >> +		memhp_auto_online = true;
> >> +	else if (sysfs_streq(buf, "offline"))
> >> +		memhp_auto_online = false;
> >> +	else
> >> +		return -EINVAL;
> >> +
> >> +	return count;
> >> +}
> >> +
> >> +static DEVICE_ATTR(auto_online_blocks, 0644, show_auto_online_blocks,
> >> +		   store_auto_online_blocks);
> >> +
> >> +/*
> >>   * Some architectures will have custom drivers to do this, and
> >>   * will not need to do it from userspace.  The fake hot-add code
> >>   * as well as ppc64 will do all of their discovery in userspace
> >> @@ -737,6 +768,7 @@ static struct attribute *memory_root_attrs[] = {
> >>  #endif
> >>
> >>  	&dev_attr_block_size_bytes.attr,
> >> +	&dev_attr_auto_online_blocks.attr,
> >>  	NULL
> >>  };
> >>
> >> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> >> index 12eab50..890c3b5 100644
> >> --- a/drivers/xen/balloon.c
> >> +++ b/drivers/xen/balloon.c
> >> @@ -338,7 +338,7 @@ static enum bp_state reserve_additional_memory(void)
> >>  	}
> >>  #endif
> >>
> >> -	rc = add_memory_resource(nid, resource);
> >> +	rc = add_memory_resource(nid, resource, false);
> >
> > This is partial solution and does not allow us to use new feature in Xen.
> > Could you add separate patch which fixes this issue?
> >
>
> Sure, I'd be glad to make this work for Xen too.

Great! Thanks a lot!

> >>  	if (rc) {
> >>  		pr_warn("Cannot add additional memory (%i)\n", rc);
> >>  		goto err;
> >> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> >> index 2ea574f..4b7949a 100644
> >> --- a/include/linux/memory_hotplug.h
> >> +++ b/include/linux/memory_hotplug.h
> >> @@ -99,6 +99,8 @@ extern void __online_page_free(struct page *page);
> >>
> >>  extern int try_online_node(int nid);
> >>
> >> +extern bool memhp_auto_online;
> >> +
> >>  #ifdef CONFIG_MEMORY_HOTREMOVE
> >>  extern bool is_pageblock_removable_nolock(struct page *page);
> >>  extern int arch_remove_memory(u64 start, u64 size);
> >> @@ -267,7 +269,7 @@ static inline void remove_memory(int nid, u64 start, u64 size) {}
> >>  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_resource(int nid, struct resource *resource);
> >> +extern int add_memory_resource(int nid, struct resource *resource, bool online);
> >>  extern int zone_for_memory(int nid, u64 start, u64 size, int zone_default,
> >>  		bool for_device);
> >>  extern int arch_add_memory(int nid, u64 start, u64 size, bool for_device);
> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >> index a042a9d..0ecf860 100644
> >> --- a/mm/memory_hotplug.c
> >> +++ b/mm/memory_hotplug.c
> >> @@ -76,6 +76,9 @@ static struct {
> >>  #define memhp_lock_acquire()      lock_map_acquire(&mem_hotplug.dep_map)
> >>  #define memhp_lock_release()      lock_map_release(&mem_hotplug.dep_map)
> >>
> >> +bool memhp_auto_online;
> >> +EXPORT_SYMBOL_GPL(memhp_auto_online);
> >> +
> >>  void get_online_mems(void)
> >>  {
> >>  	might_sleep();
> >> @@ -1232,7 +1235,7 @@ int zone_for_memory(int nid, u64 start, u64 size, int zone_default,
> >>  }
> >>
> >>  /* we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG */
> >> -int __ref add_memory_resource(int nid, struct resource *res)
> >> +int __ref add_memory_resource(int nid, struct resource *res, bool online)
> >>  {
> >>  	u64 start, size;
> >>  	pg_data_t *pgdat = NULL;
> >> @@ -1292,6 +1295,11 @@ int __ref add_memory_resource(int nid, struct resource *res)
> >>  	/* create new memmap entry */
> >>  	firmware_map_add_hotplug(start, start + size, "System RAM");
> >>
> >> +	/* online pages if requested */
> >> +	if (online)
> >> +		online_pages(start >> PAGE_SHIFT, size >> PAGE_SHIFT,
> >> +			     MMOP_ONLINE_KEEP);
> >> +
> >
> > This way we go in deadlock if auto online feature is enabled in Xen (this was
> > pointed out by David Vrabel).
>
> Yes, but as I said the patch doesn't change anything for Xen guests for
> now, we always call add_memory_resource() with online = false.
>
> > And we want to have it working out of the box.
> > So, I think that we should find proper solution. I suppose that we can schedule
> > a task here which auto online attached blocks. Hmmm... Not nice but should work.
> > Or maybe you have better idea how to fix this issue.
>
> I'd like to avoid additional delays and memory allocations between
> adding new memory and onlining it (and this is the main purpose of the
> patch). Maybe we can have a tristate online parameter ('online_now',
> 'online_delay', 'keep_offlined') and handle it
> accordingly. Alternatively I can suggest we have the onlining in Xen
> balloon driver code, memhp_auto_online is exported so we can call
> online_pages() after we release the ballon_mutex.

This is not nice too. I prefer the same code path for every case.
Give me some time. I will think how to solve that issue.

Daniel