Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in memblock_is_region_memory()

[Michal Hocko <mhocko@kernel.org>]

On Sun 18-12-16 14:47:49, Wei Yang wrote:
> The base address is already guaranteed to be in the region by
> memblock_search().

First of all the way how the check is removed is the worst possible...
Apart from that it is really not clear to me why checking the base
is not needed. You are mentioning memblock_search but what about other
callers? adjust_range_page_size_mask e.g...

You also didn't mention what is the motivation of this change? What will
work better or why it makes sense in general?

Also this seems to be a general purpose function so it should better
be robust.

> This patch removes the check on base.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

Without a proper justification and with the horrible way how it is done
Nacked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/memblock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 7608bc3..cd85303 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -1615,7 +1615,7 @@ int __init_memblock memblock_is_region_memory(phys_addr_t base, phys_addr_t size
>  
>  	if (idx == -1)
>  		return 0;
> -	return memblock.memory.regions[idx].base <= base &&
> +	return /* memblock.memory.regions[idx].base <= base && */
>  		(memblock.memory.regions[idx].base +
>  		 memblock.memory.regions[idx].size) >= end;
>  }
> -- 
> 2.5.0
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs