From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [GIT PULL] First batch of KVM changes for 4.1 Date: Fri, 17 Apr 2015 17:18:41 -0300 Message-ID: <20150417201841.GA31302@amt.cnet> References: <20150417091745.GA24151@twins.programming.kicks-ass.net> <5530DBED.5080508@redhat.com> <20150417103654.GE5029@twins.programming.kicks-ass.net> <5530E28F.2030401@redhat.com> <20150417105506.GF5029@twins.programming.kicks-ass.net> <553100C1.5000408@redhat.com> <20150417131037.GG23123@twins.programming.kicks-ass.net> <55310CF2.6070107@redhat.com> <20150417190146.GA24395@amt.cnet> <55316598.908@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Peter Zijlstra , torvalds@linux-foundation.org, linux-kernel@vger.kernel.org, gleb@kernel.org, kvm@vger.kernel.org, Ralf Baechle , luto@kernel.org To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:60596 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752627AbbDQUTY (ORCPT ); Fri, 17 Apr 2015 16:19:24 -0400 Content-Disposition: inline In-Reply-To: <55316598.908@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Apr 17, 2015 at 09:57:12PM +0200, Paolo Bonzini wrote: > > > >> From 4eb9d7132e1990c0586f28af3103675416d38974 Mon Sep 17 00:00:00 2001 > >> From: Paolo Bonzini > >> 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 > >> --- > >> 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. > > You're thinking of > http://article.gmane.org/gmane.linux.kernel.stable/129187, but see > Andy's reply: > > > > > I think there are at least two ways that would work: > > > > a) If KVM incremented version as advertised: > > > > cpu = getcpu(); > > pvti = pvti for cpu; > > > > ver1 = pvti->version; > > check stable bit; > > rdtsc_barrier, rdtsc, read scale, shift, etc. > > if (getcpu() != cpu) retry; > > if (pvti->version != ver1) retry; > > > > I think this is safe because, we're guaranteed that there was an > > interval (between the two version reads) in which the vcpu we think > > we're on was running and the kvmclock data was valid and marked > > stable, and we know that the tsc we read came from that interval. > > > > Note: rdtscp isn't needed. If we're stable, is makes no difference > > which cpu's tsc we actually read. > > > > b) If version remains buggy but we use this migrations_from hack: > > > > cpu = getcpu(); > > pvti = pvti for cpu; > > m1 = pvti->migrations_from; > > barrier(); > > > > ver1 = pvti->version; > > check stable bit; > > rdtsc_barrier, rdtsc, read scale, shift, etc. > > if (getcpu() != cpu) retry; > > if (pvti->version != ver1) retry; /* probably not really needed */ > > > > barrier(); > > if (pvti->migrations_from != m1) retry; > > > > This is just like (a), except that we're using a guest kernel hack to > > ensure that no one migrated off the vcpu during the version-protected > > critical section and that we were, in fact, on that vcpu at some point > > during that critical section. Once we've ensured that we were on > > pvti's associated vcpu for the entire time we were reading it, then we > > are protected by the existing versioning in the host. > > (a) is not going to happen until 4.2, and there are too many buggy hosts > around so we'd have to define new ABI that lets the guest distinguish a > buggy host from a fixed one. > > (b) works now, is not invasive, and I still maintain that the cost is > negligible. I'm going to run for a while with CONFIG_SCHEDSTATS to see > how often you have a migration. > > Anyhow if the task migration notifier is reverted we have to disable the > whole vsyscall support altogether. The bug which this is fixing is very rare, have no memory of a report. In fact, its even difficult to create a synthetic reproducer. You need: 1) update of kvmclock data structure (happens once every 5 minutes). 2) migration of task from vcpu1 to vcpu2 back to vcpu1. 3) a data race between kvm_write_guest (string copy) and 2 above. At the same time.