Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754870AbbGCLMY (ORCPT ); Fri, 3 Jul 2015 07:12:24 -0400 Received: from mail-wi0-f179.google.com ([209.85.212.179]:33740 "EHLO mail-wi0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751615AbbGCLMQ (ORCPT ); Fri, 3 Jul 2015 07:12:16 -0400 Subject: Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage To: Peter Zijlstra , Pontus Fuchs , Linus Torvalds References: <558BED42.1030000@gmail.com> <20150625120949.GZ3644@twins.programming.kicks-ass.net> <558BF0E4.50602@gmail.com> <20150625125514.GA3644@twins.programming.kicks-ass.net> Cc: mingo@redhat.com, "linux-kernel@vger.kernel.org" , gleb@kernel.org From: Paolo Bonzini X-Enigmail-Draft-Status: N1110 Message-ID: <55966E0B.7060100@redhat.com> Date: Fri, 3 Jul 2015 13:12:11 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.0.1 MIME-Version: 1.0 In-Reply-To: <20150625125514.GA3644@twins.programming.kicks-ass.net> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3771 Lines: 108 On 25/06/2015 14:55, Peter Zijlstra wrote: > Subject: sched,kvm: Fix KVM preempt_notifier usage > > The preempt-notifier API needs to sleep with the addition of the > static_key, we do however need to hold off preemption while modifying > the preempt notifier list, otherwise a preemption could observe an > inconsistent list state. > > There is no reason to have preemption disabled in the caller, > registering a preempt notifier is a purely task local affair. > > Therefore move the preempt_disable into the functions and change the > callers to a preemptible context. > > Cc: Gleb Natapov Uhm, you forgot at least me and the KVM mailing list. And you didn't even Cc Gleb on the original patch. So nice of you, and it makes me wonder if you even grepped for preempt_notifier_register when you wrote the original patch. Probably not, since you couldn't even be bothered to test the *only* user of preempt notifiers. In fact you shouldn't have just tested the patch on a case _without_ preemption notifiers, you should have also benchmarked the impact that static keys have _with_ preemption notifiers. In a not-really-artificial case (one single-processor guest running on the host), the static key patch adds a static_key_slow_inc on a relatively hot path for KVM, which is not acceptable. So, NACK. > Fixes: 1cde2930e154 ("sched/preempt: Add static_key() to preempt_notifiers") Linus, can you please revert the above patch instead? Paolo > Reported-and-Tested-by: Pontus Fuchs > Signed-off-by: Peter Zijlstra (Intel) > --- > kernel/sched/core.c | 11 +++++++++++ > virt/kvm/kvm_main.c | 5 +++-- > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index b803e1b8ab0c..6169c167ac98 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -2327,7 +2327,12 @@ static struct static_key preempt_notifier_key = STATIC_KEY_INIT_FALSE; > void preempt_notifier_register(struct preempt_notifier *notifier) > { > static_key_slow_inc(&preempt_notifier_key); > + /* > + * Avoid preemption while changing the preempt notifier list. > + */ > + preempt_disable(); > hlist_add_head(¬ifier->link, ¤t->preempt_notifiers); > + preempt_enable(); > } > EXPORT_SYMBOL_GPL(preempt_notifier_register); > > @@ -2339,7 +2344,13 @@ EXPORT_SYMBOL_GPL(preempt_notifier_register); > */ > void preempt_notifier_unregister(struct preempt_notifier *notifier) > { > + /* > + * Avoid preemption while changing the preempt notifier list. > + */ > + preempt_disable(); > hlist_del(¬ifier->link); > + preempt_enable(); > + > static_key_slow_dec(&preempt_notifier_key); > } > EXPORT_SYMBOL_GPL(preempt_notifier_unregister); > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 90977418aeb6..d7aafa0458a0 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -128,8 +128,9 @@ int vcpu_load(struct kvm_vcpu *vcpu) > > if (mutex_lock_killable(&vcpu->mutex)) > return -EINTR; > - cpu = get_cpu(); > preempt_notifier_register(&vcpu->preempt_notifier); > + > + cpu = get_cpu(); > kvm_arch_vcpu_load(vcpu, cpu); > put_cpu(); > return 0; > @@ -139,8 +140,8 @@ void vcpu_put(struct kvm_vcpu *vcpu) > { > preempt_disable(); > kvm_arch_vcpu_put(vcpu); > - preempt_notifier_unregister(&vcpu->preempt_notifier); > preempt_enable(); > + preempt_notifier_unregister(&vcpu->preempt_notifier); > mutex_unlock(&vcpu->mutex); > } > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/