[PATCH 0/2] mm/migrate: 5.15 fixes for automatic demotion

[PATCH 0/2] mm/migrate: 5.15 fixes for automatic demotion

[Dave Hansen]

This contains two fixes for the "automatic demotion" code which was
merged into 5.15:

 * Fix memory hotplug performance regresssion by watching
   suppressing any real action on irrelevant hotplug events.
 * Ensure CPU hotplug handler is registered when memory hotplug
   is disabled.

Cc: "Huang, Ying" <ying.huang@intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Wei Xu <weixugc@google.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: David Rientjes <rientjes@google.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Greg Thelen <gthelen@google.com>
Cc: Yang Shi <yang.shi@linux.alibaba.com>
Cc: Andrew Morton <akpm@linux-foundation.org>

[PATCH 1/2] mm/migrate: optimize hotplug-time demotion order updates

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>
== tl;dr ==

Automatic demotion opted for a simple, lazy approach to handling
hotplug events.  This noticeably slows down memory hotplug[1].
Optimize away updates to the demotion order when CPU and memory
hotplug events should have no effect.

== Background ==

Automatic demotion is a memory migration strategy to ensure that
new allocations have room in faster memory tiers on tiered memory
systems.  The kernel maintains an array (node_demotion[]) to
drive these migrations.

The node_demotion[] path is calculated by starting at nodes with
CPUs and then "walking" to nodes with memory.  Only hotplug
events which online or offline a node with memory (N_ONLINE) or
CPUs (N_CPU) will actually affect the migration order.

== Problem ==

However, the current code is lazy.  It completely regenerates the
migration order on *any* CPU or memory hotplug event.  The logic
was that these events are extremely rare and that the overhead
from indiscriminate order regeneration is minimal.

Part of the update logic involves a synchronize_rcu(), which is a
pretty big hammer.  Its overhead was large enough to be detected
by some 0day tests that watch memory hotplug performance[1].

== Solution ==

Add a new helper (node_demotion_topo_changed()) which can
differentiate between superfluous and impactful hotplug events.
Skip the expensive update operation for superfluous events.

== Aside: Locking ==

It took me a few moments to declare the locking to be safe enough
for node_demotion_topo_changed() to work.  It all hinges on the
memory hotplug lock:

During memory hotplug events, 'mem_hotplug_lock' is held for
write.  This ensures that two memory hotplug events can not be
called simultaneously.

CPU hotplug has a similar lock (cpuhp_state_mutex) which also
provides mutual exclusion between CPU hotplug events.  In
addition, the demotion code acquire and hold the mem_hotplug_lock
for read during its CPU hotplug handlers.  This provides mutual
exclusion between the demotion memory hotplug callbacks and the
CPU hotplug callbacks.

This effectively allows treating the migration target generation
code to act as if it is single-threaded.

1. https://lore.kernel.org/all/20210905135932.GE15026@xsang-OptiPlex-9020/

Fixes: 884a6e5d1f93 ("mm/migrate: update node demotion order on hotplug events")
Reported-by: kernel test robot <oliver.sang@intel.com>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "Huang, Ying" <ying.huang@intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Wei Xu <weixugc@google.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: David Rientjes <rientjes@google.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Greg Thelen <gthelen@google.com>
Cc: Yang Shi <yang.shi@linux.alibaba.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---

 b/mm/migrate.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff -puN mm/migrate.c~faster-node-order mm/migrate.c
--- a/mm/migrate.c~faster-node-order	2021-09-14 11:05:04.998951737 -0700
+++ b/mm/migrate.c	2021-09-14 11:05:05.002951737 -0700
@@ -3122,6 +3122,36 @@ static int establish_migrate_target(int
 }
 
 /*
+ * The node_demotion[] path is calculated by starting at
+ * nodes with CPUs and then "walking" to nodes with memory.
+ * Only hotplug events which online or offline a node with
+ * memory (N_ONLINE) or CPUs (N_CPU) will actually affect
+ * the migration order.
+ *
+ * Differentiate between hotplug events which are impactful
+ * or superfluous to node_demotion[].
+ *
+ * Must only be called once per hotplug event.  Callers
+ * must not make concurrent calls.
+ */
+static bool node_demotion_topo_changed(void)
+{
+	static int prev_topo_cpus = -1;
+	static int prev_topo_mems = -1;
+	int now_topo_cpus = num_node_state(N_CPU);
+	int now_topo_mems = num_node_state(N_ONLINE);
+
+	if ((now_topo_cpus == prev_topo_cpus) &&
+	    (now_topo_mems == prev_topo_mems))
+	   return false;
+
+	prev_topo_cpus = now_topo_cpus;
+	prev_topo_mems = now_topo_mems;
+
+	return true;
+}
+
+/*
  * When memory fills up on a node, memory contents can be
  * automatically migrated to another node instead of
  * discarded at reclaim.
@@ -3147,6 +3177,16 @@ static void __set_migration_target_nodes
 	int node;
 
 	/*
+	 * The "migration path" array is heavily optimized
+	 * for reads.  This is the write side which incurs a
+	 * very heavy synchronize_rcu().  Avoid this overhead
+	 * when nothing of consequence has changed since the
+	 * last write.
+	 */
+	if (!node_demotion_topo_changed())
+		return;
+
+	/*
 	 * Avoid any oddities like cycles that could occur
 	 * from changes in the topology.  This will leave
 	 * a momentary gap when migration is disabled.
_

[PATCH 2/2] mm/migrate: add CPU hotplug to demotion #ifdef

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>

Once upon a time, the node demotion updates were driven solely by
memory hotplug events.  But now, there are  handlers for both CPU
and memory hotplug.

However, the #ifdef around the code checks only memory hotplug.
A system that has HOTPLUG_CPU=y but MEMORY_HOTPLUG=n would miss
CPU hotplug events.

Update the #ifdef around the common code.  Add memory and
CPU-specific #ifdefs for their handlers.  These memory/CPU
#ifdefs avoid unused function warnings when their Kconfig option
is off.

Fixes: 884a6e5d1f93 ("mm/migrate: update node demotion order on hotplug events")
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "Huang, Ying" <ying.huang@intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Wei Xu <weixugc@google.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: David Rientjes <rientjes@google.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Greg Thelen <gthelen@google.com>
Cc: Yang Shi <yang.shi@linux.alibaba.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---

 b/mm/migrate.c |   46 +++++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff -puN mm/migrate.c~add-cpu-hotplug-config mm/migrate.c
--- a/mm/migrate.c~add-cpu-hotplug-config	2021-09-16 16:14:01.770140593 -0700
+++ b/mm/migrate.c	2021-09-17 11:30:19.197027668 -0700
@@ -3066,7 +3066,7 @@ void migrate_vma_finalize(struct migrate
 EXPORT_SYMBOL(migrate_vma_finalize);
 #endif /* CONFIG_DEVICE_PRIVATE */
 
-#if defined(CONFIG_MEMORY_HOTPLUG)
+#if defined(CONFIG_MEMORY_HOTPLUG) || defined(CONFIG_HOTPLUG_CPU)
 /* Disable reclaim-based migration. */
 static void __disable_all_migrate_targets(void)
 {
@@ -3248,25 +3248,7 @@ static void set_migration_target_nodes(v
 	put_online_mems();
 }
 
-/*
- * React to hotplug events that might affect the migration targets
- * like events that online or offline NUMA nodes.
- *
- * The ordering is also currently dependent on which nodes have
- * CPUs.  That means we need CPU on/offline notification too.
- */
-static int migration_online_cpu(unsigned int cpu)
-{
-	set_migration_target_nodes();
-	return 0;
-}
-
-static int migration_offline_cpu(unsigned int cpu)
-{
-	set_migration_target_nodes();
-	return 0;
-}
-
+#if defined(CONFIG_MEMORY_HOTPLUG)
 /*
  * This leaves migrate-on-reclaim transiently disabled between
  * the MEM_GOING_OFFLINE and MEM_OFFLINE events.  This runs
@@ -3313,6 +3295,27 @@ static int __meminit migrate_on_reclaim_
 
 	return notifier_from_errno(0);
 }
+#endif /* CONFIG_MEMORY_HOTPLUG */
+
+#ifdef CONFIG_HOTPLUG_CPU
+/*
+ * React to hotplug events that might affect the migration targets
+ * like events that online or offline NUMA nodes.
+ *
+ * The ordering is also currently dependent on which nodes have
+ * CPUs.  That means we need CPU on/offline notification too.
+ */
+static int migration_online_cpu(unsigned int cpu)
+{
+	set_migration_target_nodes();
+	return 0;
+}
+
+static int migration_offline_cpu(unsigned int cpu)
+{
+	set_migration_target_nodes();
+	return 0;
+}
 
 static int __init migrate_on_reclaim_init(void)
 {
@@ -3333,4 +3336,5 @@ static int __init migrate_on_reclaim_ini
 	return 0;
 }
 late_initcall(migrate_on_reclaim_init);
-#endif /* CONFIG_MEMORY_HOTPLUG */
+#endif /* CONFIG_HOTPLUG_CPU */
+#endif /* CONFIG_MEMORY_HOTPLUG || CONFIG_HOTPLUG_CPU */
_

Re: [PATCH 2/2] mm/migrate: add CPU hotplug to demotion #ifdef

[Wei Xu]

The initialization of node_demotion doesn't have to depend on
CONFIG_MEMORY_HOTPLUG and CONFIG_HOTPLUG_CPU.  While you are at this,
can you replace cpuhp_setup_state() with cpuhp_setup_state_nocalls()
and also call set_migration_target_nodes() directly in
migrate_on_reclaim_init() outside
CONFIG_MEMORY_HOTPLUG/CONFIG_HOTPLUG_CPU?  Thanks.

Wei

On Fri, Sep 17, 2021 at 3:35 PM Dave Hansen <dave.hansen@linux.intel.com> wrote:
>
>
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> Once upon a time, the node demotion updates were driven solely by
> memory hotplug events.  But now, there are  handlers for both CPU
> and memory hotplug.
>
> However, the #ifdef around the code checks only memory hotplug.
> A system that has HOTPLUG_CPU=y but MEMORY_HOTPLUG=n would miss
> CPU hotplug events.
>
> Update the #ifdef around the common code.  Add memory and
> CPU-specific #ifdefs for their handlers.  These memory/CPU
> #ifdefs avoid unused function warnings when their Kconfig option
> is off.
>
> Fixes: 884a6e5d1f93 ("mm/migrate: update node demotion order on hotplug events")
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "Huang, Ying" <ying.huang@intel.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Wei Xu <weixugc@google.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Greg Thelen <gthelen@google.com>
> Cc: Yang Shi <yang.shi@linux.alibaba.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>
>  b/mm/migrate.c |   46 +++++++++++++++++++++++++---------------------
>  1 file changed, 25 insertions(+), 21 deletions(-)
>
> diff -puN mm/migrate.c~add-cpu-hotplug-config mm/migrate.c
> --- a/mm/migrate.c~add-cpu-hotplug-config       2021-09-16 16:14:01.770140593 -0700
> +++ b/mm/migrate.c      2021-09-17 11:30:19.197027668 -0700
> @@ -3066,7 +3066,7 @@ void migrate_vma_finalize(struct migrate
>  EXPORT_SYMBOL(migrate_vma_finalize);
>  #endif /* CONFIG_DEVICE_PRIVATE */
>
> -#if defined(CONFIG_MEMORY_HOTPLUG)
> +#if defined(CONFIG_MEMORY_HOTPLUG) || defined(CONFIG_HOTPLUG_CPU)
>  /* Disable reclaim-based migration. */
>  static void __disable_all_migrate_targets(void)
>  {
> @@ -3248,25 +3248,7 @@ static void set_migration_target_nodes(v
>         put_online_mems();
>  }
>
> -/*
> - * React to hotplug events that might affect the migration targets
> - * like events that online or offline NUMA nodes.
> - *
> - * The ordering is also currently dependent on which nodes have
> - * CPUs.  That means we need CPU on/offline notification too.
> - */
> -static int migration_online_cpu(unsigned int cpu)
> -{
> -       set_migration_target_nodes();
> -       return 0;
> -}
> -
> -static int migration_offline_cpu(unsigned int cpu)
> -{
> -       set_migration_target_nodes();
> -       return 0;
> -}
> -
> +#if defined(CONFIG_MEMORY_HOTPLUG)
>  /*
>   * This leaves migrate-on-reclaim transiently disabled between
>   * the MEM_GOING_OFFLINE and MEM_OFFLINE events.  This runs
> @@ -3313,6 +3295,27 @@ static int __meminit migrate_on_reclaim_
>
>         return notifier_from_errno(0);
>  }
> +#endif /* CONFIG_MEMORY_HOTPLUG */
> +
> +#ifdef CONFIG_HOTPLUG_CPU
> +/*
> + * React to hotplug events that might affect the migration targets
> + * like events that online or offline NUMA nodes.
> + *
> + * The ordering is also currently dependent on which nodes have
> + * CPUs.  That means we need CPU on/offline notification too.
> + */
> +static int migration_online_cpu(unsigned int cpu)
> +{
> +       set_migration_target_nodes();
> +       return 0;
> +}
> +
> +static int migration_offline_cpu(unsigned int cpu)
> +{
> +       set_migration_target_nodes();
> +       return 0;
> +}
>
>  static int __init migrate_on_reclaim_init(void)
>  {
> @@ -3333,4 +3336,5 @@ static int __init migrate_on_reclaim_ini
>         return 0;
>  }
>  late_initcall(migrate_on_reclaim_init);
> -#endif /* CONFIG_MEMORY_HOTPLUG */
> +#endif /* CONFIG_HOTPLUG_CPU */
> +#endif /* CONFIG_MEMORY_HOTPLUG || CONFIG_HOTPLUG_CPU */
> _

Re: [PATCH 1/2] mm/migrate: optimize hotplug-time demotion order updates

[Huang, Ying]

Dave Hansen <dave.hansen@linux.intel.com> writes:

> From: Dave Hansen <dave.hansen@linux.intel.com>
> == tl;dr ==
>
> Automatic demotion opted for a simple, lazy approach to handling
> hotplug events.  This noticeably slows down memory hotplug[1].
> Optimize away updates to the demotion order when CPU and memory
> hotplug events should have no effect.
>
> == Background ==
>
> Automatic demotion is a memory migration strategy to ensure that
> new allocations have room in faster memory tiers on tiered memory
> systems.  The kernel maintains an array (node_demotion[]) to
> drive these migrations.
>
> The node_demotion[] path is calculated by starting at nodes with
> CPUs and then "walking" to nodes with memory.  Only hotplug
> events which online or offline a node with memory (N_ONLINE) or
> CPUs (N_CPU) will actually affect the migration order.
>
> == Problem ==
>
> However, the current code is lazy.  It completely regenerates the
> migration order on *any* CPU or memory hotplug event.  The logic
> was that these events are extremely rare and that the overhead
> from indiscriminate order regeneration is minimal.
>
> Part of the update logic involves a synchronize_rcu(), which is a
> pretty big hammer.  Its overhead was large enough to be detected
> by some 0day tests that watch memory hotplug performance[1].
>
> == Solution ==
>
> Add a new helper (node_demotion_topo_changed()) which can
> differentiate between superfluous and impactful hotplug events.
> Skip the expensive update operation for superfluous events.
>
> == Aside: Locking ==
>
> It took me a few moments to declare the locking to be safe enough
> for node_demotion_topo_changed() to work.  It all hinges on the
> memory hotplug lock:
>
> During memory hotplug events, 'mem_hotplug_lock' is held for
> write.  This ensures that two memory hotplug events can not be
> called simultaneously.
>
> CPU hotplug has a similar lock (cpuhp_state_mutex) which also
> provides mutual exclusion between CPU hotplug events.  In
> addition, the demotion code acquire and hold the mem_hotplug_lock
> for read during its CPU hotplug handlers.  This provides mutual
> exclusion between the demotion memory hotplug callbacks and the
> CPU hotplug callbacks.
>
> This effectively allows treating the migration target generation
> code to act as if it is single-threaded.
>
> 1. https://lore.kernel.org/all/20210905135932.GE15026@xsang-OptiPlex-9020/
>
> Fixes: 884a6e5d1f93 ("mm/migrate: update node demotion order on hotplug events")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "Huang, Ying" <ying.huang@intel.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Wei Xu <weixugc@google.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Greg Thelen <gthelen@google.com>
> Cc: Yang Shi <yang.shi@linux.alibaba.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
>
>  b/mm/migrate.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff -puN mm/migrate.c~faster-node-order mm/migrate.c
> --- a/mm/migrate.c~faster-node-order	2021-09-14 11:05:04.998951737 -0700
> +++ b/mm/migrate.c	2021-09-14 11:05:05.002951737 -0700
> @@ -3122,6 +3122,36 @@ static int establish_migrate_target(int
>  }
>  
>  /*
> + * The node_demotion[] path is calculated by starting at
> + * nodes with CPUs and then "walking" to nodes with memory.
> + * Only hotplug events which online or offline a node with
> + * memory (N_ONLINE) or CPUs (N_CPU) will actually affect
> + * the migration order.
> + *
> + * Differentiate between hotplug events which are impactful
> + * or superfluous to node_demotion[].
> + *
> + * Must only be called once per hotplug event.  Callers
> + * must not make concurrent calls.
> + */
> +static bool node_demotion_topo_changed(void)
> +{
> +	static int prev_topo_cpus = -1;
> +	static int prev_topo_mems = -1;
> +	int now_topo_cpus = num_node_state(N_CPU);
> +	int now_topo_mems = num_node_state(N_ONLINE);
> +
> +	if ((now_topo_cpus == prev_topo_cpus) &&
> +	    (now_topo_mems == prev_topo_mems))
> +	   return false;
> +
> +	prev_topo_cpus = now_topo_cpus;
> +	prev_topo_mems = now_topo_mems;
> +
> +	return true;
> +}
> +
> +/*
>   * When memory fills up on a node, memory contents can be
>   * automatically migrated to another node instead of
>   * discarded at reclaim.
> @@ -3147,6 +3177,16 @@ static void __set_migration_target_nodes
>  	int node;
>  
>  	/*
> +	 * The "migration path" array is heavily optimized
> +	 * for reads.  This is the write side which incurs a
> +	 * very heavy synchronize_rcu().  Avoid this overhead
> +	 * when nothing of consequence has changed since the
> +	 * last write.
> +	 */
> +	if (!node_demotion_topo_changed())
> +		return;
> +
> +	/*
>  	 * Avoid any oddities like cycles that could occur
>  	 * from changes in the topology.  This will leave
>  	 * a momentary gap when migration is disabled.

Now synchronize_rcu() is called in disable_all_migrate_targets(), which
is called for MEM_GOING_OFFLINE.  Can we remove the synchronize_rcu()
from disable_all_migrate_targets() and call it in
__set_migration_target_nodes() before we update the node_demotion[]?

Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm/migrate: optimize hotplug-time demotion order updates

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

On 9/17/21 5:55 PM, Huang, Ying wrote:
>> @@ -3147,6 +3177,16 @@ static void __set_migration_target_nodes
>>  	int node;
>>  
>>  	/*
>> +	 * The "migration path" array is heavily optimized
>> +	 * for reads.  This is the write side which incurs a
>> +	 * very heavy synchronize_rcu().  Avoid this overhead
>> +	 * when nothing of consequence has changed since the
>> +	 * last write.
>> +	 */
>> +	if (!node_demotion_topo_changed())
>> +		return;
>> +
>> +	/*
>>  	 * Avoid any oddities like cycles that could occur
>>  	 * from changes in the topology.  This will leave
>>  	 * a momentary gap when migration is disabled.
> Now synchronize_rcu() is called in disable_all_migrate_targets(), which
> is called for MEM_GOING_OFFLINE.  Can we remove the synchronize_rcu()
> from disable_all_migrate_targets() and call it in
> __set_migration_target_nodes() before we update the node_demotion[]?

I see what you are saying.  This patch just targeted
__set_migration_target_nodes() which is called in for
MEM_ONLINE/OFFLINE.  But, it missed MEM_GOING_OFFLINE's call to
disable_all_migrate_targets().

I think I found something better than what I had in this patch, or the
tweak you suggested: The 'memory_notify->status_change_nid' field is
passed to all memory hotplug notifiers and tells us whether the node is
going online/offline.  Instead of trying to track the changes, I think
we can simply rely on it to tell us when a node is going online/offline.

This removes the need for the demotion code to track *any* state.  I've
attached a totally untested patch to do this.

[-- Attachment #2: faster-node-order.patch --]
[-- Type: text/x-patch, Size: 815 bytes --]

diff -puN mm/migrate.c~faster-node-order mm/migrate.c
--- a/mm/migrate.c~faster-node-order	2021-09-17 14:44:58.697476940 -0700
+++ b/mm/migrate.c	2021-09-20 14:31:43.570477095 -0700
@@ -3239,8 +3239,18 @@ static int migration_offline_cpu(unsigne
  * set_migration_target_nodes().
  */
 static int __meminit migrate_on_reclaim_callback(struct notifier_block *self,
-						 unsigned long action, void *arg)
+						 unsigned long action, void *_arg)
 {
+	struct memory_notify *arg = _arg;
+
+	/*
+	 * Only update the node migration order when a node is
+	 * changing status, like online->offline.  This avoids
+	 * the overhead of synchronize_rcu() in most cases.
+	 */
+	if (arg->status_change_nid < 0)
+		return notifier_from_errno(0);
+
 	switch (action) {
 	case MEM_GOING_OFFLINE:
 		/*
_

Re: [PATCH 1/2] mm/migrate: optimize hotplug-time demotion order updates

[David Hildenbrand]

On 20.09.21 23:37, Dave Hansen wrote:
> On 9/17/21 5:55 PM, Huang, Ying wrote:
>>> @@ -3147,6 +3177,16 @@ static void __set_migration_target_nodes
>>>   	int node;
>>>   
>>>   	/*
>>> +	 * The "migration path" array is heavily optimized
>>> +	 * for reads.  This is the write side which incurs a
>>> +	 * very heavy synchronize_rcu().  Avoid this overhead
>>> +	 * when nothing of consequence has changed since the
>>> +	 * last write.
>>> +	 */
>>> +	if (!node_demotion_topo_changed())
>>> +		return;
>>> +
>>> +	/*
>>>   	 * Avoid any oddities like cycles that could occur
>>>   	 * from changes in the topology.  This will leave
>>>   	 * a momentary gap when migration is disabled.
>> Now synchronize_rcu() is called in disable_all_migrate_targets(), which
>> is called for MEM_GOING_OFFLINE.  Can we remove the synchronize_rcu()
>> from disable_all_migrate_targets() and call it in
>> __set_migration_target_nodes() before we update the node_demotion[]?
> 
> I see what you are saying.  This patch just targeted
> __set_migration_target_nodes() which is called in for
> MEM_ONLINE/OFFLINE.  But, it missed MEM_GOING_OFFLINE's call to
> disable_all_migrate_targets().
> 
> I think I found something better than what I had in this patch, or the
> tweak you suggested: The 'memory_notify->status_change_nid' field is
> passed to all memory hotplug notifiers and tells us whether the node is
> going online/offline.  Instead of trying to track the changes, I think
> we can simply rely on it to tell us when a node is going online/offline.
> 
> This removes the need for the demotion code to track *any* state.  I've
> attached a totally untested patch to do this.
> 

Sounds sane to me (although I really detest that status_change_nid... 
interface).

I was just about to ask "but how does this interact with !CONFIG_NUMA" 
... until I realized that having a single node go completely offline is 
rather unrealistic ;)

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/2] mm/migrate: optimize hotplug-time demotion order updates

[Huang, Ying]

Dave Hansen <dave.hansen@intel.com> writes:

> On 9/17/21 5:55 PM, Huang, Ying wrote:
>>> @@ -3147,6 +3177,16 @@ static void __set_migration_target_nodes
>>>  	int node;
>>>  
>>>  	/*
>>> +	 * The "migration path" array is heavily optimized
>>> +	 * for reads.  This is the write side which incurs a
>>> +	 * very heavy synchronize_rcu().  Avoid this overhead
>>> +	 * when nothing of consequence has changed since the
>>> +	 * last write.
>>> +	 */
>>> +	if (!node_demotion_topo_changed())
>>> +		return;
>>> +
>>> +	/*
>>>  	 * Avoid any oddities like cycles that could occur
>>>  	 * from changes in the topology.  This will leave
>>>  	 * a momentary gap when migration is disabled.
>> Now synchronize_rcu() is called in disable_all_migrate_targets(), which
>> is called for MEM_GOING_OFFLINE.  Can we remove the synchronize_rcu()
>> from disable_all_migrate_targets() and call it in
>> __set_migration_target_nodes() before we update the node_demotion[]?
>
> I see what you are saying.  This patch just targeted
> __set_migration_target_nodes() which is called in for
> MEM_ONLINE/OFFLINE.  But, it missed MEM_GOING_OFFLINE's call to
> disable_all_migrate_targets().
>
> I think I found something better than what I had in this patch, or the
> tweak you suggested: The 'memory_notify->status_change_nid' field is
> passed to all memory hotplug notifiers and tells us whether the node is
> going online/offline.  Instead of trying to track the changes, I think
> we can simply rely on it to tell us when a node is going online/offline.
>
> This removes the need for the demotion code to track *any* state.  I've
> attached a totally untested patch to do this.

Yes.  This sounds good.  I will try to test this patch on my side.

From another point of view, we still need to update demotion order upon
CPU hotplug too, because whether a node has CPU may be changed there.
And we need a solution for that too.

Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm/migrate: optimize hotplug-time demotion order updates

[Dave Hansen]

On 9/21/21 7:36 AM, Huang, Ying wrote:
>> This removes the need for the demotion code to track *any* state.  I've
>> attached a totally untested patch to do this.
> Yes.  This sounds good.  I will try to test this patch on my side.
> 
>>From another point of view, we still need to update demotion order upon
> CPU hotplug too, because whether a node has CPU may be changed there.
> And we need a solution for that too.

Just to recap...  The reason I sent this series is that there's a known,
detectable regression in a memory hotplug "benchmark".  This affects the
5.15 series.

While I agree that we should look into the impact on CPU hotplug, I
think we should probably focus on the *known* memory hotplug issue for 5.15.

Re: [PATCH 1/2] mm/migrate: optimize hotplug-time demotion order updates

[Huang, Ying]

Dave Hansen <dave.hansen@intel.com> writes:

> On 9/21/21 7:36 AM, Huang, Ying wrote:
>>> This removes the need for the demotion code to track *any* state.  I've
>>> attached a totally untested patch to do this.
>> Yes.  This sounds good.  I will try to test this patch on my side.
>> 
>>>From another point of view, we still need to update demotion order upon
>> CPU hotplug too, because whether a node has CPU may be changed there.
>> And we need a solution for that too.
>
> Just to recap...  The reason I sent this series is that there's a known,
> detectable regression in a memory hotplug "benchmark".  This affects the
> 5.15 series.
>
> While I agree that we should look into the impact on CPU hotplug, I
> think we should probably focus on the *known* memory hotplug issue for 5.15.

Yes.  We got a regression report about memory hotplug.  And that
reminded me that CPU hotplug may be a problem too.  Because CPU hotplug
is used during suspend/resume for every laptop.  The latency of
suspend/resume may impact the user experience of the Linux laptop users.

And, it seems that your previous solution can deal with CPU hotplug
too.   So, can we keep that too for CPU hotplug?

Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm/migrate: optimize hotplug-time demotion order updates

[Huang, Ying]

"Huang, Ying" <ying.huang@intel.com> writes:

> Dave Hansen <dave.hansen@intel.com> writes:
>
>> On 9/17/21 5:55 PM, Huang, Ying wrote:
>>>> @@ -3147,6 +3177,16 @@ static void __set_migration_target_nodes
>>>>  	int node;
>>>>  
>>>>  	/*
>>>> +	 * The "migration path" array is heavily optimized
>>>> +	 * for reads.  This is the write side which incurs a
>>>> +	 * very heavy synchronize_rcu().  Avoid this overhead
>>>> +	 * when nothing of consequence has changed since the
>>>> +	 * last write.
>>>> +	 */
>>>> +	if (!node_demotion_topo_changed())
>>>> +		return;
>>>> +
>>>> +	/*
>>>>  	 * Avoid any oddities like cycles that could occur
>>>>  	 * from changes in the topology.  This will leave
>>>>  	 * a momentary gap when migration is disabled.
>>> Now synchronize_rcu() is called in disable_all_migrate_targets(), which
>>> is called for MEM_GOING_OFFLINE.  Can we remove the synchronize_rcu()
>>> from disable_all_migrate_targets() and call it in
>>> __set_migration_target_nodes() before we update the node_demotion[]?
>>
>> I see what you are saying.  This patch just targeted
>> __set_migration_target_nodes() which is called in for
>> MEM_ONLINE/OFFLINE.  But, it missed MEM_GOING_OFFLINE's call to
>> disable_all_migrate_targets().
>>
>> I think I found something better than what I had in this patch, or the
>> tweak you suggested: The 'memory_notify->status_change_nid' field is
>> passed to all memory hotplug notifiers and tells us whether the node is
>> going online/offline.  Instead of trying to track the changes, I think
>> we can simply rely on it to tell us when a node is going online/offline.
>>
>> This removes the need for the demotion code to track *any* state.  I've
>> attached a totally untested patch to do this.
>
> Yes.  This sounds good.  I will try to test this patch on my side.

I have tested this patch, it works as expected for memory hotplug.  I
have asked 0-Day guys to test the original test case, but 0-Day doesn't
work very well for now, we need to wait for a while for 0-Day test
result.

Best Regards,
Huang, Ying

Re: [PATCH 1/2] mm/migrate: optimize hotplug-time demotion order updates

[David Hildenbrand]

On 24.09.21 18:12, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> == tl;dr ==
> 
> Automatic demotion opted for a simple, lazy approach to handling
> hotplug events.  This noticeably slows down memory hotplug[1].
> Optimize away updates to the demotion order when memory hotplug
> events should have no effect.
> 
> This has no effect on CPU hotplug.  There is no known problem on
> the CPU side and any work there will be in a separate series.
> 
> == Background ==
> 
> Automatic demotion is a memory migration strategy to ensure that
> new allocations have room in faster memory tiers on tiered memory
> systems.  The kernel maintains an array (node_demotion[]) to
> drive these migrations.
> 
> The node_demotion[] path is calculated by starting at nodes with
> CPUs and then "walking" to nodes with memory.  Only hotplug
> events which online or offline a node with memory (N_ONLINE) or
> CPUs (N_CPU) will actually affect the migration order.
> 
> == Problem ==
> 
> However, the current code is lazy.  It completely regenerates the
> migration order on *any* CPU or memory hotplug event.  The logic
> was that these events are extremely rare and that the overhead
> from indiscriminate order regeneration is minimal.
> 
> Part of the update logic involves a synchronize_rcu(), which is a
> pretty big hammer.  Its overhead was large enough to be detected
> by some 0day tests that watch memory hotplug performance[1].
> 
> == Solution ==
> 
> Add a new helper (node_demotion_topo_changed()) which can
> differentiate between superfluous and impactful hotplug events.
> Skip the expensive update operation for superfluous events.
> 
> == Aside: Locking ==
> 
> It took me a few moments to declare the locking to be safe enough
> for node_demotion_topo_changed() to work.  It all hinges on the
> memory hotplug lock:
> 
> During memory hotplug events, 'mem_hotplug_lock' is held for
> write.  This ensures that two memory hotplug events can not be
> called simultaneously.
> 
> CPU hotplug has a similar lock (cpuhp_state_mutex) which also
> provides mutual exclusion between CPU hotplug events.  In
> addition, the demotion code acquire and hold the mem_hotplug_lock
> for read during its CPU hotplug handlers.  This provides mutual
> exclusion between the demotion memory hotplug callbacks and the
> CPU hotplug callbacks.
> 
> This effectively allows treating the migration target generation
> code to act as if it is single-threaded.
> 
> 1. https://lore.kernel.org/all/20210905135932.GE15026@xsang-OptiPlex-9020/
> 
> Fixes: 884a6e5d1f93 ("mm/migrate: update node demotion order on hotplug events")
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "Huang, Ying" <ying.huang@intel.com>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Wei Xu <weixugc@google.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: David Hildenbrand <david@redhat.com>
> Cc: Greg Thelen <gthelen@google.com>
> Cc: Yang Shi <yang.shi@linux.alibaba.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>   b/mm/migrate.c |   12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff -puN mm/migrate.c~faster-node-order mm/migrate.c
> --- a/mm/migrate.c~faster-node-order	2021-09-24 09:12:30.988377798 -0700
> +++ b/mm/migrate.c	2021-09-24 09:12:30.988377798 -0700
> @@ -3239,8 +3239,18 @@ static int migration_offline_cpu(unsigne
>    * set_migration_target_nodes().
>    */
>   static int __meminit migrate_on_reclaim_callback(struct notifier_block *self,
> -						 unsigned long action, void *arg)
> +						 unsigned long action, void *_arg)
>   {
> +	struct memory_notify *arg = _arg;
> +
> +	/*
> +	 * Only update the node migration order when a node is
> +	 * changing status, like online->offline.  This avoids
> +	 * the overhead of synchronize_rcu() in most cases.
> +	 */
> +	if (arg->status_change_nid < 0)
> +		return notifier_from_errno(0);
> +
>   	switch (action) {
>   	case MEM_GOING_OFFLINE:
>   		/*
> _
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

[PATCH 1/2] mm/migrate: optimize hotplug-time demotion order updates

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>
== tl;dr ==

Automatic demotion opted for a simple, lazy approach to handling
hotplug events.  This noticeably slows down memory hotplug[1].
Optimize away updates to the demotion order when memory hotplug
events should have no effect.

This has no effect on CPU hotplug.  There is no known problem on
the CPU side and any work there will be in a separate series.

== Background ==

Automatic demotion is a memory migration strategy to ensure that
new allocations have room in faster memory tiers on tiered memory
systems.  The kernel maintains an array (node_demotion[]) to
drive these migrations.

The node_demotion[] path is calculated by starting at nodes with
CPUs and then "walking" to nodes with memory.  Only hotplug
events which online or offline a node with memory (N_ONLINE) or
CPUs (N_CPU) will actually affect the migration order.

== Problem ==

However, the current code is lazy.  It completely regenerates the
migration order on *any* CPU or memory hotplug event.  The logic
was that these events are extremely rare and that the overhead
from indiscriminate order regeneration is minimal.

Part of the update logic involves a synchronize_rcu(), which is a
pretty big hammer.  Its overhead was large enough to be detected
by some 0day tests that watch memory hotplug performance[1].

== Solution ==

Add a new helper (node_demotion_topo_changed()) which can
differentiate between superfluous and impactful hotplug events.
Skip the expensive update operation for superfluous events.

== Aside: Locking ==

It took me a few moments to declare the locking to be safe enough
for node_demotion_topo_changed() to work.  It all hinges on the
memory hotplug lock:

During memory hotplug events, 'mem_hotplug_lock' is held for
write.  This ensures that two memory hotplug events can not be
called simultaneously.

CPU hotplug has a similar lock (cpuhp_state_mutex) which also
provides mutual exclusion between CPU hotplug events.  In
addition, the demotion code acquire and hold the mem_hotplug_lock
for read during its CPU hotplug handlers.  This provides mutual
exclusion between the demotion memory hotplug callbacks and the
CPU hotplug callbacks.

This effectively allows treating the migration target generation
code to act as if it is single-threaded.

1. https://lore.kernel.org/all/20210905135932.GE15026@xsang-OptiPlex-9020/

Fixes: 884a6e5d1f93 ("mm/migrate: update node demotion order on hotplug events")
Reported-by: kernel test robot <oliver.sang@intel.com>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "Huang, Ying" <ying.huang@intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Wei Xu <weixugc@google.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: David Rientjes <rientjes@google.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Greg Thelen <gthelen@google.com>
Cc: Yang Shi <yang.shi@linux.alibaba.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---

 b/mm/migrate.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff -puN mm/migrate.c~faster-node-order mm/migrate.c
--- a/mm/migrate.c~faster-node-order	2021-09-24 09:12:30.988377798 -0700
+++ b/mm/migrate.c	2021-09-24 09:12:30.988377798 -0700
@@ -3239,8 +3239,18 @@ static int migration_offline_cpu(unsigne
  * set_migration_target_nodes().
  */
 static int __meminit migrate_on_reclaim_callback(struct notifier_block *self,
-						 unsigned long action, void *arg)
+						 unsigned long action, void *_arg)
 {
+	struct memory_notify *arg = _arg;
+
+	/*
+	 * Only update the node migration order when a node is
+	 * changing status, like online->offline.  This avoids
+	 * the overhead of synchronize_rcu() in most cases.
+	 */
+	if (arg->status_change_nid < 0)
+		return notifier_from_errno(0);
+
 	switch (action) {
 	case MEM_GOING_OFFLINE:
 		/*
_

[PATCH 1/2] mm/migrate: optimize hotplug-time demotion order updates

[Dave Hansen]

From: Dave Hansen <dave.hansen@linux.intel.com>
== tl;dr ==

Automatic demotion opted for a simple, lazy approach to handling
hotplug events.  This noticeably slows down memory hotplug[1].
Optimize away updates to the demotion order when memory hotplug
events should have no effect.

This has no effect on CPU hotplug.  There is no known problem on
the CPU side and any work there will be in a separate series.

== Background ==

Automatic demotion is a memory migration strategy to ensure that
new allocations have room in faster memory tiers on tiered memory
systems.  The kernel maintains an array (node_demotion[]) to
drive these migrations.

The node_demotion[] path is calculated by starting at nodes with
CPUs and then "walking" to nodes with memory.  Only hotplug
events which online or offline a node with memory (N_ONLINE) or
CPUs (N_CPU) will actually affect the migration order.

== Problem ==

However, the current code is lazy.  It completely regenerates the
migration order on *any* CPU or memory hotplug event.  The logic
was that these events are extremely rare and that the overhead
from indiscriminate order regeneration is minimal.

Part of the update logic involves a synchronize_rcu(), which is a
pretty big hammer.  Its overhead was large enough to be detected
by some 0day tests that watch memory hotplug performance[1].

== Solution ==

Add a new helper (node_demotion_topo_changed()) which can
differentiate between superfluous and impactful hotplug events.
Skip the expensive update operation for superfluous events.

== Aside: Locking ==

It took me a few moments to declare the locking to be safe enough
for node_demotion_topo_changed() to work.  It all hinges on the
memory hotplug lock:

During memory hotplug events, 'mem_hotplug_lock' is held for
write.  This ensures that two memory hotplug events can not be
called simultaneously.

CPU hotplug has a similar lock (cpuhp_state_mutex) which also
provides mutual exclusion between CPU hotplug events.  In
addition, the demotion code acquire and hold the mem_hotplug_lock
for read during its CPU hotplug handlers.  This provides mutual
exclusion between the demotion memory hotplug callbacks and the
CPU hotplug callbacks.

This effectively allows treating the migration target generation
code to act as if it is single-threaded.

1. https://lore.kernel.org/all/20210905135932.GE15026@xsang-OptiPlex-9020/

Fixes: 884a6e5d1f93 ("mm/migrate: update node demotion order on hotplug events")
Reported-by: kernel test robot <oliver.sang@intel.com>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "Huang, Ying" <ying.huang@intel.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Wei Xu <weixugc@google.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: David Rientjes <rientjes@google.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Greg Thelen <gthelen@google.com>
Cc: Yang Shi <yang.shi@linux.alibaba.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---

 b/mm/migrate.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff -puN mm/migrate.c~faster-node-order mm/migrate.c
--- a/mm/migrate.c~faster-node-order	2021-09-24 09:12:30.988377798 -0700
+++ b/mm/migrate.c	2021-09-24 09:12:30.988377798 -0700
@@ -3239,8 +3239,18 @@ static int migration_offline_cpu(unsigne
  * set_migration_target_nodes().
  */
 static int __meminit migrate_on_reclaim_callback(struct notifier_block *self,
-						 unsigned long action, void *arg)
+						 unsigned long action, void *_arg)
 {
+	struct memory_notify *arg = _arg;
+
+	/*
+	 * Only update the node migration order when a node is
+	 * changing status, like online->offline.  This avoids
+	 * the overhead of synchronize_rcu() in most cases.
+	 */
+	if (arg->status_change_nid < 0)
+		return notifier_from_errno(0);
+
 	switch (action) {
 	case MEM_GOING_OFFLINE:
 		/*
_