From patchwork Fri Apr 17 13:10:37 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Peter Zijlstra X-Patchwork-Id: 6230701 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id EE3799F1AC for ; Fri, 17 Apr 2015 13:11:09 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0CA1D2027D for ; Fri, 17 Apr 2015 13:11:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id ED34F2037D for ; Fri, 17 Apr 2015 13:11:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753507AbbDQNKw (ORCPT ); Fri, 17 Apr 2015 09:10:52 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:44472 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753183AbbDQNKv (ORCPT ); Fri, 17 Apr 2015 09:10:51 -0400 Received: from 178-85-85-44.dynamic.upc.nl ([178.85.85.44] helo=twins) by bombadil.infradead.org with esmtpsa (Exim 4.80.1 #2 (Red Hat Linux)) id 1Yj62i-0003zC-5N; Fri, 17 Apr 2015 13:10:48 +0000 Received: by twins (Postfix, from userid 1000) id B0B0B10019B21; Fri, 17 Apr 2015 15:10:37 +0200 (CEST) Date: Fri, 17 Apr 2015 15:10:37 +0200 From: Peter Zijlstra To: Paolo Bonzini Cc: torvalds@linux-foundation.org, linux-kernel@vger.kernel.org, gleb@kernel.org, kvm@vger.kernel.org, Ralf Baechle , mtosatti@redhat.com, luto@kernel.org Subject: Re: [GIT PULL] First batch of KVM changes for 4.1 Message-ID: <20150417131037.GG23123@twins.programming.kicks-ass.net> References: <1428678089-16291-1-git-send-email-pbonzini@redhat.com> <20150417085238.GJ17717@twins.programming.kicks-ass.net> <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> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <553100C1.5000408@redhat.com> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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. > > You're missing that this snippet is running in the host, while this > patch is concerned with the guest (paravirt). This doesn't make sense; but it 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. Why should the guest task care about the physical cpu of the vcpu; that's a layering fail if ever there was one. Furthermore, the only thing that migration handler seems to do is increment a variable that is not actually used in that file. > 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. And the fact you're inflicting these costs on _everyone_ for a single x86_64-paravirt case is insane. 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. --- arch/x86/kernel/pvclock.c | 24 ------------------------ include/linux/sched.h | 8 -------- kernel/sched/core.c | 15 --------------- 3 files changed, 47 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c index e5ecd20e72dd..82f116de3835 100644 --- a/arch/x86/kernel/pvclock.c +++ b/arch/x86/kernel/pvclock.c @@ -160,27 +160,6 @@ struct pvclock_vcpu_time_info *pvclock_get_vsyscall_time_info(int cpu) } #ifdef CONFIG_X86_64 -static int pvclock_task_migrate(struct notifier_block *nb, unsigned long l, - void *v) -{ - struct task_migration_notifier *mn = v; - struct pvclock_vsyscall_time_info *pvti; - - pvti = pvclock_get_vsyscall_user_time_info(mn->from_cpu); - - /* this is NULL when pvclock vsyscall is not initialized */ - if (unlikely(pvti == NULL)) - return NOTIFY_DONE; - - pvti->migrate_count++; - - return NOTIFY_DONE; -} - -static struct notifier_block pvclock_migrate = { - .notifier_call = pvclock_task_migrate, -}; - /* * Initialize the generic pvclock vsyscall state. This will allocate * a/some page(s) for the per-vcpu pvclock information, set up a @@ -202,9 +181,6 @@ int __init pvclock_init_vsyscall(struct pvclock_vsyscall_time_info *i, PAGE_KERNEL_VVAR); } - - register_task_migration_notifier(&pvclock_migrate); - return 0; } #endif diff --git a/include/linux/sched.h b/include/linux/sched.h index 3f3308824fa4..0eabab99e126 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -176,14 +176,6 @@ extern void get_iowait_load(unsigned long *nr_waiters, unsigned long *load); extern void calc_global_load(unsigned long ticks); extern void update_cpu_load_nohz(void); -/* Notifier for when a task gets migrated to a new CPU */ -struct task_migration_notifier { - struct task_struct *task; - int from_cpu; - int to_cpu; -}; -extern void register_task_migration_notifier(struct notifier_block *n); - extern unsigned long get_parent_ip(unsigned long addr); extern void dump_cpu_task(int cpu); diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 1d0bc4fe266d..dbfc93d40292 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -1013,13 +1013,6 @@ void check_preempt_curr(struct rq *rq, struct task_struct *p, int flags) rq_clock_skip_update(rq, true); } -static ATOMIC_NOTIFIER_HEAD(task_migration_notifier); - -void register_task_migration_notifier(struct notifier_block *n) -{ - atomic_notifier_chain_register(&task_migration_notifier, n); -} - #ifdef CONFIG_SMP void set_task_cpu(struct task_struct *p, unsigned int new_cpu) { @@ -1050,18 +1043,10 @@ 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) { - struct task_migration_notifier tmn; - 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); - - tmn.task = p; - tmn.from_cpu = task_cpu(p); - tmn.to_cpu = new_cpu; - - atomic_notifier_call_chain(&task_migration_notifier, 0, &tmn); } __set_task_cpu(p, new_cpu);