Re: [PATCH 1/3] mm: sparse: pass section_nr to section_mark_present

[Michal Hocko]

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 <ohoono.kwon@xxxxxxxxxxx>
> ---
>  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