Util-Linux Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH RFC] drivers/base/memory.c: indicate all memory blocks as removable
@ 2020-01-24 15:53 David Hildenbrand
  2020-01-24 19:10 ` Fontenot, Nathan
  2020-01-27 13:29 ` Michal Hocko
  0 siblings, 2 replies; 8+ messages in thread
From: David Hildenbrand @ 2020-01-24 15:53 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-mm, David Hildenbrand, Michal Hocko, Dan Williams,
	Greg Kroah-Hartman, Rafael J. Wysocki, Andrew Morton,
	powerpc-utils-devel, util-linux, Badari Pulavarty,
	Nathan Fontenot, Robert Jennings, Heiko Carstens, Karel Zak

We see multiple issues with the implementation/interface to compute
whether a memory block can be offlined (exposed via
/sys/devices/system/memory/memoryX/removable) and would like to simplify
it (remove the implementation).

1. It runs basically lockless. While this might be good for performance,
   we see possible races with memory offlining/unplug that will require
   at least some sort of locking to fix.

2. Nowadays, more false positives are possible. No arch-specific checks
   are performed that validate if memory offlining will not be denied
   right away (and such check will require locking). For example, arm64
   won't allow to offline any memory block that was added during boot -
   which will imply a very high error rate. Other archs have other
   constraints.

3. The interface is inherently racy. E.g., if a memory block is
   detected to be removable (and was not a false positive at that time),
   there is still no guarantee that offlining will actually succeed. So
   any caller already has to deal with false positives.

4. It is unclear which performance benefit this interface actually
   provides. The introducing commit 5c755e9fd813 ("memory-hotplug: add
   sysfs removable attribute for hotplug memory remove") mentioned
	"A user-level agent must be able to identify which sections of
	 memory are likely to be removable before attempting the
	 potentially expensive operation."
   However, no actual performance comparison was included.

Known users:
- lsmem: Will group memory blocks based on the "removable" property. [1]
- chmem: Indirect user. It has a RANGE mode where one can specify
	 removable ranges identified via lsmem to be offlined. However, it
	 also has a "SIZE" mode, which allows a sysadmin to skip the manual
	 "identify removable blocks" step. [2]
- powerpc-utils: Uses the "removable" attribute to skip some memory
		 blocks right away when trying to find some to
		 offline+remove. However, with ballooning enabled, it
		 already skips this information completely (because it
		 once resulted in many false negatives). Therefore, the
		 implementation can deal with false positives properly
		 already. [3]

With CONFIG_MEMORY_HOTREMOVE, always indicating "removable" should not
break any user space tool. We implement a very bad heuristic now. (in
contrast: always returning "not removable" would at least affect
powerpc-utils)

Without CONFIG_MEMORY_HOTREMOVE we cannot offline anything, so report
"not removable" as before.

Original discussion can be found in [4] ("[PATCH RFC v1] mm:
is_mem_section_removable() overhaul").

Other users of is_mem_section_removable() will be removed next, so that
we can remove is_mem_section_removable() completely.

[1] http://man7.org/linux/man-pages/man1/lsmem.1.html
[2] http://man7.org/linux/man-pages/man8/chmem.8.html
[3] https://github.com/ibm-power-utilities/powerpc-utils
[4] https://lkml.kernel.org/r/20200117105759.27905-1-david@redhat.com

Suggested-by: Michal Hocko <mhocko@kernel.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: powerpc-utils-devel@googlegroups.com
Cc: util-linux@vger.kernel.org
Cc: Badari Pulavarty <pbadari@us.ibm.com>
Cc: Nathan Fontenot <nfont@linux.vnet.ibm.com>
Cc: Robert Jennings <rcj@linux.vnet.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Karel Zak <kzak@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---

1. Are there any use cases that really require this interface to keep
   producing "more reliable" results?

2. Is there any real performance advantage when using this interface to
   identify memory blocks to offline?

---
 drivers/base/memory.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 6503f5d0b749..d78a92f09984 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -105,30 +105,17 @@ static ssize_t phys_index_show(struct device *dev,
 }
 
 /*
- * Show whether the memory block is likely to be offlineable (or is already
- * offline). Once offline, the memory block could be removed. The return
- * value does, however, not indicate that there is a way to remove the
- * memory block.
+ * Legacy interface that we cannot remove. Always indicate "removable"
+ * with CONFIG_MEMORY_HOTREMOVE - bad heuristic.
  */
 static ssize_t removable_show(struct device *dev, struct device_attribute *attr,
 			      char *buf)
 {
-	struct memory_block *mem = to_memory_block(dev);
-	unsigned long pfn;
-	int ret = 1, i;
-
-	if (mem->state != MEM_ONLINE)
-		goto out;
-
-	for (i = 0; i < sections_per_block; i++) {
-		if (!present_section_nr(mem->start_section_nr + i))
-			continue;
-		pfn = section_nr_to_pfn(mem->start_section_nr + i);
-		ret &= is_mem_section_removable(pfn, PAGES_PER_SECTION);
-	}
-
-out:
-	return sprintf(buf, "%d\n", ret);
+#ifdef CONFIG_MEMORY_HOTREMOVE
+	return sprintf(buf, "1\n");
+#else
+	return sprintf(buf, "0\n");
+#endif
 }
 
 /*
-- 
2.24.1


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

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-24 15:53 [PATCH RFC] drivers/base/memory.c: indicate all memory blocks as removable David Hildenbrand
2020-01-24 19:10 ` Fontenot, Nathan
2020-01-27  9:23   ` Michal Hocko
2020-01-27  9:33     ` David Hildenbrand
2020-01-27 13:25       ` Michal Hocko
2020-01-27 15:23       ` Fontenot, Nathan
2020-01-27 13:29 ` Michal Hocko
2020-01-27 13:34   ` David Hildenbrand

Util-Linux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/util-linux/0 util-linux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 util-linux util-linux/ https://lore.kernel.org/util-linux \
		util-linux@vger.kernel.org
	public-inbox-index util-linux

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.util-linux


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git