Re: [PATCH v2] kvm: remove dependence on delay-accounting
On Sat, Jan 14, 2012 at 08:30:51PM +0400, Konstantin Khlebnikov wrote: KVM selects delay-accounting only to get sched-info for steal-time accounting. Meanwhile delay-accounting can be disabled by boot option. This is ridiculous. This patch adds internal boolean option CONFIG_TASK_SCHED_INFO to enable only task-sched_info and its collecting inside scheduler. v2: * stupid misprint fixed Signed-off-by: Konstantin Khlebnikov khlebni...@openvz.org --- arch/x86/kvm/Kconfig |5 + include/linux/sched.h |6 ++ init/Kconfig |7 +++ kernel/sched/core.c |2 +- kernel/sched/stats.h |4 ++-- lib/Kconfig.debug |1 + 6 files changed, 14 insertions(+), 11 deletions(-) diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig index 1a7fe86..e3952e8 100644 --- a/arch/x86/kvm/Kconfig +++ b/arch/x86/kvm/Kconfig @@ -22,8 +22,6 @@ config KVM depends on HAVE_KVM # for device assignment: depends on PCI - # for TASKSTATS/TASK_DELAY_ACCT: - depends on NET select PREEMPT_NOTIFIERS select MMU_NOTIFIER select ANON_INODES @@ -33,8 +31,7 @@ config KVM select KVM_ASYNC_PF select USER_RETURN_NOTIFIER select KVM_MMIO - select TASKSTATS - select TASK_DELAY_ACCT + select TASK_SCHED_INFO select PERF_EVENTS ---help--- Support hosting fully virtualized guest machines using hardware diff --git a/include/linux/sched.h b/include/linux/sched.h index 868cb83..dd5bf78 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -734,7 +734,6 @@ extern struct user_struct root_user; struct backing_dev_info; -#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT) struct sched_info { /* cumulative counters */ unsigned long pcount; /* # of times run on this cpu */ @@ -744,7 +743,6 @@ struct sched_info { unsigned long long last_arrival,/* when we last ran on a cpu */ last_queued; /* when we were last queued to run */ }; -#endif /* defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT) */ #ifdef CONFIG_TASK_DELAY_ACCT struct task_delay_info { @@ -782,7 +780,7 @@ struct task_delay_info { static inline int sched_info_on(void) { -#ifdef CONFIG_SCHEDSTATS +#if IS_ENABLED(CONFIG_SCHEDSTATS) || IS_ENABLED(CONFIG_KVM) return 1; CONFIG_TASK_SCHED_INFO? -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm: remove dependence on delay-accounting
On Sat, 2012-01-14 at 20:30 +0400, Konstantin Khlebnikov wrote: KVM selects delay-accounting only to get sched-info for steal-time accounting. Meanwhile delay-accounting can be disabled by boot option. This is ridiculous. This patch adds internal boolean option CONFIG_TASK_SCHED_INFO to enable only task-sched_info and its collecting inside scheduler. Urgh, more stupid config knobs, we should be removing them, not adding moar. diff --git a/include/linux/sched.h b/include/linux/sched.h index 868cb83..dd5bf78 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -734,7 +734,6 @@ extern struct user_struct root_user; struct backing_dev_info; -#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT) struct sched_info { /* cumulative counters */ unsigned long pcount; /* # of times run on this cpu */ @@ -744,7 +743,6 @@ struct sched_info { unsigned long long last_arrival,/* when we last ran on a cpu */ last_queued; /* when we were last queued to run */ }; -#endif /* defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT) */ Not having that structure helps with compile errors. #ifdef CONFIG_TASK_DELAY_ACCT struct task_delay_info { @@ -782,7 +780,7 @@ struct task_delay_info { static inline int sched_info_on(void) { -#ifdef CONFIG_SCHEDSTATS +#if IS_ENABLED(CONFIG_SCHEDSTATS) || IS_ENABLED(CONFIG_KVM) WTF is IS_ENABLED and why do you use it? Not much like this stuff. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm: remove dependence on delay-accounting
Marcelo Tosatti wrote: On Sat, Jan 14, 2012 at 08:30:51PM +0400, Konstantin Khlebnikov wrote: KVM selects delay-accounting only to get sched-info for steal-time accounting. Meanwhile delay-accounting can be disabled by boot option. This is ridiculous. This patch adds internal boolean option CONFIG_TASK_SCHED_INFO to enable only task-sched_info and its collecting inside scheduler. cut static inline int sched_info_on(void) { -#ifdef CONFIG_SCHEDSTATS +#if IS_ENABLED(CONFIG_SCHEDSTATS) || IS_ENABLED(CONFIG_KVM) return 1; CONFIG_TASK_SCHED_INFO? It makes it equal to constant 1, because all its callers are under #ifdef CONFIG_TASK_SCHED_INFO =) its current code: static inline int sched_info_on(void) { #ifdef CONFIG_SCHEDSTATS return 1; #elif defined(CONFIG_TASK_DELAY_ACCT) extern int delayacct_on; return delayacct_on; #else return 0; #endif } CONFIG_SCHEDSTATS == debug option in lib/Kconfig.debug for /proc/schedstat CONFIG_TASK_DELAY_ACCT == wierd net-link based statistics collecting tool Thus, may be better to remove this function, because it can return 0 only if delay-accounting is compiled but disabled by boot option (delayacct_on =1 by default since 2.6.18) patch follows... -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] kvm: remove dependence on delay-accounting
Peter Zijlstra wrote: On Sat, 2012-01-14 at 20:30 +0400, Konstantin Khlebnikov wrote: KVM selects delay-accounting only to get sched-info for steal-time accounting. Meanwhile delay-accounting can be disabled by boot option. This is ridiculous. This patch adds internal boolean option CONFIG_TASK_SCHED_INFO to enable only task-sched_info and its collecting inside scheduler. Urgh, more stupid config knobs, we should be removing them, not adding moar. Unfortunately, removing task-delay-accounting is not the option =) diff --git a/include/linux/sched.h b/include/linux/sched.h index 868cb83..dd5bf78 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -734,7 +734,6 @@ extern struct user_struct root_user; struct backing_dev_info; -#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT) struct sched_info { /* cumulative counters */ unsigned long pcount; /* # of times run on this cpu */ @@ -744,7 +743,6 @@ struct sched_info { unsigned long long last_arrival,/* when we last ran on a cpu */ last_queued; /* when we were last queued to run */ }; -#endif /* defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT) */ Not having that structure helps with compile errors. I don't think so, this only helps to catch useless declarations and definitions in code and inside other structures. #ifdef CONFIG_TASK_DELAY_ACCT struct task_delay_info { @@ -782,7 +780,7 @@ struct task_delay_info { static inline int sched_info_on(void) { -#ifdef CONFIG_SCHEDSTATS +#if IS_ENABLED(CONFIG_SCHEDSTATS) || IS_ENABLED(CONFIG_KVM) WTF is IS_ENABLED and why do you use it? IS_ENABLED(smth) == (defined(smth) || defined(smth_MODULE)) but using it for CONFIG_SCHEDSTATS is overkill, indeed. Not much like this stuff. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html