From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1041410AbdDUQkU (ORCPT ); Fri, 21 Apr 2017 12:40:20 -0400 Received: from prod-mail-xrelay07.akamai.com ([23.79.238.175]:42634 "EHLO prod-mail-xrelay07.akamai.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161820AbdDUQkJ (ORCPT ); Fri, 21 Apr 2017 12:40:09 -0400 Subject: Re: [PATCH 1/3] jump_label: Pull get_online_cpus() into generic code To: Peter Zijlstra , rostedt@goodmis.org, tglx@linutronix.de, mingo@kernel.org, bigeasy@linutronix.de References: <20170418103213.089888286@infradead.org> <20170418103422.590118425@infradead.org> Cc: linux-kernel@vger.kernel.org From: Jason Baron Message-ID: <2bc2af64-e014-b56a-4a01-d8d9ea732504@akamai.com> Date: Fri, 21 Apr 2017 12:08:26 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170418103422.590118425@infradead.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/18/2017 06:32 AM, Peter Zijlstra wrote: > This change does two things; it moves the get_online_cpus() call into > generic code, with the aim of later providing some static_key ops that > avoid it. > > And as a side effect it inverts the relation between cpu_hotplug_lock > and jump_label_mutex. > > Signed-off-by: Peter Zijlstra (Intel) > --- ... > @@ -146,6 +154,7 @@ static void __static_key_slow_dec(struct > * returns is unbalanced, because all other static_key_slow_inc() > * instances block while the update is in progress. > */ > + get_online_cpus(); > if (!atomic_dec_and_mutex_lock(&key->enabled, &jump_label_mutex)) { > WARN(atomic_read(&key->enabled) < 0, > "jump label: negative count!\n"); So the get and put can be unbalanced here since the above: 'if (!atomic_dec_and_mutex_lock(&key->enabled, &jump_label_mutex))' is followed by 'return;'. However, I see that the next patch removes this and so things are balanced again... > @@ -159,6 +168,7 @@ static void __static_key_slow_dec(struct > jump_label_update(key); > } > jump_label_unlock(); > + put_online_cpus(); > } > > static void jump_label_update_timeout(struct work_struct *work) > @@ -592,6 +602,10 @@ jump_label_module_notify(struct notifier > > switch (val) { > case MODULE_STATE_COMING: > + /* > + * XXX do we need get_online_cpus() ? the module isn't > + * executable yet, so nothing should be looking at our code. > + */ Since we're just updating the table of places we potentially need to patch, but not actually doing any patching, we should not need get_online_cpus() here...so in attempt to reduce confusion I would remove this. Thanks, -Jason