Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S933041AbcLSPPb (ORCPT <rfc822;w@1wt.eu>);
        Mon, 19 Dec 2016 10:15:31 -0500
Received: from mail-wm0-f68.google.com ([74.125.82.68]:36529 "EHLO
        mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753608AbcLSPPR (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Mon, 19 Dec 2016 10:15:17 -0500
Date: Mon, 19 Dec 2016 16:15:14 +0100
From: Michal Hocko <mhocko@kernel.org>
To: Wei Yang <richard.weiyang@gmail.com>
Cc: trivial@kernel.org, akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH V2 1/2] mm/memblock.c: trivial code refine in
 memblock_is_region_memory()
Message-ID: <20161219151514.GB5175@dhcp22.suse.cz>
References: <1482072470-26151-1-git-send-email-richard.weiyang@gmail.com>
 <1482072470-26151-2-git-send-email-richard.weiyang@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1482072470-26151-2-git-send-email-richard.weiyang@gmail.com>
User-Agent: Mutt/1.6.0 (2016-04-01)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 1636
Lines: 51

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