Re: [PATCH] sched,kvm: Fix KVM preempt_notifier usage

[Paolo Bonzini]

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 <gleb@xxxxxxxxxx>

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 <pontus.fuchs@xxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
>  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(&notifier->link, &current->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(&notifier->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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/