| Message ID | 1446737696-9749-2-git-send-email-stefano.stabellini@eu.citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
How can this be missing? Things compile fine now, right? So please better explain why we do this change.
On Thu, 5 Nov 2015, Peter Zijlstra wrote: > How can this be missing? Things compile fine now, right? Fair enough. > So please better explain why we do this change. asm/paravirt.h is included by one of the other headers included in kernel/sched/cputime.c on x86, but not on other architecures. On arm and arm64, where I am about to introduce asm/paravirt.h and stolen time support, without #include <asm/paravirt.h> in cputime.c I would get: kernel/sched/cputime.c: In function ‘steal_account_process_tick’: kernel/sched/cputime.c:260:24: error: ‘paravirt_steal_enabled’ undeclared (first use in this function) if (static_key_false(¶virt_steal_enabled)) { A bit of digging on x86 (using gcc -E on cputime.c) tells me that asm/paravirt.h is coming from the following include chain: #include <kernel/sched/sched.h> #include <linux/spinlock.h> #include <linux/preempt.h> #include <asm/preempt.h> #include <linux/thread_info.h> #include <asm/thread_info.h> #include <asm/processor.h> #include <asm/msr.h> #include <asm/paravirt.h>
On Thu, Nov 05, 2015 at 05:30:01PM +0000, Stefano Stabellini wrote: > On Thu, 5 Nov 2015, Peter Zijlstra wrote: > > How can this be missing? Things compile fine now, right? > > Fair enough. > > > > So please better explain why we do this change. > > asm/paravirt.h is included by one of the other headers included in > kernel/sched/cputime.c on x86, but not on other architecures. On arm and > arm64, where I am about to introduce asm/paravirt.h and stolen time > support, without #include <asm/paravirt.h> in cputime.c I would get: > > kernel/sched/cputime.c: In function ‘steal_account_process_tick’: > kernel/sched/cputime.c:260:24: error: ‘paravirt_steal_enabled’ undeclared (first use in this function) > if (static_key_false(¶virt_steal_enabled)) { > > A bit of digging on x86 (using gcc -E on cputime.c) tells me that > asm/paravirt.h is coming from the following include chain: > > #include <kernel/sched/sched.h> > #include <linux/spinlock.h> > #include <linux/preempt.h> > #include <asm/preempt.h> > #include <linux/thread_info.h> > #include <asm/thread_info.h> > #include <asm/processor.h> > #include <asm/msr.h> > #include <asm/paravirt.h> Fair enough; a slightly shorter version of that for a changelog will do nicely. Thanks!
On Mon, 9 Nov 2015, Peter Zijlstra wrote: > On Thu, Nov 05, 2015 at 05:30:01PM +0000, Stefano Stabellini wrote: > > On Thu, 5 Nov 2015, Peter Zijlstra wrote: > > > How can this be missing? Things compile fine now, right? > > > > Fair enough. > > > > > > > So please better explain why we do this change. > > > > asm/paravirt.h is included by one of the other headers included in > > kernel/sched/cputime.c on x86, but not on other architecures. On arm and > > arm64, where I am about to introduce asm/paravirt.h and stolen time > > support, without #include <asm/paravirt.h> in cputime.c I would get: > > > > kernel/sched/cputime.c: In function ‘steal_account_process_tick’: > > kernel/sched/cputime.c:260:24: error: ‘paravirt_steal_enabled’ undeclared (first use in this function) > > if (static_key_false(¶virt_steal_enabled)) { > > > > A bit of digging on x86 (using gcc -E on cputime.c) tells me that > > asm/paravirt.h is coming from the following include chain: > > > > #include <kernel/sched/sched.h> > > #include <linux/spinlock.h> > > #include <linux/preempt.h> > > #include <asm/preempt.h> > > #include <linux/thread_info.h> > > #include <asm/thread_info.h> > > #include <asm/processor.h> > > #include <asm/msr.h> > > #include <asm/paravirt.h> > > > Fair enough; a slightly shorter version of that for a changelog will do > nicely. Sure. Can I add your Acked-by to it?
On Tue, Nov 10, 2015 at 11:27:33AM +0000, Stefano Stabellini wrote: > On Mon, 9 Nov 2015, Peter Zijlstra wrote: > > On Thu, Nov 05, 2015 at 05:30:01PM +0000, Stefano Stabellini wrote: > > > On Thu, 5 Nov 2015, Peter Zijlstra wrote: > > > > How can this be missing? Things compile fine now, right? > > > > > > Fair enough. > > > > > > > > > > So please better explain why we do this change. > > > > > > asm/paravirt.h is included by one of the other headers included in > > > kernel/sched/cputime.c on x86, but not on other architecures. On arm and > > > arm64, where I am about to introduce asm/paravirt.h and stolen time > > > support, without #include <asm/paravirt.h> in cputime.c I would get: > > > > > > kernel/sched/cputime.c: In function ‘steal_account_process_tick’: > > > kernel/sched/cputime.c:260:24: error: ‘paravirt_steal_enabled’ undeclared (first use in this function) > > > if (static_key_false(¶virt_steal_enabled)) { > > > > > > A bit of digging on x86 (using gcc -E on cputime.c) tells me that > > > asm/paravirt.h is coming from the following include chain: > > > > > > #include <kernel/sched/sched.h> > > > #include <linux/spinlock.h> > > > #include <linux/preempt.h> > > > #include <asm/preempt.h> > > > #include <linux/thread_info.h> > > > #include <asm/thread_info.h> > > > #include <asm/processor.h> > > > #include <asm/msr.h> > > > #include <asm/paravirt.h> > > > > > > Fair enough; a slightly shorter version of that for a changelog will do > > nicely. > > Sure. Can I add your Acked-by to it? Yep, no objection if the changelog is updated as per the above. Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c index 8cbc3db..c7a27c4 100644 --- a/kernel/sched/cputime.c +++ b/kernel/sched/cputime.c @@ -5,6 +5,9 @@ #include <linux/static_key.h> #include <linux/context_tracking.h> #include "sched.h" +#ifdef CONFIG_PARAVIRT +#include <asm/paravirt.h> +#endif #ifdef CONFIG_IRQ_TIME_ACCOUNTING
Add include asm/paravirt.h to cputime.c, as steal_account_process_tick calls paravirt_steal_clock, which is defined in asm/paravirt.h. The ifdef CONFIG_PARAVIRT is necessary because not all archs have an asm/paravirt.h to include. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> CC: mingo@redhat.com CC: peterz@infradead.org --- Changes in v11: - add ifdef CONFIG_PARAVIRT to cputime.c, because not all architectures have an asm/paravirt.h header file to include - drop the removal of ifdef CONFIG_PARAVIRT from kernel/sched/core.c for the same reason --- kernel/sched/cputime.c | 3 +++ 1 file changed, 3 insertions(+)