Message-ID: <348f3a1f-c7d3-c21f-419d-7acd2e5290b6@nvidia.com>
Date:   Wed, 24 Nov 2021 14:45:59 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.2
Subject: Re: [PATCH] mm: Use BUG_ON instead of if condition followed by BUG
Content-Language: en-US
To:     Andrew Morton <akpm@linux-foundation.org>,
        Matthew Wilcox <willy@infradead.org>
Cc:     cgel.zte@gmail.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, chiminghao <chi.minghao@zte.com.cn>,
        Zeal Robot <zealci@zte.com.cn>
References: <20211124030849.34998-1-chi.minghao@zte.com.cn>
 <YZ483gwnwTysPt0G@casper.infradead.org>
 <20211124142751.e48cdcc3aea9e0ef899f4347@linux-foundation.org>
From:   John Hubbard <jhubbard@nvidia.com>
In-Reply-To: <20211124142751.e48cdcc3aea9e0ef899f4347@linux-foundation.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YzZlbGR4SVE0SHhzZHVuWnBMU1l2Sm4yalV1VnN0aW5UMkZRamlMUmJ5L1E4?=
 =?utf-8?B?c3cxU3o0c1N4YXM4LzhIcDV0OXlneHBhUmw4UHpCOXR6VWltWURmWHhsSGtr?=
 =?utf-8?B?UGpNYk1DZnhuZndHTURnYVRMWVBldlNYMURyV3dIYXo4d3JUZ2pQVU9TQmcr?=
 =?utf-8?B?eGVSVDJYWVV2a1JjOHYrMnM3S3dHQkMxQnp0UzBkZFZydzVLZXFuU2tVQ09w?=
 =?utf-8?B?eFV2MmM5WTBFVDBPQnpuajYzK1J1NEQydjl6a1U4VEl2aTRnL1lMRjZ6TkRF?=
 =?utf-8?B?bU9KMWZBQVIyZjh4aWtPelAyZmlEelFjUVV5a0RrVUdFN0lDVUdrZWpPZDBJ?=
 =?utf-8?B?dzNiYktiUkZ0d2RBVENCVXBxZTd3SEtpY3dSa2tkUEJ2M292bVRUZDFGa21U?=
 =?utf-8?B?N3dGVjVmeW05Qm1yaDE1MmNMdHhWRUJMS0FYbVlSajU0eVlnYWlZM0dmK3Ny?=
 =?utf-8?B?M3ZWUnRIbjlTOTExUGI4ZVlzdmdUVHVvZi9weGlzUlhlM1ZnQnRtQW5Uc2sr?=
 =?utf-8?B?NEJCYmdwcUJCYkcxUEN5UkgxS0Y5SnlBbzdqb1dkRkRQVnpyeEF5Z0dEUzFT?=
 =?utf-8?B?bTFva1VLL3ZYTGVMRFRDSmJ3bWd4eVVrb0hsRWd3VU1tN3NzNm10UUgwNjRZ?=
 =?utf-8?B?am1vVWhmV2U1Z3FrN0RTL01NOWZocWppVENaMlVSeTc1VmlhT0pscWwyTnVn?=
 =?utf-8?B?dzlYc0txTWlUZURiUWN2QmpzUUJxNGFRMTlWbU4wSmVuVzFFcTU2R1E3VmJL?=
 =?utf-8?B?dS9yT0p4UXZpaGkxWGRpdFRWZmZkOVk3Tk4xdmJaY3pxbDJwL3VsWG0vQzZl?=
 =?utf-8?B?TTVzdVBSbmRnRHo4VENTVitYcTVPNUFUVlRMU2JqMEVSVDVDa1JZN3JyQWps?=
 =?utf-8?B?bnZuNWVockp4ZHhKNDFVMnc4YkZWTVhmTk5MQTVadzVXWHlmNkxUZXpBKzBJ?=
 =?utf-8?B?N1BBek5Gb1Eyb0poTWUzYXhySGF0ZStrVkViaEJSSit6dU9PUFNFMUJyUitH?=
 =?utf-8?B?ODU0NWwxTFFCYmZDUzRqeFE0QW8xYjFxZDBmb2doN0h1MktOWDlyZWw2NC9l?=
 =?utf-8?B?ZmZiNmdOeWovcC9zUEJrNVNYcWhxN25GVlR1WXBUSUZCZ3JwMVdpVWdmY1JB?=
 =?utf-8?B?dnR2TW90eEkzeUJKRTF2K1RMSTFnajZEL3ZlczM3aWkrRkYyNHc3b051UVc1?=
 =?utf-8?B?QmxoVk53YVlrNnB2OHUvUjJ1aG1YbFk3c1lOWDF1c2ZFcGxWM0l1WEZMczds?=
 =?utf-8?B?US8vTnZjS1dsRnhPZGdXWi9HcEVpODA4bEM4SEd0R0ZLbU1tWHpoVFNrek1I?=
 =?utf-8?B?RmJXamloT0JsajdsVFJGQ1o0VmxGekNsT1ZzWmNLVmlFTndjOEl6K0VFQWw4?=
 =?utf-8?B?SWtLTG9KdzVEVG82dllqK1ZFbG1DbUpkRThKYzdFSitKcTZpa3ZFWlROS2pN?=
 =?utf-8?B?VmZzTFM0ZzA4aFIyM0w4Q3hTTjBxbVRRcEVGVHk2aGdHb0ZmcHYrK2NqNnRi?=
 =?utf-8?B?Z1JGL093U3hJbFlCZGVXTkdTazFuRjJLMkZxWldva0xueW00aTFkME50THJT?=
 =?utf-8?B?WFRPTStwZGVJY0xiRFhqR3NuaGxlRzBoam1YLzNSbVBhOFI4ODNkTkRoOWQw?=
 =?utf-8?B?QXlrd2NTRWRJbDJaQWxJV3RMbFdaOWdzMENwdWZRUS95Zm02ZFVJYThYNVI5?=
 =?utf-8?B?K3Q1QjFVZHc1MFZxQzlMT1NOdzV4V3RkWE5uTkhtL2FWOUE2Qi9SNVBWWjZK?=
 =?utf-8?B?Z1RBVFNYd0RlRGkybGdacDBsdS95US80djdyY0JPT016MnF2MHl1Z0VlSXJP?=
 =?utf-8?B?dFR1cnZpREtSemVvdWJBWjRPaVlhL3NENVVsRHgrVHF3TWFaUUsvL3M2bjNZ?=
 =?utf-8?B?TW1XRUwxb3FLSERzTWU1WjlBQ1g4d3VHWHcxMUw3UzBRS05DQm05NUZQaFBN?=
 =?utf-8?B?RlRXNGJDVFpPTmhtZVQzMEc4dGxLWHVnd1NUeWRJZ21mbmpYOWRMa1FPSVNE?=
 =?utf-8?B?ZlJqV2w2UU9zV0lHUkJTOFFNUE5lT1N2alZMY1RXS0xOMm05eDdhNERObHA1?=
 =?utf-8?B?ZEMyUGN3Qk5FRWxIa2ZHWDBJR1FtSC9nLzdOWEhMcWY2SWFYNzNUa0tlSFFH?=
 =?utf-8?B?ZXFFZzdjK3hKN3JrR0VqY0pKWnVEUlVMZEtOSVo3V1Y5WkxySHcxRXNNNEYv?=
 =?utf-8?Q?ldUdoyK2uwYbq+r6PMqzDJU=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1aa65abf-7185-4913-2a6b-08d9af9c305c
X-MS-Exchange-CrossTenant-AuthSource: BY5PR12MB4130.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 24 Nov 2021 22:46:01.2075
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WXyC7TR43kohoe3f70Ej+moOF8DU0uZ8KbJlKhIfRhWja16pec79kNaSkG11YdFad82z2CH1CrSCN1f1KCluRA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB2837
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 11/24/21 14:27, Andrew Morton wrote:
> On Wed, 24 Nov 2021 13:23:42 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> 
>> On Wed, Nov 24, 2021 at 03:08:49AM +0000, cgel.zte@gmail.com wrote:
>>> From: chiminghao <chi.minghao@zte.com.cn>
>>>
>>> Fix the following coccinelle report:
>>> ./mm/memory_hotplug.c:2210:2-5:
>>> WARNING  Use BUG_ON instead of if condition followed by BUG.
>>
>> What coccinelle script is reporting this?
>>
>>> -	if (try_remove_memory(start, size))
>>> -		BUG();
>>> +	BUG_ON(try_remove_memory(start, size));
>>
>> I really, really, really do not like this.  For functions with
>> side-effects, this is bad style.  If it's a pure predicate, then
>> sure, but this is bad.
> 
> I don't like it either.  Yes, BUG() is special but it's such dangerous
> practice.  I'd vote to change coccinelle.
> 

Definitely! Or at least use a safer pattern/habit, with just a passive
variable in the BUG_ON() call, approximately like this:

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 852041f6be41..48bd5ff341e7 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -2201,13 +2201,12 @@ static int __ref try_remove_memory(u64 start, u64 size)
*/
void __remove_memory(u64 start, u64 size)
{
-
+       int ret = try_remove_memory(start, size);
/*
* trigger BUG() if some memory is not offlined prior to calling this
* function
*/
-       if (try_remove_memory(start, size))
-               BUG();
+       BUG_ON(ret);
}

/*

...and by the way, while going to type that, I immediately stumbled upon
another pre-existing case of this sort of thing, in try_remove_memory(),
which does this:

static int __ref try_remove_memory(u64 start, u64 size)
{
	struct vmem_altmap mhp_altmap = {};
	struct vmem_altmap *altmap = NULL;
	unsigned long nr_vmemmap_pages;
	int rc = 0, nid = NUMA_NO_NODE;

	BUG_ON(check_hotplug_memory_range(start, size));

...


thanks,
-- 
John Hubbard
NVIDIA