Re: [Xen-devel] [PATCH RFCv2 1/4] mm/memory_hotplug: Introduce memory block types

[Wei Yang <richard.weiyang@xxxxxxxxx>]

On Fri, Nov 30, 2018 at 06:59:19PM +0100, David Hildenbrand wrote:
>Memory onlining should always be handled by user space, because only user
>space knows which use cases it wants to satisfy. E.g. memory might be
>onlined to the MOVABLE zone even if it can never be removed from the
>system, e.g. to make usage of huge pages more reliable.
>
>However to implement such rules (especially default rules in distributions)
>we need more information about the memory that was added in user space.
>
>E.g. on x86 we want to online memory provided by balloon devices (e.g.
>XEN, Hyper-V) differently (-> will not be unplugged by offlining the whole
>block) than ordinary DIMMs (-> might eventually be unplugged by offlining
>the whole block). This might also become relevat for other architectures.
>
>Also, udev rules right now check if running on s390x and treat all added
>memory blocks as standby memory (-> don't online automatically). As soon as
>we support other memory hotplug mechanism (e.g. virtio-mem) checks would
>have to get more involved (e.g. also check if under KVM) but eventually
>also wrong (e.g. if KVM ever supports standby memory we are doomed).
>
>I decided to allow to specify the type of memory that is getting added
>to the system. Let's start with two types, BOOT and UNSPECIFIED to get the
>basic infrastructure running. We'll introduce and use further types in
>follow-up patches. For now we classify any hotplugged memory temporarily
>as as UNSPECIFIED (which will eventually be dropped later on).
>
>Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
>Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>Cc: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
>Cc: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx>
>Cc: Andrew Banman <andrew.banman@xxxxxxx>
>Cc: "mike.travis@xxxxxxx" <mike.travis@xxxxxxx>
>Cc: Oscar Salvador <osalvador@xxxxxxxx>
>Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
>Cc: Michal Hocko <mhocko@xxxxxxxxxx>
>Cc: Michal Such??nek <msuchanek@xxxxxxx>
>Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
>Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
>Cc: Pavel Tatashin <pavel.tatashin@xxxxxxxxxxxxx>
>Cc: Martin Schwidefsky <schwidefsky@xxxxxxxxxx>
>Cc: Heiko Carstens <heiko.carstens@xxxxxxxxxx>
>Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>---
> drivers/base/memory.c  | 38 +++++++++++++++++++++++++++++++++++---
> include/linux/memory.h | 27 +++++++++++++++++++++++++++
> 2 files changed, 62 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/base/memory.c b/drivers/base/memory.c
>index 0c290f86ab20..17f2985c07c5 100644
>--- a/drivers/base/memory.c
>+++ b/drivers/base/memory.c
>@@ -381,6 +381,29 @@ static ssize_t show_phys_device(struct device *dev,
>       return sprintf(buf, "%d\n", mem->phys_device);
> }
> 
>+static ssize_t type_show(struct device *dev, struct device_attribute *attr,
>+                       char *buf)
>+{
>+      struct memory_block *mem = to_memory_block(dev);
>+      ssize_t len = 0;
>+
>+      switch (mem->type) {
>+      case MEMORY_BLOCK_UNSPECIFIED:
>+              len = sprintf(buf, "unspecified\n");
>+              break;
>+      case MEMORY_BLOCK_BOOT:
>+              len = sprintf(buf, "boot\n");
>+              break;
>+      default:
>+              len = sprintf(buf, "ERROR-UNKNOWN-%ld\n",
>+                              mem->state);
>+              WARN_ON(1);
>+              break;
>+      }
>+
>+      return len;
>+}
>+
> #ifdef CONFIG_MEMORY_HOTREMOVE
> static void print_allowed_zone(char *buf, int nid, unsigned long start_pfn,
>               unsigned long nr_pages, int online_type,
>@@ -442,6 +465,7 @@ static DEVICE_ATTR(phys_index, 0444, 
>show_mem_start_phys_index, NULL);
> static DEVICE_ATTR(state, 0644, show_mem_state, store_mem_state);
> static DEVICE_ATTR(phys_device, 0444, show_phys_device, NULL);
> static DEVICE_ATTR(removable, 0444, show_mem_removable, NULL);
>+static DEVICE_ATTR_RO(type);

This is correct, while looks not consistent with other attributes.

Not that beautiful :-)

> 
> /*
>  * Block size attribute stuff
>@@ -620,6 +644,7 @@ static struct attribute *memory_memblk_attrs[] = {
>       &dev_attr_state.attr,
>       &dev_attr_phys_device.attr,
>       &dev_attr_removable.attr,
>+      &dev_attr_type.attr,
> #ifdef CONFIG_MEMORY_HOTREMOVE
>       &dev_attr_valid_zones.attr,
> #endif
>@@ -657,13 +682,17 @@ int register_memory(struct memory_block *memory)
> }
> 
> static int init_memory_block(struct memory_block **memory,
>-                           struct mem_section *section, unsigned long state)
>+                           struct mem_section *section, unsigned long state,
>+                           int type)
> {
>       struct memory_block *mem;
>       unsigned long start_pfn;
>       int scn_nr;
>       int ret = 0;
> 
>+      if (type == MEMORY_BLOCK_NONE)
>+              return -EINVAL;

No one will pass in this value. Can we omit this check for now?

>+
>       mem = kzalloc(sizeof(*mem), GFP_KERNEL);
>       if (!mem)
>               return -ENOMEM;
>@@ -675,6 +704,7 @@ static int init_memory_block(struct memory_block **memory,
>       mem->state = state;
>       start_pfn = section_nr_to_pfn(mem->start_section_nr);
>       mem->phys_device = arch_get_memory_phys_device(start_pfn);
>+      mem->type = type;
> 
>       ret = register_memory(mem);
> 
>@@ -699,7 +729,8 @@ static int add_memory_block(int base_section_nr)
> 
>       if (section_count == 0)
>               return 0;
>-      ret = init_memory_block(&mem, __nr_to_section(section_nr), MEM_ONLINE);
>+      ret = init_memory_block(&mem, __nr_to_section(section_nr), MEM_ONLINE,
>+                              MEMORY_BLOCK_BOOT);
>       if (ret)
>               return ret;
>       mem->section_count = section_count;
>@@ -722,7 +753,8 @@ int hotplug_memory_register(int nid, struct mem_section 
>*section)
>               mem->section_count++;
>               put_device(&mem->dev);
>       } else {
>-              ret = init_memory_block(&mem, section, MEM_OFFLINE);
>+              ret = init_memory_block(&mem, section, MEM_OFFLINE,
>+                                      MEMORY_BLOCK_UNSPECIFIED);
>               if (ret)
>                       goto out;
>               mem->section_count++;
>diff --git a/include/linux/memory.h b/include/linux/memory.h
>index d75ec88ca09d..06268e96e0da 100644
>--- a/include/linux/memory.h
>+++ b/include/linux/memory.h
>@@ -34,12 +34,39 @@ struct memory_block {
>       int (*phys_callback)(struct memory_block *);
>       struct device dev;
>       int nid;                        /* NID for this memory block */
>+      int type;                       /* type of this memory block */
> };
> 
> int arch_get_memory_phys_device(unsigned long start_pfn);
> unsigned long memory_block_size_bytes(void);
> int set_memory_block_size_order(unsigned int order);
> 
>+/*
>+ * Memory block types allow user space to formulate rules if and how to
>+ * online memory blocks. The types are exposed to user space as text
>+ * strings in sysfs.
>+ *
>+ * MEMORY_BLOCK_NONE:
>+ *  No memory block is to be created (e.g. device memory). Not exposed to
>+ *  user space.
>+ *
>+ * MEMORY_BLOCK_UNSPECIFIED:
>+ *  The type of memory block was not further specified when adding the
>+ *  memory block.
>+ *
>+ * MEMORY_BLOCK_BOOT:
>+ *  This memory block was added during boot by the basic system. No
>+ *  specific device driver takes care of this memory block. This memory
>+ *  block type is onlined automatically by the kernel during boot and might
>+ *  later be managed by a different device driver, in which case the type
>+ *  might change.
>+ */
>+enum {
>+      MEMORY_BLOCK_NONE = 0,
>+      MEMORY_BLOCK_UNSPECIFIED,
>+      MEMORY_BLOCK_BOOT,
>+};
>+
> /* These states are exposed to userspace as text strings in sysfs */
> #define       MEM_ONLINE              (1<<0) /* exposed to userspace */
> #define       MEM_GOING_OFFLINE       (1<<1) /* exposed to userspace */
>-- 
>2.17.2

-- 
Wei Yang
Help you, Help me

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel