| Message ID | 20191023032254.159510-1-maowenan@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Select SCHED_INFO before SCHEDSTATS | expand |
On Wed, Oct 23, 2019 at 04:22:54AM +0100, Mao Wenan wrote: > If KVM=y, it will select SCHEDSTATS, below erros can > be seen: > kernel/sched/stats.h: In function rq_sched_info_arrive: > kernel/sched/stats.h:12:20: error: struct sched_info > has no member named run_delay > rq->rq_sched_info.run_delay += delta; > ^ > kernel/sched/stats.h:13:20: error: struct sched_info > has no member named pcount > rq->rq_sched_info.pcount++; > ^ > kernel/sched/stats.h: In function rq_sched_info_dequeued: > kernel/sched/stats.h:31:20: error: struct sched_info has > no member named run_delay > rq->rq_sched_info.run_delay += delta; > > These are because CONFIG_SCHED_INFO is not set, This patch > is to select SCHED_INFO before SCHEDSTATS. It looks like I didn't spot this because when DEBUG_KERNEL is enabled then KVM selects SCHEDSTATS, which selects SCHED_INFO. Thanks for spotting this. > > Fixes: 8564d6372a7d ("KVM: arm64: Support stolen time reporting via shared structure") > Signed-off-by: Mao Wenan <maowenan@huawei.com> Reviewed-by: Steven Price <steven.price@arm.com> > --- > arch/arm64/kvm/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig > index d8b88e4..3c46eac 100644 > --- a/arch/arm64/kvm/Kconfig > +++ b/arch/arm64/kvm/Kconfig > @@ -39,6 +39,7 @@ config KVM > select IRQ_BYPASS_MANAGER > select HAVE_KVM_IRQ_BYPASS > select HAVE_KVM_VCPU_RUN_PID_CHANGE > + select SCHED_INFO > select SCHEDSTATS > ---help--- > Support hosting virtualized guest machines. > -- > 2.7.4 >
On 2019-10-23 04:22, Mao Wenan wrote: > If KVM=y, it will select SCHEDSTATS, below erros can > be seen: > kernel/sched/stats.h: In function rq_sched_info_arrive: > kernel/sched/stats.h:12:20: error: struct sched_info > has no member named run_delay > rq->rq_sched_info.run_delay += delta; > ^ > kernel/sched/stats.h:13:20: error: struct sched_info > has no member named pcount > rq->rq_sched_info.pcount++; > ^ > kernel/sched/stats.h: In function rq_sched_info_dequeued: > kernel/sched/stats.h:31:20: error: struct sched_info has > no member named run_delay > rq->rq_sched_info.run_delay += delta; > > These are because CONFIG_SCHED_INFO is not set, This patch > is to select SCHED_INFO before SCHEDSTATS. > > Fixes: 8564d6372a7d ("KVM: arm64: Support stolen time reporting via > shared structure") > Signed-off-by: Mao Wenan <maowenan@huawei.com> > --- > arch/arm64/kvm/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig > index d8b88e4..3c46eac 100644 > --- a/arch/arm64/kvm/Kconfig > +++ b/arch/arm64/kvm/Kconfig > @@ -39,6 +39,7 @@ config KVM > select IRQ_BYPASS_MANAGER > select HAVE_KVM_IRQ_BYPASS > select HAVE_KVM_VCPU_RUN_PID_CHANGE > + select SCHED_INFO > select SCHEDSTATS > ---help--- > Support hosting virtualized guest machines. SCHEDSTATS is really an odd choice. Here's what I get after disabling DEBUG_KERNEL (from defconfig): WARNING: unmet direct dependencies detected for SCHEDSTATS Depends on [n]: DEBUG_KERNEL [=n] && PROC_FS [=y] Selected by [y]: - KVM [=y] && VIRTUALIZATION [=y] && OF [=y] WARNING: unmet direct dependencies detected for SCHEDSTATS Depends on [n]: DEBUG_KERNEL [=n] && PROC_FS [=y] Selected by [y]: - KVM [=y] && VIRTUALIZATION [=y] && OF [=y] WARNING: unmet direct dependencies detected for SCHEDSTATS Depends on [n]: DEBUG_KERNEL [=n] && PROC_FS [=y] Selected by [y]: - KVM [=y] && VIRTUALIZATION [=y] && OF [=y] So clearly SCHEDSTATS isn't meant to be selected on its own. We can either just select SCHED_INFO (which *nobody else does*), or go the full x86 way which selects TASK_DELAY_ACCT (and thus depends on NET && MULTIUSER). My gut feeling is that we shouldn't deviate too much from x86... Thoughts? M.
On 23/10/2019 17:51, Marc Zyngier wrote: > On 2019-10-23 04:22, Mao Wenan wrote: >> If KVM=y, it will select SCHEDSTATS, below erros can >> be seen: >> kernel/sched/stats.h: In function rq_sched_info_arrive: >> kernel/sched/stats.h:12:20: error: struct sched_info >> has no member named run_delay >> rq->rq_sched_info.run_delay += delta; >> ^ >> kernel/sched/stats.h:13:20: error: struct sched_info >> has no member named pcount >> rq->rq_sched_info.pcount++; >> ^ >> kernel/sched/stats.h: In function rq_sched_info_dequeued: >> kernel/sched/stats.h:31:20: error: struct sched_info has >> no member named run_delay >> rq->rq_sched_info.run_delay += delta; >> >> These are because CONFIG_SCHED_INFO is not set, This patch >> is to select SCHED_INFO before SCHEDSTATS. >> >> Fixes: 8564d6372a7d ("KVM: arm64: Support stolen time reporting via >> shared structure") >> Signed-off-by: Mao Wenan <maowenan@huawei.com> >> --- >> arch/arm64/kvm/Kconfig | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig >> index d8b88e4..3c46eac 100644 >> --- a/arch/arm64/kvm/Kconfig >> +++ b/arch/arm64/kvm/Kconfig >> @@ -39,6 +39,7 @@ config KVM >> select IRQ_BYPASS_MANAGER >> select HAVE_KVM_IRQ_BYPASS >> select HAVE_KVM_VCPU_RUN_PID_CHANGE >> + select SCHED_INFO >> select SCHEDSTATS >> ---help--- >> Support hosting virtualized guest machines. > > SCHEDSTATS is really an odd choice. Here's what I get after disabling > DEBUG_KERNEL (from defconfig): > > WARNING: unmet direct dependencies detected for SCHEDSTATS > Depends on [n]: DEBUG_KERNEL [=n] && PROC_FS [=y] > Selected by [y]: > - KVM [=y] && VIRTUALIZATION [=y] && OF [=y] > > WARNING: unmet direct dependencies detected for SCHEDSTATS > Depends on [n]: DEBUG_KERNEL [=n] && PROC_FS [=y] > Selected by [y]: > - KVM [=y] && VIRTUALIZATION [=y] && OF [=y] > > WARNING: unmet direct dependencies detected for SCHEDSTATS > Depends on [n]: DEBUG_KERNEL [=n] && PROC_FS [=y] > Selected by [y]: > - KVM [=y] && VIRTUALIZATION [=y] && OF [=y] > > So clearly SCHEDSTATS isn't meant to be selected on its own. > > We can either just select SCHED_INFO (which *nobody else does*), or go > the full x86 way which selects TASK_DELAY_ACCT (and thus depends on > NET && MULTIUSER). My gut feeling is that we shouldn't deviate too much > from x86... > > Thoughts? I suspect you're right - TASK_DELAY_ACCT seems to be the closest to what we need. SCHEDSTATS has the "advantage" of forcing sched_info_on() to return true - preventing it from being disabled. But we clearly don't want to require CONFIG_DEBUG_KERNEL for CONFIG_KVM. The next best is CONFIG_TASK_DELAY_ACCT which enables sched_info_on() unless "nodelayacct" is specified on the cmdline. It seems reasonable that the cmdline option might break stolen time. So let's just copy x86: -----8<----- From 915893f5c57241cc29d90769b3f720a6135277d7 Mon Sep 17 00:00:00 2001 From: Steven Price <steven.price@arm.com> Date: Thu, 24 Oct 2019 12:14:36 +0100 Subject: [PATCH] KVM: arm64: Select TASK_DELAY_ACCT rather than SCHEDSTATS SCHEDSTATS requires DEBUG_KERNEL (and PROC_FS) and therefore isn't a good choice for enabling the scheduling statistics required for stolen time. Instead match the x86 configuration and select TASK_DELAY_ACCT. This adds the dependencies of NET && MULTIUSER for arm64 KVM. Suggested-by: Marc Zyngier <maz@kernel.org> Fixes: 8564d6372a7d ("KVM: arm64: Support stolen time reporting via shared structure") Signed-off-by: Steven Price <steven.price@arm.com> --- arch/arm64/kvm/Kconfig | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig index d8b88e40d223..1ffb300e2d92 100644 --- a/arch/arm64/kvm/Kconfig +++ b/arch/arm64/kvm/Kconfig @@ -21,6 +21,8 @@ if VIRTUALIZATION config KVM bool "Kernel-based Virtual Machine (KVM) support" depends on OF + # for TASKSTATS/TASK_DELAY_ACCT: + depends on NET && MULTIUSER select MMU_NOTIFIER select PREEMPT_NOTIFIERS select HAVE_KVM_CPU_RELAX_INTERCEPT @@ -39,7 +41,7 @@ config KVM select IRQ_BYPASS_MANAGER select HAVE_KVM_IRQ_BYPASS select HAVE_KVM_VCPU_RUN_PID_CHANGE - select SCHEDSTATS + select TASK_DELAY_ACCT ---help--- Support hosting virtualized guest machines. We don't support KVM with 16K page tables yet, due to the multiple
On 2019-10-24 12:22, Steven Price wrote: [...] > From 915893f5c57241cc29d90769b3f720a6135277d7 Mon Sep 17 00:00:00 > 2001 > From: Steven Price <steven.price@arm.com> > Date: Thu, 24 Oct 2019 12:14:36 +0100 > Subject: [PATCH] KVM: arm64: Select TASK_DELAY_ACCT rather than > SCHEDSTATS > > SCHEDSTATS requires DEBUG_KERNEL (and PROC_FS) and therefore isn't a > good choice for enabling the scheduling statistics required for > stolen > time. > > Instead match the x86 configuration and select TASK_DELAY_ACCT. This > adds the dependencies of NET && MULTIUSER for arm64 KVM. > > Suggested-by: Marc Zyngier <maz@kernel.org> > Fixes: 8564d6372a7d ("KVM: arm64: Support stolen time reporting via > shared structure") > Signed-off-by: Steven Price <steven.price@arm.com> > --- > arch/arm64/kvm/Kconfig | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig > index d8b88e40d223..1ffb300e2d92 100644 > --- a/arch/arm64/kvm/Kconfig > +++ b/arch/arm64/kvm/Kconfig > @@ -21,6 +21,8 @@ if VIRTUALIZATION > config KVM > bool "Kernel-based Virtual Machine (KVM) support" > depends on OF > + # for TASKSTATS/TASK_DELAY_ACCT: > + depends on NET && MULTIUSER > select MMU_NOTIFIER > select PREEMPT_NOTIFIERS > select HAVE_KVM_CPU_RELAX_INTERCEPT > @@ -39,7 +41,7 @@ config KVM > select IRQ_BYPASS_MANAGER > select HAVE_KVM_IRQ_BYPASS > select HAVE_KVM_VCPU_RUN_PID_CHANGE > - select SCHEDSTATS > + select TASK_DELAY_ACCT > ---help--- > Support hosting virtualized guest machines. > We don't support KVM with 16K page tables yet, due to the > multiple Same issue as before: you have an implicit config symbol selection. TASK_DELAY_ACCT depends on TASKSTATS (which is why you have this NET && MULTIUSER constraint). You need to select both TASK_DELAY_ACCT and TASKSTATS, as the comment you add suggests. M.
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig index d8b88e4..3c46eac 100644 --- a/arch/arm64/kvm/Kconfig +++ b/arch/arm64/kvm/Kconfig @@ -39,6 +39,7 @@ config KVM select IRQ_BYPASS_MANAGER select HAVE_KVM_IRQ_BYPASS select HAVE_KVM_VCPU_RUN_PID_CHANGE + select SCHED_INFO select SCHEDSTATS ---help--- Support hosting virtualized guest machines.
If KVM=y, it will select SCHEDSTATS, below erros can be seen: kernel/sched/stats.h: In function rq_sched_info_arrive: kernel/sched/stats.h:12:20: error: struct sched_info has no member named run_delay rq->rq_sched_info.run_delay += delta; ^ kernel/sched/stats.h:13:20: error: struct sched_info has no member named pcount rq->rq_sched_info.pcount++; ^ kernel/sched/stats.h: In function rq_sched_info_dequeued: kernel/sched/stats.h:31:20: error: struct sched_info has no member named run_delay rq->rq_sched_info.run_delay += delta; These are because CONFIG_SCHED_INFO is not set, This patch is to select SCHED_INFO before SCHEDSTATS. Fixes: 8564d6372a7d ("KVM: arm64: Support stolen time reporting via shared structure") Signed-off-by: Mao Wenan <maowenan@huawei.com> --- arch/arm64/kvm/Kconfig | 1 + 1 file changed, 1 insertion(+)