Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752188AbdC0JLO (ORCPT ); Mon, 27 Mar 2017 05:11:14 -0400 Received: from 92-243-34-74.adsl.nanet.at ([92.243.34.74]:40390 "EHLO mail.osadl.at" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1751834AbdC0JLF (ORCPT ); Mon, 27 Mar 2017 05:11:05 -0400 Date: Mon, 27 Mar 2017 09:10:16 +0000 From: Nicholas Mc Guire To: Peter Zijlstra Cc: Ingo Molnar , Rusty Russell , Nick Piggin , linux-kernel@vger.kernel.org Subject: Re: [PATCH] sched/core: switch to BUG_ON() Message-ID: <20170327091016.GA9937@osadl.at> References: <1490600446-1774-1-git-send-email-der.herr@hofr.at> <20170327082440.6lvb7il66mzdaij5@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170327082440.6lvb7il66mzdaij5@hirez.programming.kicks-ass.net> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1170 Lines: 30 On Mon, Mar 27, 2017 at 10:24:40AM +0200, Peter Zijlstra wrote: > On Mon, Mar 27, 2017 at 09:40:46AM +0200, Nicholas Mc Guire wrote: > > > Found by coccinelle: bugon.cocci > > ./kernel/sched/core.c:5913:2-5: WARNING: Use BUG_ON instead of if condition followed by BUG. > > Right, so I disagree with that here. > > > @@ -5909,8 +5909,8 @@ void __init sched_init_smp(void) > > mutex_unlock(&sched_domains_mutex); > > > > /* Move init over to a non-isolated CPU */ > > - if (set_cpus_allowed_ptr(current, non_isolated_cpus) < 0) > > - BUG(); > > + BUG_ON(set_cpus_allowed_ptr(current, non_isolated_cpus) < 0); > > + > > sched_init_granularity(); > > free_cpumask_var(non_isolated_cpus); > > If the condition were without side effects, that pattern would hold, but > here the function call set_cpus_allowed_ptr() is very much not a pure > function, so wrapping it in a BUG_ON() just fels wrong. Got it - just looked throug core.c and there it seems only side-effect free test-functions are wrapped in BUG_ON() so this change is inconsistent. Other subsystems seem not to follow that logic - might be worth a note somewhere below Documentation/. thx! hofrat