* [PATCH v1 0/6] mm: Further memory block device cleanups
@ 2019-06-14 10:01 David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 1/6] mm: Section numbers use the type "unsigned long" David Hildenbrand
` (5 more replies)
0 siblings, 6 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-06-14 10:01 UTC (permalink / raw)
To: linux-kernel
Cc: Oscar Salvador, Michal Hocko, David Hildenbrand, Wei Yang,
Keith Busch, linux-mm, Arun KS, Rashmica Gupta, Thomas Gleixner,
Stephen Rothwell, Michael Neuling, Baoquan He, Rafael J. Wysocki,
Pavel Tatashin, linux-acpi, Len Brown, Pavel Tatashin,
Pavel Tatashin, Anshuman Khandual, mike.travis, linuxppc-dev,
Mike Rapoport, Qian Cai, Dan Williams, Vlastimil Babka,
Oscar Salvador, Juergen Gross, Andrew Banman, Greg Kroah-Hartman,
Rafael J. Wysocki, Johannes Weiner, Paul Mackerras,
Andrew Morton, Mel Gorman
Some further cleanups around memory block devices. Especially, clean up
and simplify walk_memory_range(). Including some other minor cleanups.
Based on: linux-next
Minor conflict with Dan's subsection hot-add series.
Compiled + tested on x86 with DIMMs under QEMU.
David Hildenbrand (6):
mm: Section numbers use the type "unsigned long"
drivers/base/memory: Use "unsigned long" for block ids
mm: Make register_mem_sect_under_node() static
mm/memory_hotplug: Rename walk_memory_range() and pass start+size
instead of pfns
mm/memory_hotplug: Move and simplify walk_memory_blocks()
drivers/base/memory.c: Get rid of find_memory_block_hinted()
arch/powerpc/platforms/powernv/memtrace.c | 22 +++---
drivers/acpi/acpi_memhotplug.c | 19 ++----
drivers/base/memory.c | 81 +++++++++++++++++------
drivers/base/node.c | 8 ++-
include/linux/memory.h | 5 +-
include/linux/memory_hotplug.h | 2 -
include/linux/mmzone.h | 4 +-
include/linux/node.h | 7 --
mm/memory_hotplug.c | 57 +---------------
mm/sparse.c | 12 ++--
10 files changed, 92 insertions(+), 125 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v1 1/6] mm: Section numbers use the type "unsigned long"
2019-06-14 10:01 [PATCH v1 0/6] mm: Further memory block device cleanups David Hildenbrand
@ 2019-06-14 10:01 ` David Hildenbrand
2019-06-14 19:00 ` Andrew Morton
2019-06-14 10:01 ` [PATCH v1 2/6] drivers/base/memory: Use "unsigned long" for block ids David Hildenbrand
` (4 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: David Hildenbrand @ 2019-06-14 10:01 UTC (permalink / raw)
To: linux-kernel
Cc: Stephen Rothwell, Michal Hocko, Pavel Tatashin, Baoquan He,
David Hildenbrand, Greg Kroah-Hartman, Rafael J. Wysocki,
Mel Gorman, Wei Yang, linux-mm, linux-acpi, Mike Rapoport,
Arun KS, Johannes Weiner, Dan Williams, linuxppc-dev,
Andrew Morton, Vlastimil Babka, Oscar Salvador
We are using a mixture of "int" and "unsigned long". Let's make this
consistent by using "unsigned long" everywhere. We'll do the same with
memory block ids next.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Arun KS <arunks@codeaurora.org>
Cc: Pavel Tatashin <pasha.tatashin@oracle.com>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Mike Rapoport <rppt@linux.vnet.ibm.com>
Cc: Baoquan He <bhe@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
drivers/base/memory.c | 9 +++++----
include/linux/mmzone.h | 4 ++--
mm/sparse.c | 12 ++++++------
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 826dd76f662e..5b3a2fd250ba 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -34,7 +34,7 @@ static DEFINE_MUTEX(mem_sysfs_mutex);
static int sections_per_block;
-static inline int base_memory_block_id(int section_nr)
+static inline int base_memory_block_id(unsigned long section_nr)
{
return section_nr / sections_per_block;
}
@@ -691,10 +691,11 @@ static int init_memory_block(struct memory_block **memory, int block_id,
return ret;
}
-static int add_memory_block(int base_section_nr)
+static int add_memory_block(unsigned long base_section_nr)
{
+ int ret, section_count = 0;
struct memory_block *mem;
- int i, ret, section_count = 0;
+ unsigned long i;
for (i = base_section_nr;
i < base_section_nr + sections_per_block;
@@ -822,7 +823,7 @@ static const struct attribute_group *memory_root_attr_groups[] = {
*/
int __init memory_dev_init(void)
{
- unsigned int i;
+ unsigned long i;
int ret;
int err;
unsigned long block_sz;
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 427b79c39b3c..83b6aae16f13 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1220,7 +1220,7 @@ static inline struct mem_section *__nr_to_section(unsigned long nr)
return NULL;
return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK];
}
-extern int __section_nr(struct mem_section* ms);
+extern unsigned long __section_nr(struct mem_section *ms);
extern unsigned long usemap_size(void);
/*
@@ -1292,7 +1292,7 @@ static inline struct mem_section *__pfn_to_section(unsigned long pfn)
return __nr_to_section(pfn_to_section_nr(pfn));
}
-extern int __highest_present_section_nr;
+extern unsigned long __highest_present_section_nr;
#ifndef CONFIG_HAVE_ARCH_PFN_VALID
static inline int pfn_valid(unsigned long pfn)
diff --git a/mm/sparse.c b/mm/sparse.c
index 1552c855d62a..e8c57e039be8 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -102,7 +102,7 @@ static inline int sparse_index_init(unsigned long section_nr, int nid)
#endif
#ifdef CONFIG_SPARSEMEM_EXTREME
-int __section_nr(struct mem_section* ms)
+unsigned long __section_nr(struct mem_section *ms)
{
unsigned long root_nr;
struct mem_section *root = NULL;
@@ -121,9 +121,9 @@ int __section_nr(struct mem_section* ms)
return (root_nr * SECTIONS_PER_ROOT) + (ms - root);
}
#else
-int __section_nr(struct mem_section* ms)
+unsigned long __section_nr(struct mem_section *ms)
{
- return (int)(ms - mem_section[0]);
+ return (unsigned long)(ms - mem_section[0]);
}
#endif
@@ -178,10 +178,10 @@ void __meminit mminit_validate_memmodel_limits(unsigned long *start_pfn,
* Keeping track of this gives us an easy way to break out of
* those loops early.
*/
-int __highest_present_section_nr;
+unsigned long __highest_present_section_nr;
static void section_mark_present(struct mem_section *ms)
{
- int section_nr = __section_nr(ms);
+ unsigned long section_nr = __section_nr(ms);
if (section_nr > __highest_present_section_nr)
__highest_present_section_nr = section_nr;
@@ -189,7 +189,7 @@ static void section_mark_present(struct mem_section *ms)
ms->section_mem_map |= SECTION_MARKED_PRESENT;
}
-static inline int next_present_section_nr(int section_nr)
+static inline unsigned long next_present_section_nr(unsigned long section_nr)
{
do {
section_nr++;
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 2/6] drivers/base/memory: Use "unsigned long" for block ids
2019-06-14 10:01 [PATCH v1 0/6] mm: Further memory block device cleanups David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 1/6] mm: Section numbers use the type "unsigned long" David Hildenbrand
@ 2019-06-14 10:01 ` David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 3/6] mm: Make register_mem_sect_under_node() static David Hildenbrand
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-06-14 10:01 UTC (permalink / raw)
To: linux-kernel
Cc: David Hildenbrand, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-mm, linux-acpi, Dan Williams, linuxppc-dev, Andrew Morton
Block ids are just shifted section numbers, so let's also use
"unsigned long" for them, too.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
drivers/base/memory.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 5b3a2fd250ba..3ed08e67e64f 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -34,12 +34,12 @@ static DEFINE_MUTEX(mem_sysfs_mutex);
static int sections_per_block;
-static inline int base_memory_block_id(unsigned long section_nr)
+static inline unsigned long base_memory_block_id(unsigned long section_nr)
{
return section_nr / sections_per_block;
}
-static inline int pfn_to_block_id(unsigned long pfn)
+static inline unsigned long pfn_to_block_id(unsigned long pfn)
{
return base_memory_block_id(pfn_to_section_nr(pfn));
}
@@ -587,7 +587,7 @@ int __weak arch_get_memory_phys_device(unsigned long start_pfn)
* A reference for the returned object is held and the reference for the
* hinted object is released.
*/
-static struct memory_block *find_memory_block_by_id(int block_id,
+static struct memory_block *find_memory_block_by_id(unsigned long block_id,
struct memory_block *hint)
{
struct device *hintdev = hint ? &hint->dev : NULL;
@@ -604,7 +604,7 @@ static struct memory_block *find_memory_block_by_id(int block_id,
struct memory_block *find_memory_block_hinted(struct mem_section *section,
struct memory_block *hint)
{
- int block_id = base_memory_block_id(__section_nr(section));
+ unsigned long block_id = base_memory_block_id(__section_nr(section));
return find_memory_block_by_id(block_id, hint);
}
@@ -663,8 +663,8 @@ int register_memory(struct memory_block *memory)
return ret;
}
-static int init_memory_block(struct memory_block **memory, int block_id,
- unsigned long state)
+static int init_memory_block(struct memory_block **memory,
+ unsigned long block_id, unsigned long state)
{
struct memory_block *mem;
unsigned long start_pfn;
@@ -730,8 +730,8 @@ static void unregister_memory(struct memory_block *memory)
*/
int create_memory_block_devices(unsigned long start, unsigned long size)
{
- const int start_block_id = pfn_to_block_id(PFN_DOWN(start));
- int end_block_id = pfn_to_block_id(PFN_DOWN(start + size));
+ const unsigned long start_block_id = pfn_to_block_id(PFN_DOWN(start));
+ unsigned long end_block_id = pfn_to_block_id(PFN_DOWN(start + size));
struct memory_block *mem;
unsigned long block_id;
int ret = 0;
@@ -767,10 +767,10 @@ int create_memory_block_devices(unsigned long start, unsigned long size)
*/
void remove_memory_block_devices(unsigned long start, unsigned long size)
{
- const int start_block_id = pfn_to_block_id(PFN_DOWN(start));
- const int end_block_id = pfn_to_block_id(PFN_DOWN(start + size));
+ const unsigned long start_block_id = pfn_to_block_id(PFN_DOWN(start));
+ const unsigned long end_block_id = pfn_to_block_id(PFN_DOWN(start + size));
struct memory_block *mem;
- int block_id;
+ unsigned long block_id;
if (WARN_ON_ONCE(!IS_ALIGNED(start, memory_block_size_bytes()) ||
!IS_ALIGNED(size, memory_block_size_bytes())))
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 3/6] mm: Make register_mem_sect_under_node() static
2019-06-14 10:01 [PATCH v1 0/6] mm: Further memory block device cleanups David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 1/6] mm: Section numbers use the type "unsigned long" David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 2/6] drivers/base/memory: Use "unsigned long" for block ids David Hildenbrand
@ 2019-06-14 10:01 ` David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 4/6] mm/memory_hotplug: Rename walk_memory_range() and pass start+size instead of pfns David Hildenbrand
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-06-14 10:01 UTC (permalink / raw)
To: linux-kernel
Cc: Keith Busch, David Hildenbrand, Greg Kroah-Hartman,
Rafael J. Wysocki, linux-mm, linux-acpi, Dan Williams,
linuxppc-dev, Andrew Morton, Oscar Salvador
It is only used internally.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Oscar Salvador <osalvador@suse.de>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
drivers/base/node.c | 3 ++-
include/linux/node.h | 7 -------
2 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/base/node.c b/drivers/base/node.c
index 9be88fd05147..e6364e3e3e31 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -752,7 +752,8 @@ static int __ref get_nid_for_pfn(unsigned long pfn)
}
/* register memory section under specified node if it spans that node */
-int register_mem_sect_under_node(struct memory_block *mem_blk, void *arg)
+static int register_mem_sect_under_node(struct memory_block *mem_blk,
+ void *arg)
{
int ret, nid = *(int *)arg;
unsigned long pfn, sect_start_pfn, sect_end_pfn;
diff --git a/include/linux/node.h b/include/linux/node.h
index 548c226966a2..4866f32a02d8 100644
--- a/include/linux/node.h
+++ b/include/linux/node.h
@@ -137,8 +137,6 @@ static inline int register_one_node(int nid)
extern void unregister_one_node(int nid);
extern int register_cpu_under_node(unsigned int cpu, unsigned int nid);
extern int unregister_cpu_under_node(unsigned int cpu, unsigned int nid);
-extern int register_mem_sect_under_node(struct memory_block *mem_blk,
- void *arg);
extern void unregister_memory_block_under_nodes(struct memory_block *mem_blk);
extern int register_memory_node_under_compute_node(unsigned int mem_nid,
@@ -170,11 +168,6 @@ static inline int unregister_cpu_under_node(unsigned int cpu, unsigned int nid)
{
return 0;
}
-static inline int register_mem_sect_under_node(struct memory_block *mem_blk,
- void *arg)
-{
- return 0;
-}
static inline void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
{
}
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 4/6] mm/memory_hotplug: Rename walk_memory_range() and pass start+size instead of pfns
2019-06-14 10:01 [PATCH v1 0/6] mm: Further memory block device cleanups David Hildenbrand
` (2 preceding siblings ...)
2019-06-14 10:01 ` [PATCH v1 3/6] mm: Make register_mem_sect_under_node() static David Hildenbrand
@ 2019-06-14 10:01 ` David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 5/6] mm/memory_hotplug: Move and simplify walk_memory_blocks() David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 6/6] drivers/base/memory.c: Get rid of find_memory_block_hinted() David Hildenbrand
5 siblings, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-06-14 10:01 UTC (permalink / raw)
To: linux-kernel
Cc: Michal Hocko, David Hildenbrand, Wei Yang, linux-mm,
Paul Mackerras, Rashmica Gupta, Dan Williams, Michael Neuling,
linux-acpi, Len Brown, Pavel Tatashin, Anshuman Khandual,
Qian Cai, Thomas Gleixner, Oscar Salvador, Juergen Gross,
Greg Kroah-Hartman, Rafael J. Wysocki, Arun KS, Andrew Morton,
linuxppc-dev
walk_memory_range() was once used to iterate over sections. Now, it
iterates over memory blocks. Rename the function, fixup the
documentation. Also, pass start+size instead of PFNs, which is what most
callers already have at hand. (we'll rework link_mem_sections() most
probably soon)
Follow-up patches wil rework, simplify, and move walk_memory_blocks() to
drivers/base/memory.c.
Note: walk_memory_blocks() only works correctly right now if the
start_pfn is aligned to a section start. This is the case right now,
but we'll generalize the function in a follow up patch so the semantics
match the documentation.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: David Hildenbrand <david@redhat.com>
Cc: Rashmica Gupta <rashmica.g@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Pavel Tatashin <pavel.tatashin@microsoft.com>
Cc: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Michael Neuling <mikey@neuling.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Oscar Salvador <osalvador@suse.de>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Qian Cai <cai@lca.pw>
Cc: Arun KS <arunks@codeaurora.org>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
arch/powerpc/platforms/powernv/memtrace.c | 22 ++++++++++-----------
drivers/acpi/acpi_memhotplug.c | 19 ++++--------------
drivers/base/node.c | 5 +++--
include/linux/memory_hotplug.h | 2 +-
mm/memory_hotplug.c | 24 ++++++++++++-----------
5 files changed, 32 insertions(+), 40 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/memtrace.c b/arch/powerpc/platforms/powernv/memtrace.c
index 5e53c1392d3b..8c82c041afe6 100644
--- a/arch/powerpc/platforms/powernv/memtrace.c
+++ b/arch/powerpc/platforms/powernv/memtrace.c
@@ -70,23 +70,24 @@ static int change_memblock_state(struct memory_block *mem, void *arg)
/* called with device_hotplug_lock held */
static bool memtrace_offline_pages(u32 nid, u64 start_pfn, u64 nr_pages)
{
+ const unsigned long start = PFN_PHYS(start_pfn);
+ const unsigned long size = PFN_PHYS(nr_pages);
u64 end_pfn = start_pfn + nr_pages - 1;
- if (walk_memory_range(start_pfn, end_pfn, NULL,
- check_memblock_online))
+ if (walk_memory_blocks(start, size, NULL, check_memblock_online))
return false;
- walk_memory_range(start_pfn, end_pfn, (void *)MEM_GOING_OFFLINE,
- change_memblock_state);
+ walk_memory_blocks(start, size, (void *)MEM_GOING_OFFLINE,
+ change_memblock_state);
if (offline_pages(start_pfn, nr_pages)) {
- walk_memory_range(start_pfn, end_pfn, (void *)MEM_ONLINE,
- change_memblock_state);
+ walk_memory_blocks(start, size, (void *)MEM_ONLINE,
+ change_memblock_state);
return false;
}
- walk_memory_range(start_pfn, end_pfn, (void *)MEM_OFFLINE,
- change_memblock_state);
+ walk_memory_blocks(start, size, (void *)MEM_OFFLINE,
+ change_memblock_state);
return true;
@@ -242,9 +243,8 @@ static int memtrace_online(void)
*/
if (!memhp_auto_online) {
lock_device_hotplug();
- walk_memory_range(PFN_DOWN(ent->start),
- PFN_UP(ent->start + ent->size - 1),
- NULL, online_mem_block);
+ walk_memory_blocks(ent->start, ent->size, NULL,
+ online_mem_block);
unlock_device_hotplug();
}
diff --git a/drivers/acpi/acpi_memhotplug.c b/drivers/acpi/acpi_memhotplug.c
index db013dc21c02..e294f44a7850 100644
--- a/drivers/acpi/acpi_memhotplug.c
+++ b/drivers/acpi/acpi_memhotplug.c
@@ -155,16 +155,6 @@ static int acpi_memory_check_device(struct acpi_memory_device *mem_device)
return 0;
}
-static unsigned long acpi_meminfo_start_pfn(struct acpi_memory_info *info)
-{
- return PFN_DOWN(info->start_addr);
-}
-
-static unsigned long acpi_meminfo_end_pfn(struct acpi_memory_info *info)
-{
- return PFN_UP(info->start_addr + info->length-1);
-}
-
static int acpi_bind_memblk(struct memory_block *mem, void *arg)
{
return acpi_bind_one(&mem->dev, arg);
@@ -173,9 +163,8 @@ static int acpi_bind_memblk(struct memory_block *mem, void *arg)
static int acpi_bind_memory_blocks(struct acpi_memory_info *info,
struct acpi_device *adev)
{
- return walk_memory_range(acpi_meminfo_start_pfn(info),
- acpi_meminfo_end_pfn(info), adev,
- acpi_bind_memblk);
+ return walk_memory_blocks(info->start_addr, info->length, adev,
+ acpi_bind_memblk);
}
static int acpi_unbind_memblk(struct memory_block *mem, void *arg)
@@ -186,8 +175,8 @@ static int acpi_unbind_memblk(struct memory_block *mem, void *arg)
static void acpi_unbind_memory_blocks(struct acpi_memory_info *info)
{
- walk_memory_range(acpi_meminfo_start_pfn(info),
- acpi_meminfo_end_pfn(info), NULL, acpi_unbind_memblk);
+ walk_memory_blocks(info->start_addr, info->length, NULL,
+ acpi_unbind_memblk);
}
static int acpi_memory_enable_device(struct acpi_memory_device *mem_device)
diff --git a/drivers/base/node.c b/drivers/base/node.c
index e6364e3e3e31..d8c02e65df68 100644
--- a/drivers/base/node.c
+++ b/drivers/base/node.c
@@ -833,8 +833,9 @@ void unregister_memory_block_under_nodes(struct memory_block *mem_blk)
int link_mem_sections(int nid, unsigned long start_pfn, unsigned long end_pfn)
{
- return walk_memory_range(start_pfn, end_pfn, (void *)&nid,
- register_mem_sect_under_node);
+ return walk_memory_blocks(PFN_PHYS(start_pfn),
+ PFN_PHYS(end_pfn - start_pfn), (void *)&nid,
+ register_mem_sect_under_node);
}
#ifdef CONFIG_HUGETLBFS
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 79e0add6a597..d9fffc34949f 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -340,7 +340,7 @@ static inline void __remove_memory(int nid, u64 start, u64 size) {}
#endif /* CONFIG_MEMORY_HOTREMOVE */
extern void __ref free_area_init_core_hotplug(int nid);
-extern int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,
+extern int walk_memory_blocks(unsigned long start, unsigned long size,
void *arg, int (*func)(struct memory_block *, void *));
extern int __add_memory(int nid, u64 start, u64 size);
extern int add_memory(int nid, u64 start, u64 size);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a88c5f334e5a..122a7d31efdd 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1126,8 +1126,7 @@ int __ref add_memory_resource(int nid, struct resource *res)
/* online pages if requested */
if (memhp_auto_online)
- walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1),
- NULL, online_memory_block);
+ walk_memory_blocks(start, size, NULL, online_memory_block);
return ret;
error:
@@ -1665,20 +1664,24 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
#endif /* CONFIG_MEMORY_HOTREMOVE */
/**
- * walk_memory_range - walks through all mem sections in [start_pfn, end_pfn)
- * @start_pfn: start pfn of the memory range
- * @end_pfn: end pfn of the memory range
+ * walk_memory_blocks - walk through all present memory blocks overlapped
+ * by the range [start, start + size)
+ *
+ * @start: start address of the memory range
+ * @size: size of the memory range
* @arg: argument passed to func
- * @func: callback for each memory section walked
+ * @func: callback for each memory block walked
*
- * This function walks through all present mem sections in range
- * [start_pfn, end_pfn) and call func on each mem section.
+ * This function walks through all present memory blocks overlapped by the
+ * range [start, start + size), calling func on each memory block.
*
* Returns the return value of func.
*/
-int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,
+int walk_memory_blocks(unsigned long start, unsigned long size,
void *arg, int (*func)(struct memory_block *, void *))
{
+ const unsigned long start_pfn = PFN_DOWN(start);
+ const unsigned long end_pfn = PFN_UP(start + size - 1);
struct memory_block *mem = NULL;
struct mem_section *section;
unsigned long pfn, section_nr;
@@ -1824,8 +1827,7 @@ static int __ref try_remove_memory(int nid, u64 start, u64 size)
* whether all memory blocks in question are offline and return error
* if this is not the case.
*/
- rc = walk_memory_range(PFN_DOWN(start), PFN_UP(start + size - 1), NULL,
- check_memblock_offlined_cb);
+ rc = walk_memory_blocks(start, size, NULL, check_memblock_offlined_cb);
if (rc)
goto done;
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 5/6] mm/memory_hotplug: Move and simplify walk_memory_blocks()
2019-06-14 10:01 [PATCH v1 0/6] mm: Further memory block device cleanups David Hildenbrand
` (3 preceding siblings ...)
2019-06-14 10:01 ` [PATCH v1 4/6] mm/memory_hotplug: Rename walk_memory_range() and pass start+size instead of pfns David Hildenbrand
@ 2019-06-14 10:01 ` David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 6/6] drivers/base/memory.c: Get rid of find_memory_block_hinted() David Hildenbrand
5 siblings, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-06-14 10:01 UTC (permalink / raw)
To: linux-kernel
Cc: Oscar Salvador, Stephen Rothwell, Michal Hocko, Pavel Tatashin,
David Hildenbrand, mike.travis, Greg Kroah-Hartman,
Rafael J. Wysocki, Wei Yang, linux-mm, linux-acpi, Andrew Banman,
Arun KS, Qian Cai, Dan Williams, linuxppc-dev, Andrew Morton
Let's move walk_memory_blocks() to the place where memory block logic
resides and simplify it. While at it, add a type for the callback function.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: David Hildenbrand <david@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
Cc: Andrew Banman <andrew.banman@hpe.com>
Cc: "mike.travis@hpe.com" <mike.travis@hpe.com>
Cc: Oscar Salvador <osalvador@suse.com>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Wei Yang <richard.weiyang@gmail.com>
Cc: Arun KS <arunks@codeaurora.org>
Cc: Qian Cai <cai@lca.pw>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
drivers/base/memory.c | 42 ++++++++++++++++++++++++++
include/linux/memory.h | 3 ++
include/linux/memory_hotplug.h | 2 --
mm/memory_hotplug.c | 55 ----------------------------------
4 files changed, 45 insertions(+), 57 deletions(-)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 3ed08e67e64f..4f2e2f3b3d78 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -44,6 +44,11 @@ static inline unsigned long pfn_to_block_id(unsigned long pfn)
return base_memory_block_id(pfn_to_section_nr(pfn));
}
+static inline unsigned long phys_to_block_id(unsigned long phys)
+{
+ return pfn_to_block_id(PFN_DOWN(phys));
+}
+
static int memory_subsys_online(struct device *dev);
static int memory_subsys_offline(struct device *dev);
@@ -853,3 +858,40 @@ int __init memory_dev_init(void)
printk(KERN_ERR "%s() failed: %d\n", __func__, ret);
return ret;
}
+
+/**
+ * walk_memory_blocks - walk through all present memory blocks overlapped
+ * by the range [start, start + size)
+ *
+ * @start: start address of the memory range
+ * @size: size of the memory range
+ * @arg: argument passed to func
+ * @func: callback for each memory section walked
+ *
+ * This function walks through all present memory blocks overlapped by the
+ * range [start, start + size), calling func on each memory block.
+ *
+ * In case func() returns an error, walking is aborted and the error is
+ * returned.
+ */
+int walk_memory_blocks(unsigned long start, unsigned long size,
+ void *arg, walk_memory_blocks_func_t func)
+{
+ const unsigned long start_block_id = phys_to_block_id(start);
+ const unsigned long end_block_id = phys_to_block_id(start + size - 1);
+ struct memory_block *mem;
+ unsigned long block_id;
+ int ret = 0;
+
+ for (block_id = start_block_id; block_id <= end_block_id; block_id++) {
+ mem = find_memory_block_by_id(block_id, NULL);
+ if (!mem)
+ continue;
+
+ ret = func(mem, arg);
+ put_device(&mem->dev);
+ if (ret)
+ break;
+ }
+ return ret;
+}
diff --git a/include/linux/memory.h b/include/linux/memory.h
index f26a5417ec5d..b3b388775a30 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -119,6 +119,9 @@ extern int memory_isolate_notify(unsigned long val, void *v);
extern struct memory_block *find_memory_block_hinted(struct mem_section *,
struct memory_block *);
extern struct memory_block *find_memory_block(struct mem_section *);
+typedef int (*walk_memory_blocks_func_t)(struct memory_block *, void *);
+extern int walk_memory_blocks(unsigned long start, unsigned long size,
+ void *arg, walk_memory_blocks_func_t func);
#define CONFIG_MEM_BLOCK_SIZE (PAGES_PER_SECTION<<PAGE_SHIFT)
#endif /* CONFIG_MEMORY_HOTPLUG_SPARSE */
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index d9fffc34949f..475aff8efbf8 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -340,8 +340,6 @@ static inline void __remove_memory(int nid, u64 start, u64 size) {}
#endif /* CONFIG_MEMORY_HOTREMOVE */
extern void __ref free_area_init_core_hotplug(int nid);
-extern int walk_memory_blocks(unsigned long start, unsigned long size,
- void *arg, int (*func)(struct memory_block *, void *));
extern int __add_memory(int nid, u64 start, u64 size);
extern int add_memory(int nid, u64 start, u64 size);
extern int add_memory_resource(int nid, struct resource *resource);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 122a7d31efdd..fc558e9ff939 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1661,62 +1661,7 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
{
return __offline_pages(start_pfn, start_pfn + nr_pages);
}
-#endif /* CONFIG_MEMORY_HOTREMOVE */
-/**
- * walk_memory_blocks - walk through all present memory blocks overlapped
- * by the range [start, start + size)
- *
- * @start: start address of the memory range
- * @size: size of the memory range
- * @arg: argument passed to func
- * @func: callback for each memory block walked
- *
- * This function walks through all present memory blocks overlapped by the
- * range [start, start + size), calling func on each memory block.
- *
- * Returns the return value of func.
- */
-int walk_memory_blocks(unsigned long start, unsigned long size,
- void *arg, int (*func)(struct memory_block *, void *))
-{
- const unsigned long start_pfn = PFN_DOWN(start);
- const unsigned long end_pfn = PFN_UP(start + size - 1);
- struct memory_block *mem = NULL;
- struct mem_section *section;
- unsigned long pfn, section_nr;
- int ret;
-
- for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
- section_nr = pfn_to_section_nr(pfn);
- if (!present_section_nr(section_nr))
- continue;
-
- section = __nr_to_section(section_nr);
- /* same memblock? */
- if (mem)
- if ((section_nr >= mem->start_section_nr) &&
- (section_nr <= mem->end_section_nr))
- continue;
-
- mem = find_memory_block_hinted(section, mem);
- if (!mem)
- continue;
-
- ret = func(mem, arg);
- if (ret) {
- kobject_put(&mem->dev.kobj);
- return ret;
- }
- }
-
- if (mem)
- kobject_put(&mem->dev.kobj);
-
- return 0;
-}
-
-#ifdef CONFIG_MEMORY_HOTREMOVE
static int check_memblock_offlined_cb(struct memory_block *mem, void *arg)
{
int ret = !is_memblock_offlined(mem);
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v1 6/6] drivers/base/memory.c: Get rid of find_memory_block_hinted()
2019-06-14 10:01 [PATCH v1 0/6] mm: Further memory block device cleanups David Hildenbrand
` (4 preceding siblings ...)
2019-06-14 10:01 ` [PATCH v1 5/6] mm/memory_hotplug: Move and simplify walk_memory_blocks() David Hildenbrand
@ 2019-06-14 10:01 ` David Hildenbrand
5 siblings, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-06-14 10:01 UTC (permalink / raw)
To: linux-kernel
Cc: Stephen Rothwell, Pavel Tatashin, David Hildenbrand, mike.travis,
Greg Kroah-Hartman, Rafael J. Wysocki, linux-mm, linux-acpi,
Dan Williams, linuxppc-dev, Andrew Morton
No longer needed, let's remove it.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Pavel Tatashin <pasha.tatashin@soleen.com>
Cc: "mike.travis@hpe.com" <mike.travis@hpe.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
drivers/base/memory.c | 12 +++---------
include/linux/memory.h | 2 --
2 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 4f2e2f3b3d78..42e5a7493fe8 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -606,14 +606,6 @@ static struct memory_block *find_memory_block_by_id(unsigned long block_id,
return to_memory_block(dev);
}
-struct memory_block *find_memory_block_hinted(struct mem_section *section,
- struct memory_block *hint)
-{
- unsigned long block_id = base_memory_block_id(__section_nr(section));
-
- return find_memory_block_by_id(block_id, hint);
-}
-
/*
* For now, we have a linear search to go find the appropriate
* memory_block corresponding to a particular phys_index. If
@@ -624,7 +616,9 @@ struct memory_block *find_memory_block_hinted(struct mem_section *section,
*/
struct memory_block *find_memory_block(struct mem_section *section)
{
- return find_memory_block_hinted(section, NULL);
+ unsigned long block_id = base_memory_block_id(__section_nr(section));
+
+ return find_memory_block_by_id(block_id, hint);
}
static struct attribute *memory_memblk_attrs[] = {
diff --git a/include/linux/memory.h b/include/linux/memory.h
index b3b388775a30..02e633f3ede0 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -116,8 +116,6 @@ void remove_memory_block_devices(unsigned long start, unsigned long size);
extern int memory_dev_init(void);
extern int memory_notify(unsigned long val, void *v);
extern int memory_isolate_notify(unsigned long val, void *v);
-extern struct memory_block *find_memory_block_hinted(struct mem_section *,
- struct memory_block *);
extern struct memory_block *find_memory_block(struct mem_section *);
typedef int (*walk_memory_blocks_func_t)(struct memory_block *, void *);
extern int walk_memory_blocks(unsigned long start, unsigned long size,
--
2.21.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/6] mm: Section numbers use the type "unsigned long"
2019-06-14 10:01 ` [PATCH v1 1/6] mm: Section numbers use the type "unsigned long" David Hildenbrand
@ 2019-06-14 19:00 ` Andrew Morton
2019-06-14 19:34 ` David Hildenbrand
2019-06-15 8:06 ` Christophe Leroy
0 siblings, 2 replies; 12+ messages in thread
From: Andrew Morton @ 2019-06-14 19:00 UTC (permalink / raw)
To: David Hildenbrand
Cc: Stephen Rothwell, Michal Hocko, Mel Gorman, Baoquan He, linux-mm,
Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, Wei Yang,
linux-acpi, Mike Rapoport, Arun KS, Johannes Weiner,
Pavel Tatashin, Dan Williams, linuxppc-dev, Vlastimil Babka,
Oscar Salvador
On Fri, 14 Jun 2019 12:01:09 +0200 David Hildenbrand <david@redhat.com> wrote:
> We are using a mixture of "int" and "unsigned long". Let's make this
> consistent by using "unsigned long" everywhere. We'll do the same with
> memory block ids next.
>
> ...
>
> - int i, ret, section_count = 0;
> + unsigned long i;
>
> ...
>
> - unsigned int i;
> + unsigned long i;
Maybe I did too much fortran back in the day, but I think the
expectation is that a variable called "i" has type "int".
This?
s/unsigned long i/unsigned long section_nr/
--- a/drivers/base/memory.c~mm-section-numbers-use-the-type-unsigned-long-fix
+++ a/drivers/base/memory.c
@@ -131,17 +131,17 @@ static ssize_t phys_index_show(struct de
static ssize_t removable_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
- unsigned long i, pfn;
+ unsigned long section_nr, pfn;
int ret = 1;
struct memory_block *mem = to_memory_block(dev);
if (mem->state != MEM_ONLINE)
goto out;
- for (i = 0; i < sections_per_block; i++) {
- if (!present_section_nr(mem->start_section_nr + i))
+ for (section_nr = 0; section_nr < sections_per_block; section_nr++) {
+ if (!present_section_nr(mem->start_section_nr + section_nr))
continue;
- pfn = section_nr_to_pfn(mem->start_section_nr + i);
+ pfn = section_nr_to_pfn(mem->start_section_nr + section_nr);
ret &= is_mem_section_removable(pfn, PAGES_PER_SECTION);
}
@@ -695,12 +695,12 @@ static int add_memory_block(unsigned lon
{
int ret, section_count = 0;
struct memory_block *mem;
- unsigned long i;
+ unsigned long section_nr;
- for (i = base_section_nr;
- i < base_section_nr + sections_per_block;
- i++)
- if (present_section_nr(i))
+ for (section_nr = base_section_nr;
+ section_nr < base_section_nr + sections_per_block;
+ section_nr++)
+ if (present_section_nr(section_nr))
section_count++;
if (section_count == 0)
@@ -823,7 +823,7 @@ static const struct attribute_group *mem
*/
int __init memory_dev_init(void)
{
- unsigned long i;
+ unsigned long section_nr;
int ret;
int err;
unsigned long block_sz;
@@ -840,9 +840,9 @@ int __init memory_dev_init(void)
* during boot and have been initialized
*/
mutex_lock(&mem_sysfs_mutex);
- for (i = 0; i <= __highest_present_section_nr;
- i += sections_per_block) {
- err = add_memory_block(i);
+ for (section_nr = 0; section_nr <= __highest_present_section_nr;
+ section_nr += sections_per_block) {
+ err = add_memory_block(section_nr);
if (!ret)
ret = err;
}
_
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/6] mm: Section numbers use the type "unsigned long"
2019-06-14 19:00 ` Andrew Morton
@ 2019-06-14 19:34 ` David Hildenbrand
2019-06-15 8:06 ` Christophe Leroy
1 sibling, 0 replies; 12+ messages in thread
From: David Hildenbrand @ 2019-06-14 19:34 UTC (permalink / raw)
To: Andrew Morton
Cc: Stephen Rothwell, Michal Hocko, Mel Gorman, Baoquan He, linux-mm,
Greg Kroah-Hartman, Rafael J. Wysocki, linux-kernel, Wei Yang,
linux-acpi, Mike Rapoport, Arun KS, Johannes Weiner,
Pavel Tatashin, Dan Williams, linuxppc-dev, Vlastimil Babka,
Oscar Salvador
On 14.06.19 21:00, Andrew Morton wrote:
> On Fri, 14 Jun 2019 12:01:09 +0200 David Hildenbrand <david@redhat.com> wrote:
>
>> We are using a mixture of "int" and "unsigned long". Let's make this
>> consistent by using "unsigned long" everywhere. We'll do the same with
>> memory block ids next.
>>
>> ...
>>
>> - int i, ret, section_count = 0;
>> + unsigned long i;
>>
>> ...
>>
>> - unsigned int i;
>> + unsigned long i;
>
> Maybe I did too much fortran back in the day, but I think the
> expectation is that a variable called "i" has type "int".
>
> This?
t460s: ~/git/linux memory_block_devices2 $ git grep "unsigned long i;" |
wc -l
245
t460s: ~/git/linux memory_block_devices2 $ git grep "int i;" | wc -l
26827
Yes ;)
While it makes sense for the second and third occurrence, I think for
the first one it could be confusing (it's not actually a section number
but a counter for sections_per_block).
I see just now that we can avoid converting the first occurrence
completely. So maybe we should drop changing removable_show() from this
patch.
Cheers!
>
>
>
> s/unsigned long i/unsigned long section_nr/
>
> --- a/drivers/base/memory.c~mm-section-numbers-use-the-type-unsigned-long-fix
> +++ a/drivers/base/memory.c
> @@ -131,17 +131,17 @@ static ssize_t phys_index_show(struct de
> static ssize_t removable_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - unsigned long i, pfn;
> + unsigned long section_nr, pfn;
> int ret = 1;
> struct memory_block *mem = to_memory_block(dev);
>
> if (mem->state != MEM_ONLINE)
> goto out;
>
> - for (i = 0; i < sections_per_block; i++) {
> - if (!present_section_nr(mem->start_section_nr + i))
> + for (section_nr = 0; section_nr < sections_per_block; section_nr++) {
> + if (!present_section_nr(mem->start_section_nr + section_nr))
> continue;
> - pfn = section_nr_to_pfn(mem->start_section_nr + i);
> + pfn = section_nr_to_pfn(mem->start_section_nr + section_nr);
> ret &= is_mem_section_removable(pfn, PAGES_PER_SECTION);
> }
>
> @@ -695,12 +695,12 @@ static int add_memory_block(unsigned lon
> {
> int ret, section_count = 0;
> struct memory_block *mem;
> - unsigned long i;
> + unsigned long section_nr;
>
> - for (i = base_section_nr;
> - i < base_section_nr + sections_per_block;
> - i++)
> - if (present_section_nr(i))
> + for (section_nr = base_section_nr;
> + section_nr < base_section_nr + sections_per_block;
> + section_nr++)
> + if (present_section_nr(section_nr))
> section_count++;
>
> if (section_count == 0)
> @@ -823,7 +823,7 @@ static const struct attribute_group *mem
> */
> int __init memory_dev_init(void)
> {
> - unsigned long i;
> + unsigned long section_nr;
> int ret;
> int err;
> unsigned long block_sz;
> @@ -840,9 +840,9 @@ int __init memory_dev_init(void)
> * during boot and have been initialized
> */
> mutex_lock(&mem_sysfs_mutex);
> - for (i = 0; i <= __highest_present_section_nr;
> - i += sections_per_block) {
> - err = add_memory_block(i);
> + for (section_nr = 0; section_nr <= __highest_present_section_nr;
> + section_nr += sections_per_block) {
> + err = add_memory_block(section_nr);
> if (!ret)
> ret = err;
> }
> _
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/6] mm: Section numbers use the type "unsigned long"
2019-06-14 19:00 ` Andrew Morton
2019-06-14 19:34 ` David Hildenbrand
@ 2019-06-15 8:06 ` Christophe Leroy
2019-06-18 1:57 ` Andrew Morton
1 sibling, 1 reply; 12+ messages in thread
From: Christophe Leroy @ 2019-06-15 8:06 UTC (permalink / raw)
To: Andrew Morton, David Hildenbrand
Cc: Stephen Rothwell, Michal Hocko, Pavel Tatashin, linux-acpi,
Baoquan He, Rafael J. Wysocki, Greg Kroah-Hartman, linuxppc-dev,
linux-kernel, Wei Yang, linux-mm, Mike Rapoport, Arun KS,
Johannes Weiner, Dan Williams, Mel Gorman, Vlastimil Babka,
Oscar Salvador
Le 14/06/2019 à 21:00, Andrew Morton a écrit :
> On Fri, 14 Jun 2019 12:01:09 +0200 David Hildenbrand <david@redhat.com> wrote:
>
>> We are using a mixture of "int" and "unsigned long". Let's make this
>> consistent by using "unsigned long" everywhere. We'll do the same with
>> memory block ids next.
>>
>> ...
>>
>> - int i, ret, section_count = 0;
>> + unsigned long i;
>>
>> ...
>>
>> - unsigned int i;
>> + unsigned long i;
>
> Maybe I did too much fortran back in the day, but I think the
> expectation is that a variable called "i" has type "int".
>
> This?
>
>
>
> s/unsigned long i/unsigned long section_nr/
From my point of view you degrade readability by doing that.
section_nr_to_pfn(mem->start_section_nr + section_nr);
Three times the word 'section_nr' in one line, is that worth it ? Gives
me headache.
Codying style says the following, which makes full sense in my opinion:
LOCAL variable names should be short, and to the point. If you have
some random integer loop counter, it should probably be called ``i``.
Calling it ``loop_counter`` is non-productive, if there is no chance of it
being mis-understood.
What about just naming it 'nr' if we want to use something else than 'i' ?
Christophe
>
> --- a/drivers/base/memory.c~mm-section-numbers-use-the-type-unsigned-long-fix
> +++ a/drivers/base/memory.c
> @@ -131,17 +131,17 @@ static ssize_t phys_index_show(struct de
> static ssize_t removable_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> - unsigned long i, pfn;
> + unsigned long section_nr, pfn;
> int ret = 1;
> struct memory_block *mem = to_memory_block(dev);
>
> if (mem->state != MEM_ONLINE)
> goto out;
>
> - for (i = 0; i < sections_per_block; i++) {
> - if (!present_section_nr(mem->start_section_nr + i))
> + for (section_nr = 0; section_nr < sections_per_block; section_nr++) {
> + if (!present_section_nr(mem->start_section_nr + section_nr))
> continue;
> - pfn = section_nr_to_pfn(mem->start_section_nr + i);
> + pfn = section_nr_to_pfn(mem->start_section_nr + section_nr);
> ret &= is_mem_section_removable(pfn, PAGES_PER_SECTION);
> }
>
> @@ -695,12 +695,12 @@ static int add_memory_block(unsigned lon
> {
> int ret, section_count = 0;
> struct memory_block *mem;
> - unsigned long i;
> + unsigned long section_nr;
>
> - for (i = base_section_nr;
> - i < base_section_nr + sections_per_block;
> - i++)
> - if (present_section_nr(i))
> + for (section_nr = base_section_nr;
> + section_nr < base_section_nr + sections_per_block;
> + section_nr++)
> + if (present_section_nr(section_nr))
> section_count++;
>
> if (section_count == 0)
> @@ -823,7 +823,7 @@ static const struct attribute_group *mem
> */
> int __init memory_dev_init(void)
> {
> - unsigned long i;
> + unsigned long section_nr;
> int ret;
> int err;
> unsigned long block_sz;
> @@ -840,9 +840,9 @@ int __init memory_dev_init(void)
> * during boot and have been initialized
> */
> mutex_lock(&mem_sysfs_mutex);
> - for (i = 0; i <= __highest_present_section_nr;
> - i += sections_per_block) {
> - err = add_memory_block(i);
> + for (section_nr = 0; section_nr <= __highest_present_section_nr;
> + section_nr += sections_per_block) {
> + err = add_memory_block(section_nr);
> if (!ret)
> ret = err;
> }
> _
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/6] mm: Section numbers use the type "unsigned long"
2019-06-15 8:06 ` Christophe Leroy
@ 2019-06-18 1:57 ` Andrew Morton
2019-06-18 12:17 ` Michael Ellerman
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Morton @ 2019-06-18 1:57 UTC (permalink / raw)
To: Christophe Leroy
Cc: Stephen Rothwell, Michal Hocko, Pavel Tatashin, linux-acpi,
Baoquan He, David Hildenbrand, Greg Kroah-Hartman,
Rafael J. Wysocki, linux-kernel, Wei Yang, linux-mm,
Mike Rapoport, Arun KS, Johannes Weiner, Dan Williams,
linuxppc-dev, Mel Gorman, Vlastimil Babka, Oscar Salvador
On Sat, 15 Jun 2019 10:06:54 +0200 Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>
>
> Le 14/06/2019 à 21:00, Andrew Morton a écrit :
> > On Fri, 14 Jun 2019 12:01:09 +0200 David Hildenbrand <david@redhat.com> wrote:
> >
> >> We are using a mixture of "int" and "unsigned long". Let's make this
> >> consistent by using "unsigned long" everywhere. We'll do the same with
> >> memory block ids next.
> >>
> >> ...
> >>
> >> - int i, ret, section_count = 0;
> >> + unsigned long i;
> >>
> >> ...
> >>
> >> - unsigned int i;
> >> + unsigned long i;
> >
> > Maybe I did too much fortran back in the day, but I think the
> > expectation is that a variable called "i" has type "int".
> >
> > This?
> >
> >
> >
> > s/unsigned long i/unsigned long section_nr/
>
> From my point of view you degrade readability by doing that.
>
> section_nr_to_pfn(mem->start_section_nr + section_nr);
>
> Three times the word 'section_nr' in one line, is that worth it ? Gives
> me headache.
>
> Codying style says the following, which makes full sense in my opinion:
>
> LOCAL variable names should be short, and to the point. If you have
> some random integer loop counter, it should probably be called ``i``.
> Calling it ``loop_counter`` is non-productive, if there is no chance of it
> being mis-understood.
Well. It did say "integer". Calling an unsigned long `i' is flat out
misleading.
> What about just naming it 'nr' if we want to use something else than 'i' ?
Sure, that works.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v1 1/6] mm: Section numbers use the type "unsigned long"
2019-06-18 1:57 ` Andrew Morton
@ 2019-06-18 12:17 ` Michael Ellerman
0 siblings, 0 replies; 12+ messages in thread
From: Michael Ellerman @ 2019-06-18 12:17 UTC (permalink / raw)
To: Andrew Morton, Christophe Leroy
Cc: Stephen Rothwell, Michal Hocko, Mel Gorman, Baoquan He,
David Hildenbrand, Greg Kroah-Hartman, Rafael J. Wysocki,
Pavel Tatashin, linux-kernel, linux-mm, linux-acpi,
Mike Rapoport, Arun KS, Johannes Weiner, Dan Williams, Wei Yang,
linuxppc-dev, Vlastimil Babka, Oscar Salvador
Andrew Morton <akpm@linux-foundation.org> writes:
> On Sat, 15 Jun 2019 10:06:54 +0200 Christophe Leroy <christophe.leroy@c-s.fr> wrote:
>> Le 14/06/2019 à 21:00, Andrew Morton a écrit :
>> > On Fri, 14 Jun 2019 12:01:09 +0200 David Hildenbrand <david@redhat.com> wrote:
>> >
>> >> We are using a mixture of "int" and "unsigned long". Let's make this
>> >> consistent by using "unsigned long" everywhere. We'll do the same with
>> >> memory block ids next.
>> >>
>> >> ...
>> >>
>> >> - int i, ret, section_count = 0;
>> >> + unsigned long i;
>> >>
>> >> ...
>> >>
>> >> - unsigned int i;
>> >> + unsigned long i;
>> >
>> > Maybe I did too much fortran back in the day, but I think the
>> > expectation is that a variable called "i" has type "int".
...
>> Codying style says the following, which makes full sense in my opinion:
>>
>> LOCAL variable names should be short, and to the point. If you have
>> some random integer loop counter, it should probably be called ``i``.
>> Calling it ``loop_counter`` is non-productive, if there is no chance of it
>> being mis-understood.
>
> Well. It did say "integer". Calling an unsigned long `i' is flat out
> misleading.
I always thought `i` was for loop `index` not `integer`.
But I've never written any Fortran :)
cheers
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-06-18 12:22 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-14 10:01 [PATCH v1 0/6] mm: Further memory block device cleanups David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 1/6] mm: Section numbers use the type "unsigned long" David Hildenbrand
2019-06-14 19:00 ` Andrew Morton
2019-06-14 19:34 ` David Hildenbrand
2019-06-15 8:06 ` Christophe Leroy
2019-06-18 1:57 ` Andrew Morton
2019-06-18 12:17 ` Michael Ellerman
2019-06-14 10:01 ` [PATCH v1 2/6] drivers/base/memory: Use "unsigned long" for block ids David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 3/6] mm: Make register_mem_sect_under_node() static David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 4/6] mm/memory_hotplug: Rename walk_memory_range() and pass start+size instead of pfns David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 5/6] mm/memory_hotplug: Move and simplify walk_memory_blocks() David Hildenbrand
2019-06-14 10:01 ` [PATCH v1 6/6] drivers/base/memory.c: Get rid of find_memory_block_hinted() David Hildenbrand
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).