Re: [PATCH] mm, sparse: remove check with __highest_present_section_nr in for_each_present_section_nr()

[Michal Hocko <mhocko@suse.com>]

On Tue 11-12-18 11:51:28, Wei Yang wrote:
> A valid present section number is in [0, __highest_present_section_nr].
> And the return value of next_present_section_nr() meets this
> requirement. This means it is not necessary to check it with
> __highest_present_section_nr again in for_each_present_section_nr().
> 
> Since we pass an unsigned long *section_nr* to
> for_each_present_section_nr(), we need to cast it to int before
> comparing.

Why do we want this patch? Is it an improvement? If yes, it is
performance visible change or does it make the code easier to maintain?

To me at least the later seems dubious to be honest because it adds a
non-obvious dependency of the terminal condition to the
next_present_section_nr implementation and that might turn out error
prone.

> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> ---
>  mm/sparse.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index a4fdbcb21514..9eaa8f98a3d2 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -197,8 +197,7 @@ static inline int next_present_section_nr(int section_nr)
>  }
>  #define for_each_present_section_nr(start, section_nr)		\
>  	for (section_nr = next_present_section_nr(start-1);	\
> -	     ((section_nr >= 0) &&				\
> -	      (section_nr <= __highest_present_section_nr));	\
> +	     (int)section_nr >= 0;				\
>  	     section_nr = next_present_section_nr(section_nr))
>  
>  static inline unsigned long first_present_section_nr(void)
> -- 
> 2.15.1
> 

-- 
Michal Hocko
SUSE Labs