[PATCH 0/2] sched/cputime: Deltas for "replace VTIME_GEN irq time code with IRQ_TIME_ACCOUNTING code"

[PATCH 0/2] sched/cputime: Deltas for "replace VTIME_GEN irq time code with IRQ_TIME_ACCOUNTING code"

[Frederic Weisbecker]

Hi Rick,

While reviewing your 2nd patch, I thought about these cleanups. Perhaps
the first one could be merged into your patch. I let you decide.

Thanks.

Frederic Weisbecker (2):
  sched: Complete cleanup of old vtime gen irq accounting
  sched: Reorganize vtime native irqtime accounting headers

 include/linux/vtime.h  | 46 +++++++++++++++++-----------------------------
 kernel/sched/cputime.c | 33 ++++++++++-----------------------
 2 files changed, 27 insertions(+), 52 deletions(-)

-- 
2.7.0

[PATCH 1/2] sched: Complete cleanup of old vtime gen irqtime accounting

[Frederic Weisbecker]

Vtime generic irqtime accounting has been removed but there are a few
remains to cleanup:

* The vtime_accounting_cpu_enabled() check in irq entry was only used
  by CONFIG_VIRT_CPU_ACCOUNTING_GEN. We can safely remove it.

* Without the vtime_accounting_cpu_enabled(), we no longer need to
  have a vtime_common_account_irq_enter() indirect function.

* Move vtime_account_irq_enter() implementation under
  CONFIG_VIRT_CPU_ACCOUNTING_NATIVE which is the last user.

* The vtime_account_user() call was only used on irq entry for
  CONFIG_VIRT_CPU_ACCOUNTING_GEN. We can remove that too.

Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Rik van Riel <riel@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: wanpeng.li@hotmail.com
Cc: Mike Galbraith <efault@gmx.de>
Cc: rkrcmar@redhat.com
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/linux/vtime.h  | 11 -----------
 kernel/sched/cputime.c | 33 ++++++++++-----------------------
 2 files changed, 10 insertions(+), 34 deletions(-)

diff --git a/include/linux/vtime.h b/include/linux/vtime.h
index d1977d84..65aef5e 100644
--- a/include/linux/vtime.h
+++ b/include/linux/vtime.h
@@ -14,18 +14,7 @@ struct task_struct;
  */
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
 static inline bool vtime_accounting_cpu_enabled(void) { return true; }
-
-#ifdef __ARCH_HAS_VTIME_ACCOUNT
 extern void vtime_account_irq_enter(struct task_struct *tsk);
-#else
-extern void vtime_common_account_irq_enter(struct task_struct *tsk);
-static inline void vtime_account_irq_enter(struct task_struct *tsk)
-{
-	if (vtime_accounting_cpu_enabled())
-		vtime_common_account_irq_enter(tsk);
-}
-#endif /* __ARCH_HAS_VTIME_ACCOUNT */
-
 #endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
 
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index ca7e33c..16a873c 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -431,6 +431,10 @@ void vtime_common_task_switch(struct task_struct *prev)
 }
 #endif
 
+#endif /* CONFIG_VIRT_CPU_ACCOUNTING */
+
+
+#ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
 /*
  * Archs that account the whole time spent in the idle task
  * (outside irq) as idle time can rely on this and just implement
@@ -440,33 +444,16 @@ void vtime_common_task_switch(struct task_struct *prev)
  * vtime_account().
  */
 #ifndef __ARCH_HAS_VTIME_ACCOUNT
-void vtime_common_account_irq_enter(struct task_struct *tsk)
+void vtime_account_irq_enter(struct task_struct *tsk)
 {
-	if (!in_interrupt()) {
-		/*
-		 * If we interrupted user, context_tracking_in_user()
-		 * is 1 because the context tracking don't hook
-		 * on irq entry/exit. This way we know if
-		 * we need to flush user time on kernel entry.
-		 */
-		if (context_tracking_in_user()) {
-			vtime_account_user(tsk);
-			return;
-		}
-
-		if (is_idle_task(tsk)) {
-			vtime_account_idle(tsk);
-			return;
-		}
-	}
-	vtime_account_system(tsk);
+	if (!in_interrupt() && is_idle_task(tsk))
+		vtime_account_idle(tsk);
+	else
+		vtime_account_system(tsk);
 }
-EXPORT_SYMBOL_GPL(vtime_common_account_irq_enter);
+EXPORT_SYMBOL_GPL(vtime_account_irq_enter);
 #endif /* __ARCH_HAS_VTIME_ACCOUNT */
-#endif /* CONFIG_VIRT_CPU_ACCOUNTING */
 
-
-#ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
 void task_cputime_adjusted(struct task_struct *p, cputime_t *ut, cputime_t *st)
 {
 	*ut = p->utime;
-- 
2.7.0

[PATCH 2/2] sched: Reorganize vtime native irqtime accounting headers

[Frederic Weisbecker]

The vtime irqtime accounting headers are very scattered and convoluted
right now. Reorganize them such that it is obvious that only
CONFIG_VIRT_CPU_ACCOUNTING_NATIVE does use it.

Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Rik van Riel <riel@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: wanpeng.li@hotmail.com
Cc: Mike Galbraith <efault@gmx.de>
Cc: rkrcmar@redhat.com
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 include/linux/vtime.h | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/include/linux/vtime.h b/include/linux/vtime.h
index 65aef5e..aa9bfea 100644
--- a/include/linux/vtime.h
+++ b/include/linux/vtime.h
@@ -12,12 +12,9 @@ struct task_struct;
 /*
  * vtime_accounting_cpu_enabled() definitions/declarations
  */
-#ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
+#if defined(CONFIG_VIRT_CPU_ACCOUNTING_NATIVE)
 static inline bool vtime_accounting_cpu_enabled(void) { return true; }
-extern void vtime_account_irq_enter(struct task_struct *tsk);
-#endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
-
-#ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
+#elif defined(CONFIG_VIRT_CPU_ACCOUNTING_GEN)
 /*
  * Checks if vtime is enabled on some CPU. Cputime readers want to be careful
  * in that case and compute the tickless cputime.
@@ -38,11 +35,9 @@ static inline bool vtime_accounting_cpu_enabled(void)
 
 	return false;
 }
-#endif /* CONFIG_VIRT_CPU_ACCOUNTING_GEN */
-
-#ifndef CONFIG_VIRT_CPU_ACCOUNTING
+#else /* !CONFIG_VIRT_CPU_ACCOUNTING */
 static inline bool vtime_accounting_cpu_enabled(void) { return false; }
-#endif /* !CONFIG_VIRT_CPU_ACCOUNTING */
+#endif
 
 
 /*
@@ -70,14 +65,10 @@ extern void vtime_account_user(struct task_struct *tsk);
 static inline void vtime_task_switch(struct task_struct *prev) { }
 static inline void vtime_account_system(struct task_struct *tsk) { }
 static inline void vtime_account_user(struct task_struct *tsk) { }
-static inline void vtime_account_irq_enter(struct task_struct *tsk) { }
 #endif /* !CONFIG_VIRT_CPU_ACCOUNTING */
 
 #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN
 extern void arch_vtime_task_switch(struct task_struct *tsk);
-static inline void vtime_account_irq_enter(struct task_struct *tsk) { }
-static inline void vtime_account_irq_exit(struct task_struct *tsk) { }
-
 extern void vtime_user_enter(struct task_struct *tsk);
 
 static inline void vtime_user_exit(struct task_struct *tsk)
@@ -88,11 +79,6 @@ extern void vtime_guest_enter(struct task_struct *tsk);
 extern void vtime_guest_exit(struct task_struct *tsk);
 extern void vtime_init_idle(struct task_struct *tsk, int cpu);
 #else /* !CONFIG_VIRT_CPU_ACCOUNTING_GEN  */
-static inline void vtime_account_irq_exit(struct task_struct *tsk)
-{
-	/* On hard|softirq exit we always account to hard|softirq cputime */
-	vtime_account_system(tsk);
-}
 static inline void vtime_user_enter(struct task_struct *tsk) { }
 static inline void vtime_user_exit(struct task_struct *tsk) { }
 static inline void vtime_guest_enter(struct task_struct *tsk) { }
@@ -100,6 +86,19 @@ static inline void vtime_guest_exit(struct task_struct *tsk) { }
 static inline void vtime_init_idle(struct task_struct *tsk, int cpu) { }
 #endif
 
+#ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
+extern void vtime_account_irq_enter(struct task_struct *tsk);
+static inline void vtime_account_irq_exit(struct task_struct *tsk)
+{
+	/* On hard|softirq exit we always account to hard|softirq cputime */
+	vtime_account_system(tsk);
+}
+#else /* !CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
+static inline void vtime_account_irq_enter(struct task_struct *tsk) { }
+static inline void vtime_account_irq_exit(struct task_struct *tsk) { }
+#endif
+
+
 #ifdef CONFIG_IRQ_TIME_ACCOUNTING
 extern void irqtime_account_irq(struct task_struct *tsk);
 #else
-- 
2.7.0

Re: [PATCH 0/2] sched/cputime: Deltas for "replace VTIME_GEN irq time code with IRQ_TIME_ACCOUNTING code"

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

On Thu, 2016-07-07 at 16:27 +0200, Frederic Weisbecker wrote:
> Hi Rick,
> 
> While reviewing your 2nd patch, I thought about these cleanups.
> Perhaps
> the first one could be merged into your patch. I let you decide.

I'm not convinced we want to merge cleanups and functional
changes into the same patch, given how convoluted the code
is/was.

Both of your patches look good though.

What tree should they go in through?

-- 

All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 0/2] sched/cputime: Deltas for "replace VTIME_GEN irq time code with IRQ_TIME_ACCOUNTING code"

[Ingo Molnar]

* Rik van Riel <riel@redhat.com> wrote:

> On Thu, 2016-07-07 at 16:27 +0200, Frederic Weisbecker wrote:
> > Hi Rick,
> > 
> > While reviewing your 2nd patch, I thought about these cleanups.
> > Perhaps
> > the first one could be merged into your patch. I let you decide.
> 
> I'm not convinced we want to merge cleanups and functional
> changes into the same patch, given how convoluted the code
> is/was.
> 
> Both of your patches look good though.
> 
> What tree should they go in through?

-tip I suspect. So my plan was the following, this series of yours:

  [PATCH v3 0/4] sched,time: fix irq time accounting with nohz_idle

... looked almost ready, it looked like as if I could merge v4 once you sent it.

Plus Frederic submitted these two cleanups - looks like I could merge these on top 
of your series and have them close to each other in the Git space.

And I do agree that we should keep these cleanups separate and not merge them into 
patches that change functionality.

If your series is expected to be risky then we could make things easier to handle 
later on if we switched around things and first made low-risk cleanups and then 
any changes/fixes on top - do you think that's necessary in this case?

Thanks,

	Ingo

Re: [PATCH 0/2] sched/cputime: Deltas for "replace VTIME_GEN irq time code with IRQ_TIME_ACCOUNTING code"

[Frederic Weisbecker]

On Fri, Jul 08, 2016 at 09:30:46AM +0200, Ingo Molnar wrote:
> 
> * Rik van Riel <riel@redhat.com> wrote:
> 
> > On Thu, 2016-07-07 at 16:27 +0200, Frederic Weisbecker wrote:
> > > Hi Rick,
> > > 
> > > While reviewing your 2nd patch, I thought about these cleanups.
> > > Perhaps
> > > the first one could be merged into your patch. I let you decide.
> > 
> > I'm not convinced we want to merge cleanups and functional
> > changes into the same patch, given how convoluted the code
> > is/was.
> > 
> > Both of your patches look good though.
> > 
> > What tree should they go in through?
> 
> -tip I suspect. So my plan was the following, this series of yours:
> 
>   [PATCH v3 0/4] sched,time: fix irq time accounting with nohz_idle
> 
> ... looked almost ready, it looked like as if I could merge v4 once you sent it.
> 
> Plus Frederic submitted these two cleanups - looks like I could merge these on top 
> of your series and have them close to each other in the Git space.
> 
> And I do agree that we should keep these cleanups separate and not merge them into 
> patches that change functionality.
> 
> If your series is expected to be risky then we could make things easier to handle 
> later on if we switched around things and first made low-risk cleanups and then 
> any changes/fixes on top - do you think that's necessary in this case?

I personally think that none of this is low-risk material. Perhaps we can gather
the whole in the same tree? I can resend the series proper with my patches inside
if you like. And I have yet to review the last patch of the series.

Thanks.

Re: [PATCH 0/2] sched/cputime: Deltas for "replace VTIME_GEN irq time code with IRQ_TIME_ACCOUNTING code"

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Fri, Jul 08, 2016 at 09:30:46AM +0200, Ingo Molnar wrote:
> > 
> > * Rik van Riel <riel@redhat.com> wrote:
> > 
> > > On Thu, 2016-07-07 at 16:27 +0200, Frederic Weisbecker wrote:
> > > > Hi Rick,
> > > > 
> > > > While reviewing your 2nd patch, I thought about these cleanups.
> > > > Perhaps
> > > > the first one could be merged into your patch. I let you decide.
> > > 
> > > I'm not convinced we want to merge cleanups and functional
> > > changes into the same patch, given how convoluted the code
> > > is/was.
> > > 
> > > Both of your patches look good though.
> > > 
> > > What tree should they go in through?
> > 
> > -tip I suspect. So my plan was the following, this series of yours:
> > 
> >   [PATCH v3 0/4] sched,time: fix irq time accounting with nohz_idle
> > 
> > ... looked almost ready, it looked like as if I could merge v4 once you sent it.
> > 
> > Plus Frederic submitted these two cleanups - looks like I could merge these on top 
> > of your series and have them close to each other in the Git space.
> > 
> > And I do agree that we should keep these cleanups separate and not merge them into 
> > patches that change functionality.
> > 
> > If your series is expected to be risky then we could make things easier to handle 
> > later on if we switched around things and first made low-risk cleanups and then 
> > any changes/fixes on top - do you think that's necessary in this case?
> 
> I personally think that none of this is low-risk material. Perhaps we can gather 
> the whole in the same tree? I can resend the series proper with my patches 
> inside if you like. And I have yet to review the last patch of the series.

Sure, we can do it like that, for tip:timers/nohz.

Thanks,

	Ingo

Re: [PATCH 0/2] sched/cputime: Deltas for "replace VTIME_GEN irq time code with IRQ_TIME_ACCOUNTING code"

[Frederic Weisbecker]

On Fri, Jul 08, 2016 at 02:03:03PM +0200, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > On Fri, Jul 08, 2016 at 09:30:46AM +0200, Ingo Molnar wrote:
> > > 
> > > * Rik van Riel <riel@redhat.com> wrote:
> > > 
> > > > On Thu, 2016-07-07 at 16:27 +0200, Frederic Weisbecker wrote:
> > > > > Hi Rick,
> > > > > 
> > > > > While reviewing your 2nd patch, I thought about these cleanups.
> > > > > Perhaps
> > > > > the first one could be merged into your patch. I let you decide.
> > > > 
> > > > I'm not convinced we want to merge cleanups and functional
> > > > changes into the same patch, given how convoluted the code
> > > > is/was.
> > > > 
> > > > Both of your patches look good though.
> > > > 
> > > > What tree should they go in through?
> > > 
> > > -tip I suspect. So my plan was the following, this series of yours:
> > > 
> > >   [PATCH v3 0/4] sched,time: fix irq time accounting with nohz_idle
> > > 
> > > ... looked almost ready, it looked like as if I could merge v4 once you sent it.
> > > 
> > > Plus Frederic submitted these two cleanups - looks like I could merge these on top 
> > > of your series and have them close to each other in the Git space.
> > > 
> > > And I do agree that we should keep these cleanups separate and not merge them into 
> > > patches that change functionality.
> > > 
> > > If your series is expected to be risky then we could make things easier to handle 
> > > later on if we switched around things and first made low-risk cleanups and then 
> > > any changes/fixes on top - do you think that's necessary in this case?
> > 
> > I personally think that none of this is low-risk material. Perhaps we can gather 
> > the whole in the same tree? I can resend the series proper with my patches 
> > inside if you like. And I have yet to review the last patch of the series.
> 
> Sure, we can do it like that, for tip:timers/nohz.

Ok, I'll base it on tip:sched/core, right?

Re: [PATCH 0/2] sched/cputime: Deltas for "replace VTIME_GEN irq time code with IRQ_TIME_ACCOUNTING code"

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Fri, Jul 08, 2016 at 02:03:03PM +0200, Ingo Molnar wrote:
> > 
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > On Fri, Jul 08, 2016 at 09:30:46AM +0200, Ingo Molnar wrote:
> > > > 
> > > > * Rik van Riel <riel@redhat.com> wrote:
> > > > 
> > > > > On Thu, 2016-07-07 at 16:27 +0200, Frederic Weisbecker wrote:
> > > > > > Hi Rick,
> > > > > > 
> > > > > > While reviewing your 2nd patch, I thought about these cleanups.
> > > > > > Perhaps
> > > > > > the first one could be merged into your patch. I let you decide.
> > > > > 
> > > > > I'm not convinced we want to merge cleanups and functional
> > > > > changes into the same patch, given how convoluted the code
> > > > > is/was.
> > > > > 
> > > > > Both of your patches look good though.
> > > > > 
> > > > > What tree should they go in through?
> > > > 
> > > > -tip I suspect. So my plan was the following, this series of yours:
> > > > 
> > > >   [PATCH v3 0/4] sched,time: fix irq time accounting with nohz_idle
> > > > 
> > > > ... looked almost ready, it looked like as if I could merge v4 once you sent it.
> > > > 
> > > > Plus Frederic submitted these two cleanups - looks like I could merge these on top 
> > > > of your series and have them close to each other in the Git space.
> > > > 
> > > > And I do agree that we should keep these cleanups separate and not merge them into 
> > > > patches that change functionality.
> > > > 
> > > > If your series is expected to be risky then we could make things easier to handle 
> > > > later on if we switched around things and first made low-risk cleanups and then 
> > > > any changes/fixes on top - do you think that's necessary in this case?
> > > 
> > > I personally think that none of this is low-risk material. Perhaps we can gather 
> > > the whole in the same tree? I can resend the series proper with my patches 
> > > inside if you like. And I have yet to review the last patch of the series.
> > 
> > Sure, we can do it like that, for tip:timers/nohz.
> 
> Ok, I'll base it on tip:sched/core, right?

Only if there's conflicts or dependencies - otherwise please use v4.7-rc6 as a 
base.

Thanks,

	Ingo

Re: [PATCH 0/2] sched/cputime: Deltas for "replace VTIME_GEN irq time code with IRQ_TIME_ACCOUNTING code"

[Frederic Weisbecker]

On Fri, Jul 08, 2016 at 02:21:17PM +0200, Ingo Molnar wrote:
> 
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > On Fri, Jul 08, 2016 at 02:03:03PM +0200, Ingo Molnar wrote:
> > > 
> > > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > > 
> > > > On Fri, Jul 08, 2016 at 09:30:46AM +0200, Ingo Molnar wrote:
> > > > > 
> > > > > * Rik van Riel <riel@redhat.com> wrote:
> > > > > 
> > > > > > On Thu, 2016-07-07 at 16:27 +0200, Frederic Weisbecker wrote:
> > > > > > > Hi Rick,
> > > > > > > 
> > > > > > > While reviewing your 2nd patch, I thought about these cleanups.
> > > > > > > Perhaps
> > > > > > > the first one could be merged into your patch. I let you decide.
> > > > > > 
> > > > > > I'm not convinced we want to merge cleanups and functional
> > > > > > changes into the same patch, given how convoluted the code
> > > > > > is/was.
> > > > > > 
> > > > > > Both of your patches look good though.
> > > > > > 
> > > > > > What tree should they go in through?
> > > > > 
> > > > > -tip I suspect. So my plan was the following, this series of yours:
> > > > > 
> > > > >   [PATCH v3 0/4] sched,time: fix irq time accounting with nohz_idle
> > > > > 
> > > > > ... looked almost ready, it looked like as if I could merge v4 once you sent it.
> > > > > 
> > > > > Plus Frederic submitted these two cleanups - looks like I could merge these on top 
> > > > > of your series and have them close to each other in the Git space.
> > > > > 
> > > > > And I do agree that we should keep these cleanups separate and not merge them into 
> > > > > patches that change functionality.
> > > > > 
> > > > > If your series is expected to be risky then we could make things easier to handle 
> > > > > later on if we switched around things and first made low-risk cleanups and then 
> > > > > any changes/fixes on top - do you think that's necessary in this case?
> > > > 
> > > > I personally think that none of this is low-risk material. Perhaps we can gather 
> > > > the whole in the same tree? I can resend the series proper with my patches 
> > > > inside if you like. And I have yet to review the last patch of the series.
> > > 
> > > Sure, we can do it like that, for tip:timers/nohz.
> > 
> > Ok, I'll base it on tip:sched/core, right?
> 
> Only if there's conflicts or dependencies - otherwise please use v4.7-rc6 as a 
> base.

Ah so it can be a standalone branch, good point, thanks!