This series contains cleanups to remove __section_nr().
When CONFIG_SPARSEMEM_EXTREME is enabled, __section_nr() could be
costly since it iterates all section roots to check if the given
mem_section is in its range.
On the other hand, __nr_to_section which converts section_nr to
mem_section can be done in O(1).
The only users of __section_nr() was section_mark_present() and
find_memory_block().
PATCH 1 & 2 changes both functions to use section_nr instead of
mem_section.
PATCH 3 finally removes __section_nr() function.
More details can be found in each changelogs.
Ohhoon Kwon (3):
mm: sparse: pass section_nr to section_mark_present
mm: sparse: pass section_nr to find_memory_block
mm: sparse: remove __section_nr() function
.../platforms/pseries/hotplug-memory.c | 4 +--
drivers/base/memory.c | 4 +--
include/linux/memory.h | 2 +-
include/linux/mmzone.h | 1 -
mm/sparse.c | 35 +++----------------
5 files changed, 9 insertions(+), 37 deletions(-)
--
2.17.1
With CONFIG_SPARSEMEM_EXTREME enabled, __section_nr() which converts
mem_section to section_nr could be costly since it iterates all
section roots to check if the given mem_section is in its range.
On the other hand, __nr_to_section() which converts section_nr to
mem_section can be done in O(1).
Let's pass section_nr instead of mem_section ptr to
section_mark_present() in order to reduce needless iterations.
Signed-off-by: Ohhoon Kwon <[email protected]>
---
mm/sparse.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/mm/sparse.c b/mm/sparse.c
index 55c18aff3e42..4a2700e9a65f 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -186,13 +186,14 @@ void __meminit mminit_validate_memmodel_limits(unsigned long *start_pfn,
* those loops early.
*/
unsigned long __highest_present_section_nr;
-static void section_mark_present(struct mem_section *ms)
+static void section_mark_present(unsigned long section_nr)
{
- unsigned long section_nr = __section_nr(ms);
+ struct mem_section *ms;
if (section_nr > __highest_present_section_nr)
__highest_present_section_nr = section_nr;
+ ms = __nr_to_section(section_nr);
ms->section_mem_map |= SECTION_MARKED_PRESENT;
}
@@ -279,7 +280,7 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
if (!ms->section_mem_map) {
ms->section_mem_map = sparse_encode_early_nid(nid) |
SECTION_IS_ONLINE;
- section_mark_present(ms);
+ section_mark_present(section);
}
}
}
@@ -933,7 +934,7 @@ int __meminit sparse_add_section(int nid, unsigned long start_pfn,
ms = __nr_to_section(section_nr);
set_section_nid(section_nr, nid);
- section_mark_present(ms);
+ section_mark_present(section_nr);
/* Align memmap to section boundary in the subsection case */
if (section_nr_to_pfn(section_nr) != start_pfn)
--
2.17.1
With CONFIG_SPARSEMEM_EXTREME enabled, __section_nr() which converts
mem_section to section_nr could be costly since it iterates all
section roots to check if the given mem_section is in its range.
On the other hand, __nr_to_section() which converts section_nr to
mem_section can be done in O(1).
Let's pass section_nr instead of mem_section ptr to
find_memory_block() in order to reduce needless iterations.
Signed-off-by: Ohhoon Kwon <[email protected]>
---
arch/powerpc/platforms/pseries/hotplug-memory.c | 4 +---
drivers/base/memory.c | 4 ++--
include/linux/memory.h | 2 +-
3 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
index 8377f1f7c78e..905790092e0e 100644
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -211,13 +211,11 @@ static int update_lmb_associativity_index(struct drmem_lmb *lmb)
static struct memory_block *lmb_to_memblock(struct drmem_lmb *lmb)
{
unsigned long section_nr;
- struct mem_section *mem_sect;
struct memory_block *mem_block;
section_nr = pfn_to_section_nr(PFN_DOWN(lmb->base_addr));
- mem_sect = __nr_to_section(section_nr);
- mem_block = find_memory_block(mem_sect);
+ mem_block = find_memory_block(section_nr);
return mem_block;
}
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index d5ffaab3cb61..e31598955cc4 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -578,9 +578,9 @@ static struct memory_block *find_memory_block_by_id(unsigned long block_id)
/*
* Called under device_hotplug_lock.
*/
-struct memory_block *find_memory_block(struct mem_section *section)
+struct memory_block *find_memory_block(unsigned long section_nr)
{
- unsigned long block_id = memory_block_id(__section_nr(section));
+ unsigned long block_id = memory_block_id(section_nr);
return find_memory_block_by_id(block_id);
}
diff --git a/include/linux/memory.h b/include/linux/memory.h
index 97e92e8b556a..d9a0b61cd432 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -90,7 +90,7 @@ int create_memory_block_devices(unsigned long start, unsigned long size,
void remove_memory_block_devices(unsigned long start, unsigned long size);
extern void memory_dev_init(void);
extern int memory_notify(unsigned long val, void *v);
-extern struct memory_block *find_memory_block(struct mem_section *);
+extern struct memory_block *find_memory_block(unsigned long section_nr);
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);
--
2.17.1
__section_nr() was used to convert struct mem_section * to section_nr.
With CONFIG_SPARSEMEM_EXTREME enabled, however, __section_nr() can be
costly since it iterates all section roots to check if the given
mem_section is in its range.
On the other hand, __nr_to_section() which converts section_nr to
mem_section can be done in O(1).
The only users of __section_nr() was section_mark_present() and
find_memory_block().
Since I changed both functions to use section_nr directly in the
preceeding patches, let's remove __section_nr() which has no users.
Signed-off-by: Ohhoon Kwon <[email protected]>
---
include/linux/mmzone.h | 1 -
mm/sparse.c | 26 --------------------------
2 files changed, 27 deletions(-)
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 0d53eba1c383..8931f95cf885 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1321,7 +1321,6 @@ 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 unsigned long __section_nr(struct mem_section *ms);
extern size_t mem_section_usage_size(void);
/*
diff --git a/mm/sparse.c b/mm/sparse.c
index 4a2700e9a65f..1b32d15593e4 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -108,32 +108,6 @@ static inline int sparse_index_init(unsigned long section_nr, int nid)
}
#endif
-#ifdef CONFIG_SPARSEMEM_EXTREME
-unsigned long __section_nr(struct mem_section *ms)
-{
- unsigned long root_nr;
- struct mem_section *root = NULL;
-
- for (root_nr = 0; root_nr < NR_SECTION_ROOTS; root_nr++) {
- root = __nr_to_section(root_nr * SECTIONS_PER_ROOT);
- if (!root)
- continue;
-
- if ((ms >= root) && (ms < (root + SECTIONS_PER_ROOT)))
- break;
- }
-
- VM_BUG_ON(!root);
-
- return (root_nr * SECTIONS_PER_ROOT) + (ms - root);
-}
-#else
-unsigned long __section_nr(struct mem_section *ms)
-{
- return (unsigned long)(ms - mem_section[0]);
-}
-#endif
-
/*
* During early boot, before section_mem_map is used for an actual
* mem_map, we use section_mem_map to store the section's NUMA
--
2.17.1
On Fri 02-07-21 18:41:31, Ohhoon Kwon wrote:
> With CONFIG_SPARSEMEM_EXTREME enabled, __section_nr() which converts
> mem_section to section_nr could be costly since it iterates all
> section roots to check if the given mem_section is in its range.
>
> On the other hand, __nr_to_section() which converts section_nr to
> mem_section can be done in O(1).
>
> Let's pass section_nr instead of mem_section ptr to
> find_memory_block() in order to reduce needless iterations.
Yeah, it seems like the only existing user (maybe there used to be more
in the past - haven't checked) is just doing pointless translation to
comply with the API that doesn't fit it. Just to undo all that work
in find_memory_block
Nice cleanup and potentially even a performance improvement!
> Signed-off-by: Ohhoon Kwon <[email protected]>
Acked-by: Michal Hocko <[email protected]>
> ---
> arch/powerpc/platforms/pseries/hotplug-memory.c | 4 +---
> drivers/base/memory.c | 4 ++--
> include/linux/memory.h | 2 +-
> 3 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/hotplug-memory.c b/arch/powerpc/platforms/pseries/hotplug-memory.c
> index 8377f1f7c78e..905790092e0e 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-memory.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
> @@ -211,13 +211,11 @@ static int update_lmb_associativity_index(struct drmem_lmb *lmb)
> static struct memory_block *lmb_to_memblock(struct drmem_lmb *lmb)
> {
> unsigned long section_nr;
> - struct mem_section *mem_sect;
> struct memory_block *mem_block;
>
> section_nr = pfn_to_section_nr(PFN_DOWN(lmb->base_addr));
> - mem_sect = __nr_to_section(section_nr);
>
> - mem_block = find_memory_block(mem_sect);
> + mem_block = find_memory_block(section_nr);
> return mem_block;
> }
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index d5ffaab3cb61..e31598955cc4 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -578,9 +578,9 @@ static struct memory_block *find_memory_block_by_id(unsigned long block_id)
> /*
> * Called under device_hotplug_lock.
> */
> -struct memory_block *find_memory_block(struct mem_section *section)
> +struct memory_block *find_memory_block(unsigned long section_nr)
> {
> - unsigned long block_id = memory_block_id(__section_nr(section));
> + unsigned long block_id = memory_block_id(section_nr);
>
> return find_memory_block_by_id(block_id);
> }
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index 97e92e8b556a..d9a0b61cd432 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -90,7 +90,7 @@ int create_memory_block_devices(unsigned long start, unsigned long size,
> void remove_memory_block_devices(unsigned long start, unsigned long size);
> extern void memory_dev_init(void);
> extern int memory_notify(unsigned long val, void *v);
> -extern struct memory_block *find_memory_block(struct mem_section *);
> +extern struct memory_block *find_memory_block(unsigned long section_nr);
> 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);
> --
> 2.17.1
--
Michal Hocko
SUSE Labs
On Fri 02-07-21 18:41:32, Ohhoon Kwon wrote:
> __section_nr() was used to convert struct mem_section * to section_nr.
>
> With CONFIG_SPARSEMEM_EXTREME enabled, however, __section_nr() can be
> costly since it iterates all section roots to check if the given
> mem_section is in its range.
>
> On the other hand, __nr_to_section() which converts section_nr to
> mem_section can be done in O(1).
>
> The only users of __section_nr() was section_mark_present() and
> find_memory_block().
>
> Since I changed both functions to use section_nr directly in the
> preceeding patches, let's remove __section_nr() which has no users.
>
> Signed-off-by: Ohhoon Kwon <[email protected]>
I would go with a much shorter changelog. The function is not used
anymore so it can be simply dropped.
Acked-by: Michal Hocko <[email protected]>
> ---
> include/linux/mmzone.h | 1 -
> mm/sparse.c | 26 --------------------------
> 2 files changed, 27 deletions(-)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 0d53eba1c383..8931f95cf885 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1321,7 +1321,6 @@ 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 unsigned long __section_nr(struct mem_section *ms);
> extern size_t mem_section_usage_size(void);
>
> /*
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 4a2700e9a65f..1b32d15593e4 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -108,32 +108,6 @@ static inline int sparse_index_init(unsigned long section_nr, int nid)
> }
> #endif
>
> -#ifdef CONFIG_SPARSEMEM_EXTREME
> -unsigned long __section_nr(struct mem_section *ms)
> -{
> - unsigned long root_nr;
> - struct mem_section *root = NULL;
> -
> - for (root_nr = 0; root_nr < NR_SECTION_ROOTS; root_nr++) {
> - root = __nr_to_section(root_nr * SECTIONS_PER_ROOT);
> - if (!root)
> - continue;
> -
> - if ((ms >= root) && (ms < (root + SECTIONS_PER_ROOT)))
> - break;
> - }
> -
> - VM_BUG_ON(!root);
> -
> - return (root_nr * SECTIONS_PER_ROOT) + (ms - root);
> -}
> -#else
> -unsigned long __section_nr(struct mem_section *ms)
> -{
> - return (unsigned long)(ms - mem_section[0]);
> -}
> -#endif
> -
> /*
> * During early boot, before section_mem_map is used for an actual
> * mem_map, we use section_mem_map to store the section's NUMA
> --
> 2.17.1
--
Michal Hocko
SUSE Labs
On Fri 02-07-21 18:41:30, Ohhoon Kwon wrote:
> With CONFIG_SPARSEMEM_EXTREME enabled, __section_nr() which converts
> mem_section to section_nr could be costly since it iterates all
> section roots to check if the given mem_section is in its range.
>
> On the other hand, __nr_to_section() which converts section_nr to
> mem_section can be done in O(1).
>
> Let's pass section_nr instead of mem_section ptr to
> section_mark_present() in order to reduce needless iterations.
It is indeed wasteful to spend time on something that is already known.
Both callers have already determined both the section number and the
section so why not just pass both to section_mark_present?
One could argue that from an API point of view it is a bad practice to
have two indipendent arguments referring to the same underlying object,
and I would agree, but this is not really a something that has a wider
use so it is more of a helper. Maybe we want to make that more explicit
via __ prefix.
> Signed-off-by: Ohhoon Kwon <[email protected]>
> ---
> mm/sparse.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 55c18aff3e42..4a2700e9a65f 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -186,13 +186,14 @@ void __meminit mminit_validate_memmodel_limits(unsigned long *start_pfn,
> * those loops early.
> */
> unsigned long __highest_present_section_nr;
> -static void section_mark_present(struct mem_section *ms)
> +static void section_mark_present(unsigned long section_nr)
> {
> - unsigned long section_nr = __section_nr(ms);
> + struct mem_section *ms;
>
> if (section_nr > __highest_present_section_nr)
> __highest_present_section_nr = section_nr;
>
> + ms = __nr_to_section(section_nr);
> ms->section_mem_map |= SECTION_MARKED_PRESENT;
> }
>
> @@ -279,7 +280,7 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
> if (!ms->section_mem_map) {
> ms->section_mem_map = sparse_encode_early_nid(nid) |
> SECTION_IS_ONLINE;
> - section_mark_present(ms);
> + section_mark_present(section);
> }
> }
> }
> @@ -933,7 +934,7 @@ int __meminit sparse_add_section(int nid, unsigned long start_pfn,
>
> ms = __nr_to_section(section_nr);
> set_section_nid(section_nr, nid);
> - section_mark_present(ms);
> + section_mark_present(section_nr);
>
> /* Align memmap to section boundary in the subsection case */
> if (section_nr_to_pfn(section_nr) != start_pfn)
> --
> 2.17.1
--
Michal Hocko
SUSE Labs
On Fri, Jul 02, 2021 at 06:41:29PM +0900, Ohhoon Kwon wrote:
> This series contains cleanups to remove __section_nr().
>
> When CONFIG_SPARSEMEM_EXTREME is enabled, __section_nr() could be
> costly since it iterates all section roots to check if the given
> mem_section is in its range.
>
> On the other hand, __nr_to_section which converts section_nr to
> mem_section can be done in O(1).
>
> The only users of __section_nr() was section_mark_present() and
> find_memory_block().
>
> PATCH 1 & 2 changes both functions to use section_nr instead of
> mem_section.
> PATCH 3 finally removes __section_nr() function.
>
> More details can be found in each changelogs.
>
> Ohhoon Kwon (3):
> mm: sparse: pass section_nr to section_mark_present
> mm: sparse: pass section_nr to find_memory_block
> mm: sparse: remove __section_nr() function
>
> .../platforms/pseries/hotplug-memory.c | 4 +--
> drivers/base/memory.c | 4 +--
> include/linux/memory.h | 2 +-
> include/linux/mmzone.h | 1 -
> mm/sparse.c | 35 +++----------------
> 5 files changed, 9 insertions(+), 37 deletions(-)
>
> --
> 2.17.1
For the series:
Acked-by: Mike Rapoport <[email protected]>
--
Sincerely yours,
Mike.
On 02.07.21 11:41, Ohhoon Kwon wrote:
> With CONFIG_SPARSEMEM_EXTREME enabled, __section_nr() which converts
> mem_section to section_nr could be costly since it iterates all
> section roots to check if the given mem_section is in its range.
>
> On the other hand, __nr_to_section() which converts section_nr to
> mem_section can be done in O(1).
>
> Let's pass section_nr instead of mem_section ptr to
> section_mark_present() in order to reduce needless iterations.
>
> Signed-off-by: Ohhoon Kwon <[email protected]>
> ---
> mm/sparse.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 55c18aff3e42..4a2700e9a65f 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -186,13 +186,14 @@ void __meminit mminit_validate_memmodel_limits(unsigned long *start_pfn,
> * those loops early.
> */
> unsigned long __highest_present_section_nr;
> -static void section_mark_present(struct mem_section *ms)
> +static void section_mark_present(unsigned long section_nr)
> {
> - unsigned long section_nr = __section_nr(ms);
> + struct mem_section *ms;
>
> if (section_nr > __highest_present_section_nr)
> __highest_present_section_nr = section_nr;
>
> + ms = __nr_to_section(section_nr);
> ms->section_mem_map |= SECTION_MARKED_PRESENT;
> }
>
> @@ -279,7 +280,7 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
> if (!ms->section_mem_map) {
> ms->section_mem_map = sparse_encode_early_nid(nid) |
> SECTION_IS_ONLINE;
> - section_mark_present(ms);
> + section_mark_present(section);
> }
> }
> }
> @@ -933,7 +934,7 @@ int __meminit sparse_add_section(int nid, unsigned long start_pfn,
>
> ms = __nr_to_section(section_nr);
> set_section_nid(section_nr, nid);
> - section_mark_present(ms);
> + section_mark_present(section_nr);
>
> /* Align memmap to section boundary in the subsection case */
> if (section_nr_to_pfn(section_nr) != start_pfn)
>
Fine for me either like this, or as Michal suggested. It's an internal
helper either way.
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 02.07.21 11:41, Ohhoon Kwon wrote:
> With CONFIG_SPARSEMEM_EXTREME enabled, __section_nr() which converts
> mem_section to section_nr could be costly since it iterates all
> section roots to check if the given mem_section is in its range.
>
> On the other hand, __nr_to_section() which converts section_nr to
> mem_section can be done in O(1).
>
> Let's pass section_nr instead of mem_section ptr to
> find_memory_block() in order to reduce needless iterations.
>
> Signed-off-by: Ohhoon Kwon <[email protected]>
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb
On 02.07.21 14:18, Michal Hocko wrote:
> On Fri 02-07-21 18:41:32, Ohhoon Kwon wrote:
>> __section_nr() was used to convert struct mem_section * to section_nr.
>>
>> With CONFIG_SPARSEMEM_EXTREME enabled, however, __section_nr() can be
>> costly since it iterates all section roots to check if the given
>> mem_section is in its range.
>>
>> On the other hand, __nr_to_section() which converts section_nr to
>> mem_section can be done in O(1).
>>
>> The only users of __section_nr() was section_mark_present() and
>> find_memory_block().
>>
>> Since I changed both functions to use section_nr directly in the
>> preceeding patches, let's remove __section_nr() which has no users.
>>
>> Signed-off-by: Ohhoon Kwon <[email protected]>
>
> I would go with a much shorter changelog. The function is not used
> anymore so it can be simply dropped.
> Acked-by: Michal Hocko <[email protected]>
Agreed, also avoid the use of "I" in patches.
Use something like
"As the last users of __section_nr() are gone, let's remove now unused
__section_nr()."
Reviewed-by: David Hildenbrand <[email protected]>
--
Thanks,
David / dhildenb