kvmarm.lists.cs.columbia.edu archive mirror
 help / color / mirror / Atom feed
* [PATCH] KVM: arm64: Select SCHED_INFO before SCHEDSTATS
@ 2019-10-23  3:22 Mao Wenan
  2019-10-23 12:32 ` Steven Price
  2019-10-23 16:51 ` Marc Zyngier
  0 siblings, 2 replies; 6+ messages in thread
From: Mao Wenan @ 2019-10-23  3:22 UTC (permalink / raw)
  To: maz, james.morse, julien.thierry.kdev, suzuki.poulose,
	catalin.marinas, will, steven.price
  Cc: Mao Wenan, kernel-janitors, kvmarm, linux-arm-kernel, linux-kernel

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.
-- 
2.7.4

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: arm64: Select SCHED_INFO before SCHEDSTATS
  2019-10-23  3:22 [PATCH] KVM: arm64: Select SCHED_INFO before SCHEDSTATS Mao Wenan
@ 2019-10-23 12:32 ` Steven Price
  2019-10-23 16:51 ` Marc Zyngier
  1 sibling, 0 replies; 6+ messages in thread
From: Steven Price @ 2019-10-23 12:32 UTC (permalink / raw)
  To: Mao Wenan
  Cc: maz, kernel-janitors, linux-kernel, linux-arm-kernel,
	Catalin Marinas, will, kvmarm

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
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: arm64: Select SCHED_INFO before SCHEDSTATS
  2019-10-23  3:22 [PATCH] KVM: arm64: Select SCHED_INFO before SCHEDSTATS Mao Wenan
  2019-10-23 12:32 ` Steven Price
@ 2019-10-23 16:51 ` Marc Zyngier
  2019-10-24 11:22   ` Steven Price
  1 sibling, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2019-10-23 16:51 UTC (permalink / raw)
  To: Mao Wenan, steven.price
  Cc: catalin.marinas, kernel-janitors, linux-kernel, will, kvmarm,
	linux-arm-kernel

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.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: arm64: Select SCHED_INFO before SCHEDSTATS
  2019-10-23 16:51 ` Marc Zyngier
@ 2019-10-24 11:22   ` Steven Price
  2019-10-24 13:19     ` Marc Zyngier
  0 siblings, 1 reply; 6+ messages in thread
From: Steven Price @ 2019-10-24 11:22 UTC (permalink / raw)
  To: Marc Zyngier, Mao Wenan
  Cc: catalin.marinas, kernel-janitors, linux-kernel, will, kvmarm,
	linux-arm-kernel

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
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: arm64: Select SCHED_INFO before SCHEDSTATS
  2019-10-24 11:22   ` Steven Price
@ 2019-10-24 13:19     ` Marc Zyngier
  2019-10-24 13:31       ` [PATCH v2] KVM: arm64: Select TASK_DELAY_ACCT+TASKSTATS rather than SCHEDSTATS Steven Price
  0 siblings, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2019-10-24 13:19 UTC (permalink / raw)
  To: Steven Price
  Cc: catalin.marinas, kernel-janitors, Mao Wenan, linux-kernel, will,
	kvmarm, linux-arm-kernel

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.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH v2] KVM: arm64: Select TASK_DELAY_ACCT+TASKSTATS rather than SCHEDSTATS
  2019-10-24 13:19     ` Marc Zyngier
@ 2019-10-24 13:31       ` Steven Price
  0 siblings, 0 replies; 6+ messages in thread
From: Steven Price @ 2019-10-24 13:31 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: catalin.marinas, kernel-janitors, Mao Wenan, linux-kernel,
	Steven Price, will, kvmarm, linux-arm-kernel

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 and
TASKSTATS. 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>
---

Let's try again! Somehow I'd got it into my head that TASK_DELAY_ACCT
selected TASKSTATS not depended on. Even though I'd managed to get it
right in the comment!

 arch/arm64/kvm/Kconfig | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index d8b88e40d223..a475c68cbfec 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,8 @@ config KVM
 	select IRQ_BYPASS_MANAGER
 	select HAVE_KVM_IRQ_BYPASS
 	select HAVE_KVM_VCPU_RUN_PID_CHANGE
-	select SCHEDSTATS
+	select TASKSTATS
+	select TASK_DELAY_ACCT
 	---help---
 	  Support hosting virtualized guest machines.
 	  We don't support KVM with 16K page tables yet, due to the multiple
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-24 13:31 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-23  3:22 [PATCH] KVM: arm64: Select SCHED_INFO before SCHEDSTATS Mao Wenan
2019-10-23 12:32 ` Steven Price
2019-10-23 16:51 ` Marc Zyngier
2019-10-24 11:22   ` Steven Price
2019-10-24 13:19     ` Marc Zyngier
2019-10-24 13:31       ` [PATCH v2] KVM: arm64: Select TASK_DELAY_ACCT+TASKSTATS rather than SCHEDSTATS Steven Price

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).