From nobody Thu Mar 28 09:58:31 2024 Delivered-To: importer@patchew.org Received-SPF: none (zoho.com: 192.237.175.120 is neither permitted nor denied by domain of lists.xenproject.org) client-ip=192.237.175.120; envelope-from=xen-devel-bounces@lists.xenproject.org; helo=lists.xenproject.org; Authentication-Results: mx.zohomail.com; spf=none (zoho.com: 192.237.175.120 is neither permitted nor denied by domain of lists.xenproject.org) smtp.mailfrom=xen-devel-bounces@lists.xenproject.org ARC-Seal: i=1; a=rsa-sha256; t=1570430192; cv=none; d=zoho.com; s=zohoarc; b=OGcZc5hi0MmBsZ5CoLWxlpR7FVH8HMkz+YpGrQmeLnawibtZNl6ibSg+rpKNhM0UZIKb+wOPycDYueLVbUZbR9nZn9/7cVvbSkbAC39+oPh2BzXqJ8HoqWpZ2KGEyu9D0hJ4b8A0WL6ibzqGKMyVqdT8BR/iLmW4NEniMCLr8as= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=zoho.com; s=zohoarc; t=1570430192; h=Content-Type:Content-Transfer-Encoding:Cc:Date:From:List-Subscribe:List-Post:List-Id:List-Help:List-Unsubscribe:MIME-Version:Message-ID:Sender:Subject:To:ARC-Authentication-Results; bh=gFLkrlM4FhtE1RMzzLwgYkOFVq1haywOxpfB/waPat4=; b=icL1IfPLpF9zJisPTb5uXhFkBStn2zy5isD3zdOJqaXdLteFWs9HcFOEbFlahClQQNuuzoR/nNp6bqzPcamxvbP8JRx+6N0eBG4xf0SKs5mjODt8XBM5/FY2k2tX4cl4VJKXqD/MTc1cGQydNL7za38SK6SFIEzIy+Nvo4k559Q= ARC-Authentication-Results: i=1; mx.zoho.com; spf=none (zoho.com: 192.237.175.120 is neither permitted nor denied by domain of lists.xenproject.org) smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Return-Path: Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) by mx.zohomail.com with SMTPS id 1570430192027474.4622139784991; Sun, 6 Oct 2019 23:36:32 -0700 (PDT) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iHMc9-0000Xk-TF; Mon, 07 Oct 2019 06:35:25 +0000 Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iHMc8-0000Xf-1g for xen-devel@lists.xenproject.org; Mon, 07 Oct 2019 06:35:24 +0000 Received: from mx1.suse.de (unknown [195.135.220.15]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id a32b8984-e8cc-11e9-97a3-12813bfff9fa; Mon, 07 Oct 2019 06:35:22 +0000 (UTC) Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 7B1AEAB98; Mon, 7 Oct 2019 06:35:21 +0000 (UTC) X-Inumbo-ID: a32b8984-e8cc-11e9-97a3-12813bfff9fa X-Virus-Scanned: by amavisd-new at test-mx.suse.de From: Juergen Gross To: xen-devel@lists.xenproject.org Date: Mon, 7 Oct 2019 08:35:19 +0200 Message-Id: <20191007063519.2912-1-jgross@suse.com> X-Mailer: git-send-email 2.16.4 Subject: [Xen-devel] [PATCH v2] xen/sched: let credit scheduler control its timer all alone X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Juergen Gross , Tim Deegan , Stefano Stabellini , Julien Grall , Wei Liu , Konrad Rzeszutek Wilk , George Dunlap , Andrew Cooper , Ian Jackson , Robert VanVossen , Dario Faggioli , Josh Whitehead , Jan Beulich MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" The credit scheduler is the only scheduler with tick_suspend and tick_resume callbacks. Today those callbacks are invoked without being guarded by the scheduler lock which is critical when at the same the cpu those callbacks are active is being moved to or from a cpupool. Crashes like the following are possible due to that race: (XEN) ----[ Xen-4.13.0-8.0.12-d x86_64 debug=3Dy Not tainted ]---- (XEN) CPU: 79 (XEN) RIP: e008:[] set_timer+0x39/0x1f7 (XEN) RFLAGS: 0000000000010002 CONTEXT: hypervisor (XEN) Xen call trace: (XEN) [] set_timer+0x39/0x1f7 (XEN) [] sched_credit.c#csched_tick_resume+0x54/0x59 (XEN) [] sched_tick_resume+0x67/0x86 (XEN) [] mwait-idle.c#mwait_idle+0x32b/0x357 (XEN) [] domain.c#idle_loop+0xa6/0xc2 (XEN) (XEN) Pagetable walk from 0000000000000048: (XEN) L4[0x000] =3D 00000082cfb9c063 ffffffffffffffff (XEN) L3[0x000] =3D 00000082cfb9b063 ffffffffffffffff (XEN) L2[0x000] =3D 00000082cfb9a063 ffffffffffffffff (XEN) L1[0x000] =3D 0000000000000000 ffffffffffffffff (XEN) (XEN) **************************************** (XEN) Panic on CPU 79: (XEN) FATAL PAGE FAULT (XEN) [error_code=3D0000] (XEN) Faulting linear address: 0000000000000048 (XEN) **************************************** The callbacks are used when the cpu is going to or coming from idle in order to allow higher C-states. The credit scheduler knows when it is going to schedule an idle scheduling unit or another one after idle, so it can easily stop or resume the timer itself removing the need to do so via the callback. As this timer handling is done in the main scheduling function the scheduler lock is still held, so the race with cpupool operations can no longer occur. Note that calling the callbacks from schedule_cpu_rm() and schedule_cpu_add() is no longer needed, as the transitions to and from idle in the cpupool with credit active will automatically occur and do the right thing. With the last user of the callbacks gone those can be removed. Suggested-by: George Dunlap Signed-off-by: Juergen Gross Reviewed-by: Dario Faggioli --- V2: - instead of locking handle timer in credit only (George Dunlap) --- xen/common/sched_arinc653.c | 3 --- xen/common/sched_credit.c | 34 ++++++++-------------------------- xen/common/schedule.c | 26 ++------------------------ xen/include/xen/sched-if.h | 15 --------------- 4 files changed, 10 insertions(+), 68 deletions(-) diff --git a/xen/common/sched_arinc653.c b/xen/common/sched_arinc653.c index 45c05c6cd9..565575c326 100644 --- a/xen/common/sched_arinc653.c +++ b/xen/common/sched_arinc653.c @@ -724,9 +724,6 @@ static const struct scheduler sched_arinc653_def =3D { =20 .dump_settings =3D NULL, .dump_cpu_state =3D NULL, - - .tick_suspend =3D NULL, - .tick_resume =3D NULL, }; =20 REGISTER_SCHEDULER(sched_arinc653_def); diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c index 31fdcd6a2f..fbffcf3996 100644 --- a/xen/common/sched_credit.c +++ b/xen/common/sched_credit.c @@ -1831,6 +1831,7 @@ static void csched_schedule( { const unsigned int cur_cpu =3D smp_processor_id(); const unsigned int sched_cpu =3D sched_get_resource_cpu(cur_cpu); + struct csched_pcpu *spc =3D CSCHED_PCPU(cur_cpu); struct list_head * const runq =3D RUNQ(sched_cpu); struct csched_unit * const scurr =3D CSCHED_UNIT(unit); struct csched_private *prv =3D CSCHED_PRIV(ops); @@ -2000,6 +2001,13 @@ out: unit->next_task =3D snext->unit; snext->unit->migrated =3D migrated; =20 + /* Stop credit tick when going to idle, restart it when coming from id= le. */ + if ( !is_idle_unit(unit) && is_idle_unit(unit->next_task) ) + stop_timer(&spc->ticker); + if ( is_idle_unit(unit) && !is_idle_unit(unit->next_task) ) + set_timer(&spc->ticker, now + MICROSECS(prv->tick_period_us) + - now % MICROSECS(prv->tick_period_us) ); + CSCHED_UNIT_CHECK(unit->next_task); } =20 @@ -2237,29 +2245,6 @@ csched_deinit(struct scheduler *ops) } } =20 -static void csched_tick_suspend(const struct scheduler *ops, unsigned int = cpu) -{ - struct csched_pcpu *spc; - - spc =3D CSCHED_PCPU(cpu); - - stop_timer(&spc->ticker); -} - -static void csched_tick_resume(const struct scheduler *ops, unsigned int c= pu) -{ - struct csched_private *prv; - struct csched_pcpu *spc; - uint64_t now =3D NOW(); - - spc =3D CSCHED_PCPU(cpu); - - prv =3D CSCHED_PRIV(ops); - - set_timer(&spc->ticker, now + MICROSECS(prv->tick_period_us) - - now % MICROSECS(prv->tick_period_us) ); -} - static const struct scheduler sched_credit_def =3D { .name =3D "SMP Credit Scheduler", .opt_name =3D "credit", @@ -2295,9 +2280,6 @@ static const struct scheduler sched_credit_def =3D { .switch_sched =3D csched_switch_sched, .alloc_domdata =3D csched_alloc_domdata, .free_domdata =3D csched_free_domdata, - - .tick_suspend =3D csched_tick_suspend, - .tick_resume =3D csched_tick_resume, }; =20 REGISTER_SCHEDULER(sched_credit_def); diff --git a/xen/common/schedule.c b/xen/common/schedule.c index 217fcb09ce..c327c40b92 100644 --- a/xen/common/schedule.c +++ b/xen/common/schedule.c @@ -2871,8 +2871,6 @@ int schedule_cpu_add(unsigned int cpu, struct cpupool= *c) /* _Not_ pcpu_schedule_unlock(): schedule_lock has changed! */ spin_unlock_irqrestore(old_lock, flags); =20 - sched_do_tick_resume(new_ops, cpu); - sr->granularity =3D cpupool_get_granularity(c); sr->cpupool =3D c; /* The cpu is added to a pool, trigger it to go pick up some work */ @@ -2943,8 +2941,6 @@ int schedule_cpu_rm(unsigned int cpu) ASSERT(cpumask_test_cpu(cpu, &cpupool_free_cpus)); ASSERT(!cpumask_test_cpu(cpu, sr->cpupool->cpu_valid)); =20 - sched_do_tick_suspend(old_ops, cpu); - /* See comment in schedule_cpu_add() regarding lock switching. */ old_lock =3D pcpu_schedule_lock_irqsave(cpu, &flags); =20 @@ -3082,32 +3078,14 @@ void schedule_dump(struct cpupool *c) =20 void sched_tick_suspend(void) { - struct scheduler *sched; - unsigned int cpu =3D smp_processor_id(); - - rcu_read_lock(&sched_res_rculock); - - sched =3D get_sched_res(cpu)->scheduler; - sched_do_tick_suspend(sched, cpu); - rcu_idle_enter(cpu); + rcu_idle_enter(smp_processor_id()); rcu_idle_timer_start(); - - rcu_read_unlock(&sched_res_rculock); } =20 void sched_tick_resume(void) { - struct scheduler *sched; - unsigned int cpu =3D smp_processor_id(); - - rcu_read_lock(&sched_res_rculock); - rcu_idle_timer_stop(); - rcu_idle_exit(cpu); - sched =3D get_sched_res(cpu)->scheduler; - sched_do_tick_resume(sched, cpu); - - rcu_read_unlock(&sched_res_rculock); + rcu_idle_exit(smp_processor_id()); } =20 void wait(void) diff --git a/xen/include/xen/sched-if.h b/xen/include/xen/sched-if.h index cd731d7172..29715652bc 100644 --- a/xen/include/xen/sched-if.h +++ b/xen/include/xen/sched-if.h @@ -353,9 +353,6 @@ struct scheduler { struct xen_sysctl_scheduler_op *); void (*dump_settings) (const struct scheduler *); void (*dump_cpu_state) (const struct scheduler *, int); - - void (*tick_suspend) (const struct scheduler *, unsigned in= t); - void (*tick_resume) (const struct scheduler *, unsigned in= t); }; =20 static inline int sched_init(struct scheduler *s) @@ -387,18 +384,6 @@ static inline void sched_dump_cpu_state(const struct s= cheduler *s, int cpu) s->dump_cpu_state(s, cpu); } =20 -static inline void sched_do_tick_suspend(const struct scheduler *s, int cp= u) -{ - if ( s->tick_suspend ) - s->tick_suspend(s, cpu); -} - -static inline void sched_do_tick_resume(const struct scheduler *s, int cpu) -{ - if ( s->tick_resume ) - s->tick_resume(s, cpu); -} - static inline void *sched_alloc_domdata(const struct scheduler *s, struct domain *d) { --=20 2.16.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel