linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] hmat: Register attributes for memory hot add
@ 2019-04-09 21:44 Keith Busch
  2019-04-10 11:07 ` Brice Goglin
  2019-04-11 14:42 ` Rafael J. Wysocki
  0 siblings, 2 replies; 4+ messages in thread
From: Keith Busch @ 2019-04-09 21:44 UTC (permalink / raw)
  To: linux-kernel, linux-acpi, linux-mm
  Cc: Rafael Wysocki, Dave Hansen, Dan Williams, Brice Goglin, Keith Busch

Some types of memory nodes that HMAT describes may not be online at the
time we initially parse their nodes' tables. If the node should be set
to online later, as can happen when using PMEM as RAM after boot, the
node's attributes will be missing their initiator links and performance.

Regsiter a memory notifier callback and set the memory attributes when
a node is initially brought online with hot added memory, and don't try
to register node attributes if the node is not online during initial
scanning.

Signed-off-by: Keith Busch <keith.busch@intel.com>
---
 drivers/acpi/hmat/hmat.c | 63 ++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/hmat/hmat.c b/drivers/acpi/hmat/hmat.c
index b275016ff648..cf24b885feb5 100644
--- a/drivers/acpi/hmat/hmat.c
+++ b/drivers/acpi/hmat/hmat.c
@@ -14,14 +14,15 @@
 #include <linux/init.h>
 #include <linux/list.h>
 #include <linux/list_sort.h>
+#include <linux/memory.h>
 #include <linux/node.h>
 #include <linux/sysfs.h>
 
-static __initdata u8 hmat_revision;
+static u8 hmat_revision;
 
-static __initdata LIST_HEAD(targets);
-static __initdata LIST_HEAD(initiators);
-static __initdata LIST_HEAD(localities);
+static LIST_HEAD(targets);
+static LIST_HEAD(initiators);
+static LIST_HEAD(localities);
 
 /*
  * The defined enum order is used to prioritize attributes to break ties when
@@ -41,6 +42,7 @@ struct memory_target {
 	unsigned int memory_pxm;
 	unsigned int processor_pxm;
 	struct node_hmem_attrs hmem_attrs;
+	bool registered;
 };
 
 struct memory_initiator {
@@ -53,7 +55,7 @@ struct memory_locality {
 	struct acpi_hmat_locality *hmat_loc;
 };
 
-static __init struct memory_initiator *find_mem_initiator(unsigned int cpu_pxm)
+static struct memory_initiator *find_mem_initiator(unsigned int cpu_pxm)
 {
 	struct memory_initiator *initiator;
 
@@ -63,7 +65,7 @@ static __init struct memory_initiator *find_mem_initiator(unsigned int cpu_pxm)
 	return NULL;
 }
 
-static __init struct memory_target *find_mem_target(unsigned int mem_pxm)
+static struct memory_target *find_mem_target(unsigned int mem_pxm)
 {
 	struct memory_target *target;
 
@@ -148,7 +150,7 @@ static __init const char *hmat_data_type_suffix(u8 type)
 	}
 }
 
-static __init u32 hmat_normalize(u16 entry, u64 base, u8 type)
+static u32 hmat_normalize(u16 entry, u64 base, u8 type)
 {
 	u32 value;
 
@@ -183,7 +185,7 @@ static __init u32 hmat_normalize(u16 entry, u64 base, u8 type)
 	return value;
 }
 
-static __init void hmat_update_target_access(struct memory_target *target,
+static void hmat_update_target_access(struct memory_target *target,
 					     u8 type, u32 value)
 {
 	switch (type) {
@@ -435,7 +437,7 @@ static __init int srat_parse_mem_affinity(union acpi_subtable_headers *header,
 	return 0;
 }
 
-static __init u32 hmat_initiator_perf(struct memory_target *target,
+static u32 hmat_initiator_perf(struct memory_target *target,
 			       struct memory_initiator *initiator,
 			       struct acpi_hmat_locality *hmat_loc)
 {
@@ -473,7 +475,7 @@ static __init u32 hmat_initiator_perf(struct memory_target *target,
 			      hmat_loc->data_type);
 }
 
-static __init bool hmat_update_best(u8 type, u32 value, u32 *best)
+static bool hmat_update_best(u8 type, u32 value, u32 *best)
 {
 	bool updated = false;
 
@@ -517,7 +519,7 @@ static int initiator_cmp(void *priv, struct list_head *a, struct list_head *b)
 	return ia->processor_pxm - ib->processor_pxm;
 }
 
-static __init void hmat_register_target_initiators(struct memory_target *target)
+static void hmat_register_target_initiators(struct memory_target *target)
 {
 	static DECLARE_BITMAP(p_nodes, MAX_NUMNODES);
 	struct memory_initiator *initiator;
@@ -577,22 +579,53 @@ static __init void hmat_register_target_initiators(struct memory_target *target)
 	}
 }
 
-static __init void hmat_register_target_perf(struct memory_target *target)
+static void hmat_register_target_perf(struct memory_target *target)
 {
 	unsigned mem_nid = pxm_to_node(target->memory_pxm);
 	node_set_perf_attrs(mem_nid, &target->hmem_attrs, 0);
 }
 
-static __init void hmat_register_targets(void)
+static void hmat_register_targets(void)
 {
 	struct memory_target *target;
 
 	list_for_each_entry(target, &targets, node) {
+		if (!node_online(pxm_to_node(target->memory_pxm)))
+			continue;
+
 		hmat_register_target_initiators(target);
 		hmat_register_target_perf(target);
+		target->registered = true;
 	}
 }
 
+static int hmat_callback(struct notifier_block *self,
+			 unsigned long action, void *arg)
+{
+	struct memory_notify *mnb = arg;
+	int pxm, nid = mnb->status_change_nid;
+	struct memory_target *target;
+
+	if (nid == NUMA_NO_NODE || action != MEM_ONLINE)
+		return NOTIFY_OK;
+
+	pxm = node_to_pxm(nid);
+	target = find_mem_target(pxm);
+	if (!target || target->registered)
+		return NOTIFY_OK;
+
+	hmat_register_target_initiators(target);
+	hmat_register_target_perf(target);
+	target->registered = true;
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block hmat_callback_nb = {
+	.notifier_call = hmat_callback,
+	.priority = 2,
+};
+
 static __init void hmat_free_structures(void)
 {
 	struct memory_target *target, *tnext;
@@ -658,6 +691,10 @@ static __init int hmat_init(void)
 		}
 	}
 	hmat_register_targets();
+
+	/* Keep the table and structures if the notifier may use them */
+	if (!register_hotmemory_notifier(&hmat_callback_nb))
+		return 0;
 out_put:
 	hmat_free_structures();
 	acpi_put_table(tbl);
-- 
2.14.4


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH] hmat: Register attributes for memory hot add
  2019-04-09 21:44 [PATCH] hmat: Register attributes for memory hot add Keith Busch
@ 2019-04-10 11:07 ` Brice Goglin
  2019-04-11 14:42 ` Rafael J. Wysocki
  1 sibling, 0 replies; 4+ messages in thread
From: Brice Goglin @ 2019-04-10 11:07 UTC (permalink / raw)
  To: Keith Busch, linux-kernel, linux-acpi, linux-mm
  Cc: Rafael Wysocki, Dave Hansen, Dan Williams

Thanks Keith, this solves my issue.

On a machine where node4 (PMEM) is close to node0 and node1 (1st socket
SNCs), hotplugging that node makes node0 and node1 appear as initiators
to node4 (and node4 as target to them). Same for the other socket. And
perf attributes look good.

Reported-and-tested-by: Brice Goglin <Brice.Goglin@inria.fr>

Just one minor typo below.


Le 09/04/2019 à 23:44, Keith Busch a écrit :
> Some types of memory nodes that HMAT describes may not be online at the
> time we initially parse their nodes' tables. If the node should be set
> to online later, as can happen when using PMEM as RAM after boot, the
> node's attributes will be missing their initiator links and performance.
>
> Regsiter a memory notifier callback and set the memory attributes when

Typo regsiter->register

thanks

Brice



^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] hmat: Register attributes for memory hot add
  2019-04-09 21:44 [PATCH] hmat: Register attributes for memory hot add Keith Busch
  2019-04-10 11:07 ` Brice Goglin
@ 2019-04-11 14:42 ` Rafael J. Wysocki
  2019-04-11 15:00   ` Keith Busch
  1 sibling, 1 reply; 4+ messages in thread
From: Rafael J. Wysocki @ 2019-04-11 14:42 UTC (permalink / raw)
  To: Keith Busch
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Memory Management List, Rafael Wysocki, Dave Hansen,
	Dan Williams, Brice Goglin

On Tue, Apr 9, 2019 at 11:42 PM Keith Busch <keith.busch@intel.com> wrote:
>
> Some types of memory nodes that HMAT describes may not be online at the
> time we initially parse their nodes' tables. If the node should be set
> to online later, as can happen when using PMEM as RAM after boot, the
> node's attributes will be missing their initiator links and performance.
>
> Regsiter a memory notifier callback and set the memory attributes when
> a node is initially brought online with hot added memory, and don't try
> to register node attributes if the node is not online during initial
> scanning.
>
> Signed-off-by: Keith Busch <keith.busch@intel.com>
> ---
>  drivers/acpi/hmat/hmat.c | 63 ++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 50 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/hmat/hmat.c b/drivers/acpi/hmat/hmat.c
> index b275016ff648..cf24b885feb5 100644
> --- a/drivers/acpi/hmat/hmat.c
> +++ b/drivers/acpi/hmat/hmat.c
> @@ -14,14 +14,15 @@
>  #include <linux/init.h>
>  #include <linux/list.h>
>  #include <linux/list_sort.h>
> +#include <linux/memory.h>
>  #include <linux/node.h>
>  #include <linux/sysfs.h>
>
> -static __initdata u8 hmat_revision;
> +static u8 hmat_revision;
>
> -static __initdata LIST_HEAD(targets);
> -static __initdata LIST_HEAD(initiators);
> -static __initdata LIST_HEAD(localities);
> +static LIST_HEAD(targets);
> +static LIST_HEAD(initiators);
> +static LIST_HEAD(localities);
>
>  /*
>   * The defined enum order is used to prioritize attributes to break ties when
> @@ -41,6 +42,7 @@ struct memory_target {
>         unsigned int memory_pxm;
>         unsigned int processor_pxm;
>         struct node_hmem_attrs hmem_attrs;
> +       bool registered;
>  };
>
>  struct memory_initiator {
> @@ -53,7 +55,7 @@ struct memory_locality {
>         struct acpi_hmat_locality *hmat_loc;
>  };
>
> -static __init struct memory_initiator *find_mem_initiator(unsigned int cpu_pxm)
> +static struct memory_initiator *find_mem_initiator(unsigned int cpu_pxm)
>  {
>         struct memory_initiator *initiator;
>
> @@ -63,7 +65,7 @@ static __init struct memory_initiator *find_mem_initiator(unsigned int cpu_pxm)
>         return NULL;
>  }
>
> -static __init struct memory_target *find_mem_target(unsigned int mem_pxm)
> +static struct memory_target *find_mem_target(unsigned int mem_pxm)
>  {
>         struct memory_target *target;
>
> @@ -148,7 +150,7 @@ static __init const char *hmat_data_type_suffix(u8 type)
>         }
>  }
>
> -static __init u32 hmat_normalize(u16 entry, u64 base, u8 type)
> +static u32 hmat_normalize(u16 entry, u64 base, u8 type)
>  {
>         u32 value;
>
> @@ -183,7 +185,7 @@ static __init u32 hmat_normalize(u16 entry, u64 base, u8 type)
>         return value;
>  }
>
> -static __init void hmat_update_target_access(struct memory_target *target,
> +static void hmat_update_target_access(struct memory_target *target,
>                                              u8 type, u32 value)
>  {
>         switch (type) {
> @@ -435,7 +437,7 @@ static __init int srat_parse_mem_affinity(union acpi_subtable_headers *header,
>         return 0;
>  }
>
> -static __init u32 hmat_initiator_perf(struct memory_target *target,
> +static u32 hmat_initiator_perf(struct memory_target *target,
>                                struct memory_initiator *initiator,
>                                struct acpi_hmat_locality *hmat_loc)
>  {
> @@ -473,7 +475,7 @@ static __init u32 hmat_initiator_perf(struct memory_target *target,
>                               hmat_loc->data_type);
>  }
>
> -static __init bool hmat_update_best(u8 type, u32 value, u32 *best)
> +static bool hmat_update_best(u8 type, u32 value, u32 *best)
>  {
>         bool updated = false;
>
> @@ -517,7 +519,7 @@ static int initiator_cmp(void *priv, struct list_head *a, struct list_head *b)
>         return ia->processor_pxm - ib->processor_pxm;
>  }
>
> -static __init void hmat_register_target_initiators(struct memory_target *target)
> +static void hmat_register_target_initiators(struct memory_target *target)
>  {
>         static DECLARE_BITMAP(p_nodes, MAX_NUMNODES);
>         struct memory_initiator *initiator;
> @@ -577,22 +579,53 @@ static __init void hmat_register_target_initiators(struct memory_target *target)
>         }
>  }
>
> -static __init void hmat_register_target_perf(struct memory_target *target)
> +static void hmat_register_target_perf(struct memory_target *target)
>  {
>         unsigned mem_nid = pxm_to_node(target->memory_pxm);
>         node_set_perf_attrs(mem_nid, &target->hmem_attrs, 0);
>  }
>
> -static __init void hmat_register_targets(void)
> +static void hmat_register_targets(void)
>  {
>         struct memory_target *target;
>
>         list_for_each_entry(target, &targets, node) {
> +               if (!node_online(pxm_to_node(target->memory_pxm)))
> +                       continue;
> +
>                 hmat_register_target_initiators(target);
>                 hmat_register_target_perf(target);
> +               target->registered = true;
>         }
>  }
>
> +static int hmat_callback(struct notifier_block *self,
> +                        unsigned long action, void *arg)
> +{
> +       struct memory_notify *mnb = arg;
> +       int pxm, nid = mnb->status_change_nid;
> +       struct memory_target *target;
> +
> +       if (nid == NUMA_NO_NODE || action != MEM_ONLINE)
> +               return NOTIFY_OK;
> +
> +       pxm = node_to_pxm(nid);
> +       target = find_mem_target(pxm);
> +       if (!target || target->registered)
> +               return NOTIFY_OK;
> +
> +       hmat_register_target_initiators(target);
> +       hmat_register_target_perf(target);
> +       target->registered = true;
> +
> +       return NOTIFY_OK;
> +}

This appears to assume that there will never be any races between the
two functions above.

It this guaranteed to be the case?

> +
> +static struct notifier_block hmat_callback_nb = {
> +       .notifier_call = hmat_callback,
> +       .priority = 2,
> +};
> +
>  static __init void hmat_free_structures(void)
>  {
>         struct memory_target *target, *tnext;
> @@ -658,6 +691,10 @@ static __init int hmat_init(void)
>                 }
>         }
>         hmat_register_targets();
> +
> +       /* Keep the table and structures if the notifier may use them */
> +       if (!register_hotmemory_notifier(&hmat_callback_nb))
> +               return 0;
>  out_put:
>         hmat_free_structures();
>         acpi_put_table(tbl);
> --

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] hmat: Register attributes for memory hot add
  2019-04-11 14:42 ` Rafael J. Wysocki
@ 2019-04-11 15:00   ` Keith Busch
  0 siblings, 0 replies; 4+ messages in thread
From: Keith Busch @ 2019-04-11 15:00 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Linux Kernel Mailing List, ACPI Devel Maling List,
	Linux Memory Management List, Dave Hansen, Dan Williams,
	Brice Goglin

On Thu, Apr 11, 2019 at 04:42:45PM +0200, Rafael J. Wysocki wrote:
> On Tue, Apr 9, 2019 at 11:42 PM Keith Busch <keith.busch@intel.com> wrote:
> > -static __init void hmat_register_targets(void)
> > +static void hmat_register_targets(void)
> >  {
> >         struct memory_target *target;
> >
> >         list_for_each_entry(target, &targets, node) {
> > +               if (!node_online(pxm_to_node(target->memory_pxm)))
> > +                       continue;
> > +
> >                 hmat_register_target_initiators(target);
> >                 hmat_register_target_perf(target);
> > +               target->registered = true;
> >         }
> >  }
> >
> > +static int hmat_callback(struct notifier_block *self,
> > +                        unsigned long action, void *arg)
> > +{
> > +       struct memory_notify *mnb = arg;
> > +       int pxm, nid = mnb->status_change_nid;
> > +       struct memory_target *target;
> > +
> > +       if (nid == NUMA_NO_NODE || action != MEM_ONLINE)
> > +               return NOTIFY_OK;
> > +
> > +       pxm = node_to_pxm(nid);
> > +       target = find_mem_target(pxm);
> > +       if (!target || target->registered)
> > +               return NOTIFY_OK;
> > +
> > +       hmat_register_target_initiators(target);
> > +       hmat_register_target_perf(target);
> > +       target->registered = true;
> > +
> > +       return NOTIFY_OK;
> > +}
> 
> This appears to assume that there will never be any races between the
> two functions above.
> 
> It this guaranteed to be the case?

The hmat_init() will call this directly before registering the memory
notifier callback, so those two paths should be 'ok'.

I may have assumed memory notification callbacks were single threaded,
but after taking a quick look, I think I do need additional locking for
this to be safe. I'll get that fixed up, thanks for the catch.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-04-11 14:59 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-09 21:44 [PATCH] hmat: Register attributes for memory hot add Keith Busch
2019-04-10 11:07 ` Brice Goglin
2019-04-11 14:42 ` Rafael J. Wysocki
2019-04-11 15:00   ` Keith Busch

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).