On Fri, Apr 17, 2015 at 03:38:58PM +0200, Paolo Bonzini wrote:
> On 17/04/2015 15:10, Peter Zijlstra wrote:
> > On Fri, Apr 17, 2015 at 02:46:57PM +0200, Paolo Bonzini wrote:
> >> On 17/04/2015 12:55, Peter Zijlstra wrote:
> >>> Also, it looks like you already do exactly this for other things, look
> >>> at:
> >>>
> >>> kvm_sched_in()
> >>> kvm_arch_vcpu_load()
> >>> if (unlikely(vcpu->cpu != cpu) ... )
> >>>
> >>> So no, I don't believe for one second you need this.
> >
> > This [...] brings us back to where we were last
> > time. There is _0_ justification for this in the patches, that alone is
> > grounds enough to reject it.
>
> Oh, we totally agree on that. I didn't commit that patch, but I already
> said the commit message was insufficient.
>
> > Why should the guest task care about the physical cpu of the vcpu;
> > that's a layering fail if ever there was one.
>
> It's totally within your right to not read the code, but then please
> don't try commenting at it.
>
> This code:
>
> kvm_sched_in()
> kvm_arch_vcpu_load()
> if (unlikely(vcpu->cpu != cpu) ... )
>
> runs in the host. The hypervisor obviously cares if the physical CPU of
> the VCPU changes. It has to tell the source processor (vcpu->cpu) to
> release the VCPU's data structure and only then it can use it in the
> target processor (cpu). No layering violation here.
>
> The task migration notifier runs in the guest, whenever the VCPU of
> a task changes.
>
> > Furthermore, the only thing that migration handler seems to do is
> > increment a variable that is not actually used in that file.
>
> It's used in the vDSO, so you cannot increment it in the file that uses it.
>
> >> And frankly, I think the static key is snake oil. The cost of task
> >> migration in terms of cache misses and TLB misses is in no way
> >> comparable to the cost of filling in a structure on the stack,
> >> dereferencing the head of the notifiers list and seeing that it's NULL.
> >
> > The path this notifier is called from has nothing to do with those
> > costs.
>
> How not? The task is going to incur those costs, it's not like half
> a dozen extra instruction make any difference. But anyway...
>
> > And the fact you're inflicting these costs on _everyone_ for a
> > single x86_64-paravirt case is insane.
>
> ... that's a valid objection. Please look at the patch below.
>
> > I've had enough of this, the below goes into sched/urgent and you can
> > come back with sane patches if and when you're ready.
>
> Oh, please, cut the alpha male crap.
>
> Paolo
>
> ------------------- 8< ----------------
> >From 4eb9d7132e1990c0586f28af3103675416d38974 Mon Sep 17 00:00:00 2001
> From: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Date: Fri, 17 Apr 2015 14:57:34 +0200
> Subject: [PATCH] sched: add CONFIG_TASK_MIGRATION_NOTIFIER
>
> The task migration notifier is only used in x86 paravirt. Make it
> possible to compile it out.
>
> While at it, move some code around to ensure tmn is filled from CPU
> registers.
>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
> arch/x86/Kconfig | 1 +
> init/Kconfig | 3 +++
> kernel/sched/core.c | 9 ++++++++-
> 3 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index d43e7e1c784b..9af252c8698d 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -649,6 +649,7 @@ if HYPERVISOR_GUEST
>
> config PARAVIRT
> bool "Enable paravirtualization code"
> + select TASK_MIGRATION_NOTIFIER
> ---help---
> This changes the kernel so it can modify itself when it is run
> under a hypervisor, potentially improving performance significantly
> diff --git a/init/Kconfig b/init/Kconfig
> index 3b9df1aa35db..891917123338 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -2016,6 +2016,9 @@ source "block/Kconfig"
> config PREEMPT_NOTIFIERS
> bool
>
> +config TASK_MIGRATION_NOTIFIER
> + bool
> +
> config PADATA
> depends on SMP
> bool
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index f9123a82cbb6..c07a53aa543c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1016,12 +1016,14 @@ void check_preempt_curr(struct rq *rq, struct task_struct *p, int flags)
> rq_clock_skip_update(rq, true);
> }
>
> +#ifdef CONFIG_TASK_MIGRATION_NOTIFIER
> static ATOMIC_NOTIFIER_HEAD(task_migration_notifier);
>
> void register_task_migration_notifier(struct notifier_block *n)
> {
> atomic_notifier_chain_register(&task_migration_notifier, n);
> }
> +#endif
>
> #ifdef CONFIG_SMP
> void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
> @@ -1053,18 +1055,23 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
> trace_sched_migrate_task(p, new_cpu);
>
> if (task_cpu(p) != new_cpu) {
> +#ifdef CONFIG_TASK_MIGRATION_NOTIFIER
> struct task_migration_notifier tmn;
> + int from_cpu = task_cpu(p);
> +#endif
>
> if (p->sched_class->migrate_task_rq)
> p->sched_class->migrate_task_rq(p, new_cpu);
> p->se.nr_migrations++;
> perf_sw_event_sched(PERF_COUNT_SW_CPU_MIGRATIONS, 1, 0);
>
> +#ifdef CONFIG_TASK_MIGRATION_NOTIFIER
> tmn.task = p;
> - tmn.from_cpu = task_cpu(p);
> + tmn.from_cpu = from_cpu;
> tmn.to_cpu = new_cpu;
>
> atomic_notifier_call_chain(&task_migration_notifier, 0, &tmn);
> +#endif
> }
>
> __set_task_cpu(p, new_cpu);
> --
> 2.3.5
Paolo,
Please revert the patch -- can fix properly in the host
which also conforms the KVM guest/host documented protocol.
Radim submitted a patch to kvm@ to split
the kvm_write_guest in two with a barrier in between, i think.
I'll review that patch.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
