Re: [PATCH] sched/rt: Fix rq->clock_update_flags < RQCF_ACT_SKIP warning

2018-04-05 Thread Ingo Molnar 

* Peter Zijlstra  wrote:

> On Wed, Apr 04, 2018 at 09:15:39AM -0700, Davidlohr Bueso wrote:
> > On Mon, 02 Apr 2018, Davidlohr Bueso wrote:
> > 
> > > The case for the rt task throttling (which this workload also hits) can 
> > > be ignored in
> > > that the skip_update call is actually bogus and quite the contrary (the 
> > > request bits
> > > are removed/reverted).
> > 
> > While at it, how about this trivial patch?
> > 
> > 8<
> > [PATCH 2/1] sched: Simplify helpers for rq clock update skip requests
> > 
> > By renaming the functions we can get rid of the skip parameter
> > and have better code redability. It makes zero sense to have
> > things such as:
> > 
> > rq_clock_skip_update(rq, false)
> > 
> > When the skip request is in fact not going to happen. Ever. Rename
> > things such that we end up with:
> > 
> > rq_clock_skip_update(rq)
> > rq_clock_cancel_skipupdate(rq)
> > 
> > Signed-off-by: Davidlohr Bueso 
> 
> Works for me :-)
> 
> Acked-by: Peter Zijlstra (Intel) 

I have applied both patches, thanks guys!

Ingo

Re: [PATCH] sched/rt: Fix rq->clock_update_flags < RQCF_ACT_SKIP warning

2018-04-05 Thread Ingo Molnar 

* Peter Zijlstra  wrote:

> On Wed, Apr 04, 2018 at 09:15:39AM -0700, Davidlohr Bueso wrote:
> > On Mon, 02 Apr 2018, Davidlohr Bueso wrote:
> > 
> > > The case for the rt task throttling (which this workload also hits) can 
> > > be ignored in
> > > that the skip_update call is actually bogus and quite the contrary (the 
> > > request bits
> > > are removed/reverted).
> > 
> > While at it, how about this trivial patch?
> > 
> > 8<
> > [PATCH 2/1] sched: Simplify helpers for rq clock update skip requests
> > 
> > By renaming the functions we can get rid of the skip parameter
> > and have better code redability. It makes zero sense to have
> > things such as:
> > 
> > rq_clock_skip_update(rq, false)
> > 
> > When the skip request is in fact not going to happen. Ever. Rename
> > things such that we end up with:
> > 
> > rq_clock_skip_update(rq)
> > rq_clock_cancel_skipupdate(rq)
> > 
> > Signed-off-by: Davidlohr Bueso 
> 
> Works for me :-)
> 
> Acked-by: Peter Zijlstra (Intel) 

I have applied both patches, thanks guys!

Ingo

Re: [PATCH] sched/rt: Fix rq->clock_update_flags < RQCF_ACT_SKIP warning

2018-04-04 Thread Peter Zijlstra 
On Wed, Apr 04, 2018 at 09:15:39AM -0700, Davidlohr Bueso wrote:
> On Mon, 02 Apr 2018, Davidlohr Bueso wrote:
> 
> > The case for the rt task throttling (which this workload also hits) can be 
> > ignored in
> > that the skip_update call is actually bogus and quite the contrary (the 
> > request bits
> > are removed/reverted).
> 
> While at it, how about this trivial patch?
> 
> 8<
> [PATCH 2/1] sched: Simplify helpers for rq clock update skip requests
> 
> By renaming the functions we can get rid of the skip parameter
> and have better code redability. It makes zero sense to have
> things such as:
> 
> rq_clock_skip_update(rq, false)
> 
> When the skip request is in fact not going to happen. Ever. Rename
> things such that we end up with:
> 
> rq_clock_skip_update(rq)
> rq_clock_cancel_skipupdate(rq)
> 
> Signed-off-by: Davidlohr Bueso 

Works for me :-)

Acked-by: Peter Zijlstra (Intel)

Re: [PATCH] sched/rt: Fix rq->clock_update_flags < RQCF_ACT_SKIP warning

2018-04-04 Thread Peter Zijlstra 
On Wed, Apr 04, 2018 at 09:15:39AM -0700, Davidlohr Bueso wrote:
> On Mon, 02 Apr 2018, Davidlohr Bueso wrote:
> 
> > The case for the rt task throttling (which this workload also hits) can be 
> > ignored in
> > that the skip_update call is actually bogus and quite the contrary (the 
> > request bits
> > are removed/reverted).
> 
> While at it, how about this trivial patch?
> 
> 8<
> [PATCH 2/1] sched: Simplify helpers for rq clock update skip requests
> 
> By renaming the functions we can get rid of the skip parameter
> and have better code redability. It makes zero sense to have
> things such as:
> 
> rq_clock_skip_update(rq, false)
> 
> When the skip request is in fact not going to happen. Ever. Rename
> things such that we end up with:
> 
> rq_clock_skip_update(rq)
> rq_clock_cancel_skipupdate(rq)
> 
> Signed-off-by: Davidlohr Bueso 

Works for me :-)

Acked-by: Peter Zijlstra (Intel)

Re: [PATCH] sched/rt: Fix rq->clock_update_flags < RQCF_ACT_SKIP warning

2018-04-04 Thread Davidlohr Bueso 

On Mon, 02 Apr 2018, Davidlohr Bueso wrote:


The case for the rt task throttling (which this workload also hits) can be 
ignored in
that the skip_update call is actually bogus and quite the contrary (the request 
bits
are removed/reverted).


While at it, how about this trivial patch?

8<
[PATCH 2/1] sched: Simplify helpers for rq clock update skip requests

By renaming the functions we can get rid of the skip parameter
and have better code redability. It makes zero sense to have
things such as:

rq_clock_skip_update(rq, false)

When the skip request is in fact not going to happen. Ever. Rename
things such that we end up with:

rq_clock_skip_update(rq)
rq_clock_cancel_skipupdate(rq)

Signed-off-by: Davidlohr Bueso 
---
kernel/sched/core.c |  2 +-
kernel/sched/deadline.c |  2 +-
kernel/sched/fair.c |  2 +-
kernel/sched/rt.c   |  2 +-
kernel/sched/sched.h| 17 -
5 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index de440456f15c..c4334a3350c5 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -874,7 +874,7 @@ void check_preempt_curr(struct rq *rq, struct task_struct 
*p, int flags)
 * this case, we can save a useless back to back clock update.
 */
if (task_on_rq_queued(rq->curr) && test_tsk_need_resched(rq->curr))
-   rq_clock_skip_update(rq, true);
+   rq_clock_skip_update(rq);
}

#ifdef CONFIG_SMP
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index d1c7bf7c7e5b..e7b3008b85bb 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1560,7 +1560,7 @@ static void yield_task_dl(struct rq *rq)
 * so we don't do microscopic update in schedule()
 * and double the fastpath cost.
 */
-   rq_clock_skip_update(rq, true);
+   rq_clock_skip_update(rq);
}

#ifdef CONFIG_SMP
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 0951d1c58d2f..54dc31e7ab9b 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7089,7 +7089,7 @@ static void yield_task_fair(struct rq *rq)
 * so we don't do microscopic update in schedule()
 * and double the fastpath cost.
 */
-   rq_clock_skip_update(rq, true);
+   rq_clock_skip_update(rq);
}

set_skip_buddy(se);
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index ad13e6242481..7aef6b4e885a 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -861,7 +861,7 @@ static int do_sched_rt_period_timer(struct rt_bandwidth 
*rt_b, int overrun)
 * 'runtime'.
 */
if (rt_rq->rt_nr_running && rq->curr == 
rq->idle)
-   rq_clock_skip_update(rq, false);
+   rq_clock_cancel_skipupdate(rq);
}
if (rt_rq->rt_time || rt_rq->rt_nr_running)
idle = 0;
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index c3deaee7a7a2..15750c222ca2 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -976,13 +976,20 @@ static inline u64 rq_clock_task(struct rq *rq)
return rq->clock_task;
}

-static inline void rq_clock_skip_update(struct rq *rq, bool skip)
+static inline void rq_clock_skip_update(struct rq *rq)
{
lockdep_assert_held(>lock);
-   if (skip)
-   rq->clock_update_flags |= RQCF_REQ_SKIP;
-   else
-   rq->clock_update_flags &= ~RQCF_REQ_SKIP;
+   rq->clock_update_flags |= RQCF_REQ_SKIP;
+}
+
+/*
+ * See rt task throttoling, which is the only time a skip
+ * request is cancelled.
+ */
+static inline void rq_clock_cancel_skipupdate(struct rq *rq)
+{
+   lockdep_assert_held(>lock);
+   rq->clock_update_flags &= ~RQCF_REQ_SKIP;
}

struct rq_flags {
--
2.13.6

Re: [PATCH] sched/rt: Fix rq->clock_update_flags < RQCF_ACT_SKIP warning

2018-04-04 Thread Davidlohr Bueso 

On Mon, 02 Apr 2018, Davidlohr Bueso wrote:


The case for the rt task throttling (which this workload also hits) can be 
ignored in
that the skip_update call is actually bogus and quite the contrary (the request 
bits
are removed/reverted).


While at it, how about this trivial patch?

8<
[PATCH 2/1] sched: Simplify helpers for rq clock update skip requests

By renaming the functions we can get rid of the skip parameter
and have better code redability. It makes zero sense to have
things such as:

rq_clock_skip_update(rq, false)

When the skip request is in fact not going to happen. Ever. Rename
things such that we end up with:

rq_clock_skip_update(rq)
rq_clock_cancel_skipupdate(rq)

Signed-off-by: Davidlohr Bueso 
---
kernel/sched/core.c |  2 +-
kernel/sched/deadline.c |  2 +-
kernel/sched/fair.c |  2 +-
kernel/sched/rt.c   |  2 +-
kernel/sched/sched.h| 17 -
5 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index de440456f15c..c4334a3350c5 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -874,7 +874,7 @@ void check_preempt_curr(struct rq *rq, struct task_struct 
*p, int flags)
 * this case, we can save a useless back to back clock update.
 */
if (task_on_rq_queued(rq->curr) && test_tsk_need_resched(rq->curr))
-   rq_clock_skip_update(rq, true);
+   rq_clock_skip_update(rq);
}

#ifdef CONFIG_SMP
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index d1c7bf7c7e5b..e7b3008b85bb 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1560,7 +1560,7 @@ static void yield_task_dl(struct rq *rq)
 * so we don't do microscopic update in schedule()
 * and double the fastpath cost.
 */
-   rq_clock_skip_update(rq, true);
+   rq_clock_skip_update(rq);
}

#ifdef CONFIG_SMP
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 0951d1c58d2f..54dc31e7ab9b 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7089,7 +7089,7 @@ static void yield_task_fair(struct rq *rq)
 * so we don't do microscopic update in schedule()
 * and double the fastpath cost.
 */
-   rq_clock_skip_update(rq, true);
+   rq_clock_skip_update(rq);
}

set_skip_buddy(se);
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index ad13e6242481..7aef6b4e885a 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -861,7 +861,7 @@ static int do_sched_rt_period_timer(struct rt_bandwidth 
*rt_b, int overrun)
 * 'runtime'.
 */
if (rt_rq->rt_nr_running && rq->curr == 
rq->idle)
-   rq_clock_skip_update(rq, false);
+   rq_clock_cancel_skipupdate(rq);
}
if (rt_rq->rt_time || rt_rq->rt_nr_running)
idle = 0;
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index c3deaee7a7a2..15750c222ca2 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -976,13 +976,20 @@ static inline u64 rq_clock_task(struct rq *rq)
return rq->clock_task;
}

-static inline void rq_clock_skip_update(struct rq *rq, bool skip)
+static inline void rq_clock_skip_update(struct rq *rq)
{
lockdep_assert_held(>lock);
-   if (skip)
-   rq->clock_update_flags |= RQCF_REQ_SKIP;
-   else
-   rq->clock_update_flags &= ~RQCF_REQ_SKIP;
+   rq->clock_update_flags |= RQCF_REQ_SKIP;
+}
+
+/*
+ * See rt task throttoling, which is the only time a skip
+ * request is cancelled.
+ */
+static inline void rq_clock_cancel_skipupdate(struct rq *rq)
+{
+   lockdep_assert_held(>lock);
+   rq->clock_update_flags &= ~RQCF_REQ_SKIP;
}

struct rq_flags {
--
2.13.6

Re: [PATCH] sched/rt: Fix rq->clock_update_flags < RQCF_ACT_SKIP warning

2018-04-04 Thread Peter Zijlstra 
On Mon, Apr 02, 2018 at 09:49:54AM -0700, Davidlohr Bueso wrote:
> While running rt-tests' pi_stress program I got the following splat:
> 
>   444.884597] [ cut here ]
> [  444.894784] rq->clock_update_flags < RQCF_ACT_SKIP
> [  444.894798] WARNING: CPU: 27 PID: 0 at kernel/sched/sched.h:960 
> assert_clock_updated.isra.38.part.39+0x13/0x20
> [  444.927419] Modules linked in: ...
> [  445.141336] CPU: 27 PID: 0 Comm: swapper/27 Kdump: loaded Tainted: G   
>  E 4.16.0-rc7-next-20180329-13.5-default+ #1
> [  445.168197] Hardware name: QCI QSSC-S4R/QSSC-S4R, BIOS 
> QSSC-S4R.QCI.01.00.S013.032920111005 03/29/2011
> [  445.188728] RIP: 0010:assert_clock_updated.isra.38.part.39+0x13/0x20
> [  445.202739] RSP: 0018:9af37fb03e98 EFLAGS: 00010092
> [  445.214258] RAX: 0026 RBX: 9af37faa1bc0 RCX: 
> bd064a68
> [  445.229994] RDX: 0001 RSI: 0092 RDI: 
> 0083
> [  445.245732] RBP: 0017 R08: 05f1 R09: 
> bd864ca0
> [  445.261468] R10: 3cdf0977 R11:  R12: 
> 9af186178180
> [  445.277206] R13: bd2396a0 R14: 9af37faa1bc0 R15: 
> 0017
> [  445.292944] FS:  () GS:9af37fb0() 
> knlGS:
> [  445.310790] CS:  0010 DS:  ES:  CR0: 80050033
> [  445.323457] CR2: 556773e17968 CR3: 000b3d00a002 CR4: 
> 000206e0
> [  445.339193] Call Trace:
> [  445.344578]  
> [  445.349001]  enqueue_top_rt_rq+0xf4/0x150
> [  445.357839]  ? cpufreq_dbs_governor_start+0x170/0x170
> [  445.368974]  sched_rt_rq_enqueue+0x65/0x80
> [  445.378000]  sched_rt_period_timer+0x156/0x360
> [  445.387792]  ? sched_rt_rq_enqueue+0x80/0x80
> [  445.397204]  __hrtimer_run_queues+0xfa/0x260
> [  445.406614]  hrtimer_interrupt+0xcb/0x220
> [  445.415451]  smp_apic_timer_interrupt+0x62/0x120
> [  445.425629]  apic_timer_interrupt+0xf/0x20
> [  445.434654]  
> [  445.439269] RIP: 0010:cpuidle_enter_state+0x9d/0x2b0
> [  445.450212] RSP: 0018:ac274642fec0 EFLAGS: 0246 ORIG_RAX: 
> ff13
> [  445.466908] RAX: 9af37fb21bc0 RBX: 0067952f9fb7 RCX: 
> 001f
> [  445.482645] RDX: 0067952f9fb7 RSI: 389cb571 RDI: 
> 
> [  445.498380] RBP: 0004 R08: bc01 R09: 
> c04c
> [  445.514116] R10: ac274642fea0 R11: 0eed R12: 
> cc273fb10ff0
> [  445.529853] R13: bd1813b8 R14:  R15: 
> 0067923bdc5a
> [  445.545595]  do_idle+0x183/0x1e0
> [  445.552703]  cpu_startup_entry+0x5f/0x70
> [  445.561350]  start_secondary+0x192/0x1d0
> [  445.569995]  secondary_startup_64+0xa5/0xb0
> [  445.579212] Code: 00 48 0f a3 05 9f 85 17 01 0f 83 74 ff ff ff e9 d4 b6 fe 
> ff 0f 1f 40 00 48 c7 c7 f0 fd e6 bc c6 05 2e 08 14 01 01 e8 cd 2b fc ff <0f> 
> 0b c3 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 83 ea 08
> [  445.620864] ---[ end trace 0183b70f54a7f4ba ]---
> 
> We can get rid of it be the "traditional" means of adding an 
> update_rq_clock() call
> after acquiring the rq->lock in do_sched_rt_period_timer().
> 
> The case for the rt task throttling (which this workload also hits) can be 
> ignored in
> that the skip_update call is actually bogus and quite the contrary (the 
> request bits
> are removed/reverted). By setting RQCF_UPDATED we really don't care if the 
> skip is
> happening or not and will therefore make the assert_clock_updated() check 
> happy.
> 
> Signed-off-by: Davidlohr Bueso 

Acked-by: Peter Zijlstra (Intel) 

Ingo, please merge.

> ---
>  kernel/sched/rt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 86b77987435e..ad13e6242481 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -839,6 +839,8 @@ static int do_sched_rt_period_timer(struct rt_bandwidth 
> *rt_b, int overrun)
>   continue;
>  
>   raw_spin_lock(>lock);
> + update_rq_clock(rq);
> +
>   if (rt_rq->rt_time) {
>   u64 runtime;
>  
> -- 
> 2.13.6
>

Re: [PATCH] sched/rt: Fix rq->clock_update_flags < RQCF_ACT_SKIP warning

2018-04-04 Thread Peter Zijlstra 
On Mon, Apr 02, 2018 at 09:49:54AM -0700, Davidlohr Bueso wrote:
> While running rt-tests' pi_stress program I got the following splat:
> 
>   444.884597] [ cut here ]
> [  444.894784] rq->clock_update_flags < RQCF_ACT_SKIP
> [  444.894798] WARNING: CPU: 27 PID: 0 at kernel/sched/sched.h:960 
> assert_clock_updated.isra.38.part.39+0x13/0x20
> [  444.927419] Modules linked in: ...
> [  445.141336] CPU: 27 PID: 0 Comm: swapper/27 Kdump: loaded Tainted: G   
>  E 4.16.0-rc7-next-20180329-13.5-default+ #1
> [  445.168197] Hardware name: QCI QSSC-S4R/QSSC-S4R, BIOS 
> QSSC-S4R.QCI.01.00.S013.032920111005 03/29/2011
> [  445.188728] RIP: 0010:assert_clock_updated.isra.38.part.39+0x13/0x20
> [  445.202739] RSP: 0018:9af37fb03e98 EFLAGS: 00010092
> [  445.214258] RAX: 0026 RBX: 9af37faa1bc0 RCX: 
> bd064a68
> [  445.229994] RDX: 0001 RSI: 0092 RDI: 
> 0083
> [  445.245732] RBP: 0017 R08: 05f1 R09: 
> bd864ca0
> [  445.261468] R10: 3cdf0977 R11:  R12: 
> 9af186178180
> [  445.277206] R13: bd2396a0 R14: 9af37faa1bc0 R15: 
> 0017
> [  445.292944] FS:  () GS:9af37fb0() 
> knlGS:
> [  445.310790] CS:  0010 DS:  ES:  CR0: 80050033
> [  445.323457] CR2: 556773e17968 CR3: 000b3d00a002 CR4: 
> 000206e0
> [  445.339193] Call Trace:
> [  445.344578]  
> [  445.349001]  enqueue_top_rt_rq+0xf4/0x150
> [  445.357839]  ? cpufreq_dbs_governor_start+0x170/0x170
> [  445.368974]  sched_rt_rq_enqueue+0x65/0x80
> [  445.378000]  sched_rt_period_timer+0x156/0x360
> [  445.387792]  ? sched_rt_rq_enqueue+0x80/0x80
> [  445.397204]  __hrtimer_run_queues+0xfa/0x260
> [  445.406614]  hrtimer_interrupt+0xcb/0x220
> [  445.415451]  smp_apic_timer_interrupt+0x62/0x120
> [  445.425629]  apic_timer_interrupt+0xf/0x20
> [  445.434654]  
> [  445.439269] RIP: 0010:cpuidle_enter_state+0x9d/0x2b0
> [  445.450212] RSP: 0018:ac274642fec0 EFLAGS: 0246 ORIG_RAX: 
> ff13
> [  445.466908] RAX: 9af37fb21bc0 RBX: 0067952f9fb7 RCX: 
> 001f
> [  445.482645] RDX: 0067952f9fb7 RSI: 389cb571 RDI: 
> 
> [  445.498380] RBP: 0004 R08: bc01 R09: 
> c04c
> [  445.514116] R10: ac274642fea0 R11: 0eed R12: 
> cc273fb10ff0
> [  445.529853] R13: bd1813b8 R14:  R15: 
> 0067923bdc5a
> [  445.545595]  do_idle+0x183/0x1e0
> [  445.552703]  cpu_startup_entry+0x5f/0x70
> [  445.561350]  start_secondary+0x192/0x1d0
> [  445.569995]  secondary_startup_64+0xa5/0xb0
> [  445.579212] Code: 00 48 0f a3 05 9f 85 17 01 0f 83 74 ff ff ff e9 d4 b6 fe 
> ff 0f 1f 40 00 48 c7 c7 f0 fd e6 bc c6 05 2e 08 14 01 01 e8 cd 2b fc ff <0f> 
> 0b c3 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 83 ea 08
> [  445.620864] ---[ end trace 0183b70f54a7f4ba ]---
> 
> We can get rid of it be the "traditional" means of adding an 
> update_rq_clock() call
> after acquiring the rq->lock in do_sched_rt_period_timer().
> 
> The case for the rt task throttling (which this workload also hits) can be 
> ignored in
> that the skip_update call is actually bogus and quite the contrary (the 
> request bits
> are removed/reverted). By setting RQCF_UPDATED we really don't care if the 
> skip is
> happening or not and will therefore make the assert_clock_updated() check 
> happy.
> 
> Signed-off-by: Davidlohr Bueso 

Acked-by: Peter Zijlstra (Intel) 

Ingo, please merge.

> ---
>  kernel/sched/rt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 86b77987435e..ad13e6242481 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -839,6 +839,8 @@ static int do_sched_rt_period_timer(struct rt_bandwidth 
> *rt_b, int overrun)
>   continue;
>  
>   raw_spin_lock(>lock);
> + update_rq_clock(rq);
> +
>   if (rt_rq->rt_time) {
>   u64 runtime;
>  
> -- 
> 2.13.6
>

Re: [PATCH] sched/rt: Fix rq->clock_update_flags < RQCF_ACT_SKIP warning

2018-04-03 Thread Matt Fleming 
On Mon, 02 Apr, at 09:49:54AM, Davidlohr Bueso wrote:
> 
> We can get rid of it be the "traditional" means of adding an 
> update_rq_clock() call
> after acquiring the rq->lock in do_sched_rt_period_timer().
> 
> The case for the rt task throttling (which this workload also hits) can be 
> ignored in
> that the skip_update call is actually bogus and quite the contrary (the 
> request bits
> are removed/reverted). By setting RQCF_UPDATED we really don't care if the 
> skip is
> happening or not and will therefore make the assert_clock_updated() check 
> happy.
> 
> Signed-off-by: Davidlohr Bueso 
> ---
>  kernel/sched/rt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 86b77987435e..ad13e6242481 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -839,6 +839,8 @@ static int do_sched_rt_period_timer(struct rt_bandwidth 
> *rt_b, int overrun)
>   continue;
>  
>   raw_spin_lock(>lock);
> + update_rq_clock(rq);
> +
>   if (rt_rq->rt_time) {
>   u64 runtime;

Looks good to me.

Reviewed-by: Matt Fleming

Re: [PATCH] sched/rt: Fix rq->clock_update_flags < RQCF_ACT_SKIP warning

2018-04-03 Thread Matt Fleming 
On Mon, 02 Apr, at 09:49:54AM, Davidlohr Bueso wrote:
> 
> We can get rid of it be the "traditional" means of adding an 
> update_rq_clock() call
> after acquiring the rq->lock in do_sched_rt_period_timer().
> 
> The case for the rt task throttling (which this workload also hits) can be 
> ignored in
> that the skip_update call is actually bogus and quite the contrary (the 
> request bits
> are removed/reverted). By setting RQCF_UPDATED we really don't care if the 
> skip is
> happening or not and will therefore make the assert_clock_updated() check 
> happy.
> 
> Signed-off-by: Davidlohr Bueso 
> ---
>  kernel/sched/rt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 86b77987435e..ad13e6242481 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -839,6 +839,8 @@ static int do_sched_rt_period_timer(struct rt_bandwidth 
> *rt_b, int overrun)
>   continue;
>  
>   raw_spin_lock(>lock);
> + update_rq_clock(rq);
> +
>   if (rt_rq->rt_time) {
>   u64 runtime;

Looks good to me.

Reviewed-by: Matt Fleming

[PATCH] sched/rt: Fix rq->clock_update_flags < RQCF_ACT_SKIP warning

2018-04-02 Thread Davidlohr Bueso 
While running rt-tests' pi_stress program I got the following splat:

  444.884597] [ cut here ]
[  444.894784] rq->clock_update_flags < RQCF_ACT_SKIP
[  444.894798] WARNING: CPU: 27 PID: 0 at kernel/sched/sched.h:960 
assert_clock_updated.isra.38.part.39+0x13/0x20
[  444.927419] Modules linked in: ...
[  445.141336] CPU: 27 PID: 0 Comm: swapper/27 Kdump: loaded Tainted: G 
   E 4.16.0-rc7-next-20180329-13.5-default+ #1
[  445.168197] Hardware name: QCI QSSC-S4R/QSSC-S4R, BIOS 
QSSC-S4R.QCI.01.00.S013.032920111005 03/29/2011
[  445.188728] RIP: 0010:assert_clock_updated.isra.38.part.39+0x13/0x20
[  445.202739] RSP: 0018:9af37fb03e98 EFLAGS: 00010092
[  445.214258] RAX: 0026 RBX: 9af37faa1bc0 RCX: bd064a68
[  445.229994] RDX: 0001 RSI: 0092 RDI: 0083
[  445.245732] RBP: 0017 R08: 05f1 R09: bd864ca0
[  445.261468] R10: 3cdf0977 R11:  R12: 9af186178180
[  445.277206] R13: bd2396a0 R14: 9af37faa1bc0 R15: 0017
[  445.292944] FS:  () GS:9af37fb0() 
knlGS:
[  445.310790] CS:  0010 DS:  ES:  CR0: 80050033
[  445.323457] CR2: 556773e17968 CR3: 000b3d00a002 CR4: 000206e0
[  445.339193] Call Trace:
[  445.344578]  
[  445.349001]  enqueue_top_rt_rq+0xf4/0x150
[  445.357839]  ? cpufreq_dbs_governor_start+0x170/0x170
[  445.368974]  sched_rt_rq_enqueue+0x65/0x80
[  445.378000]  sched_rt_period_timer+0x156/0x360
[  445.387792]  ? sched_rt_rq_enqueue+0x80/0x80
[  445.397204]  __hrtimer_run_queues+0xfa/0x260
[  445.406614]  hrtimer_interrupt+0xcb/0x220
[  445.415451]  smp_apic_timer_interrupt+0x62/0x120
[  445.425629]  apic_timer_interrupt+0xf/0x20
[  445.434654]  
[  445.439269] RIP: 0010:cpuidle_enter_state+0x9d/0x2b0
[  445.450212] RSP: 0018:ac274642fec0 EFLAGS: 0246 ORIG_RAX: 
ff13
[  445.466908] RAX: 9af37fb21bc0 RBX: 0067952f9fb7 RCX: 001f
[  445.482645] RDX: 0067952f9fb7 RSI: 389cb571 RDI: 
[  445.498380] RBP: 0004 R08: bc01 R09: c04c
[  445.514116] R10: ac274642fea0 R11: 0eed R12: cc273fb10ff0
[  445.529853] R13: bd1813b8 R14:  R15: 0067923bdc5a
[  445.545595]  do_idle+0x183/0x1e0
[  445.552703]  cpu_startup_entry+0x5f/0x70
[  445.561350]  start_secondary+0x192/0x1d0
[  445.569995]  secondary_startup_64+0xa5/0xb0
[  445.579212] Code: 00 48 0f a3 05 9f 85 17 01 0f 83 74 ff ff ff e9 d4 b6 fe 
ff 0f 1f 40 00 48 c7 c7 f0 fd e6 bc c6 05 2e 08 14 01 01 e8 cd 2b fc ff <0f> 0b 
c3 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 83 ea 08
[  445.620864] ---[ end trace 0183b70f54a7f4ba ]---

We can get rid of it be the "traditional" means of adding an update_rq_clock() 
call
after acquiring the rq->lock in do_sched_rt_period_timer().

The case for the rt task throttling (which this workload also hits) can be 
ignored in
that the skip_update call is actually bogus and quite the contrary (the request 
bits
are removed/reverted). By setting RQCF_UPDATED we really don't care if the skip 
is
happening or not and will therefore make the assert_clock_updated() check happy.

Signed-off-by: Davidlohr Bueso 
---
 kernel/sched/rt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 86b77987435e..ad13e6242481 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -839,6 +839,8 @@ static int do_sched_rt_period_timer(struct rt_bandwidth 
*rt_b, int overrun)
continue;
 
raw_spin_lock(>lock);
+   update_rq_clock(rq);
+
if (rt_rq->rt_time) {
u64 runtime;
 
-- 
2.13.6

[PATCH] sched/rt: Fix rq->clock_update_flags < RQCF_ACT_SKIP warning

2018-04-02 Thread Davidlohr Bueso 
While running rt-tests' pi_stress program I got the following splat:

  444.884597] [ cut here ]
[  444.894784] rq->clock_update_flags < RQCF_ACT_SKIP
[  444.894798] WARNING: CPU: 27 PID: 0 at kernel/sched/sched.h:960 
assert_clock_updated.isra.38.part.39+0x13/0x20
[  444.927419] Modules linked in: ...
[  445.141336] CPU: 27 PID: 0 Comm: swapper/27 Kdump: loaded Tainted: G 
   E 4.16.0-rc7-next-20180329-13.5-default+ #1
[  445.168197] Hardware name: QCI QSSC-S4R/QSSC-S4R, BIOS 
QSSC-S4R.QCI.01.00.S013.032920111005 03/29/2011
[  445.188728] RIP: 0010:assert_clock_updated.isra.38.part.39+0x13/0x20
[  445.202739] RSP: 0018:9af37fb03e98 EFLAGS: 00010092
[  445.214258] RAX: 0026 RBX: 9af37faa1bc0 RCX: bd064a68
[  445.229994] RDX: 0001 RSI: 0092 RDI: 0083
[  445.245732] RBP: 0017 R08: 05f1 R09: bd864ca0
[  445.261468] R10: 3cdf0977 R11:  R12: 9af186178180
[  445.277206] R13: bd2396a0 R14: 9af37faa1bc0 R15: 0017
[  445.292944] FS:  () GS:9af37fb0() 
knlGS:
[  445.310790] CS:  0010 DS:  ES:  CR0: 80050033
[  445.323457] CR2: 556773e17968 CR3: 000b3d00a002 CR4: 000206e0
[  445.339193] Call Trace:
[  445.344578]  
[  445.349001]  enqueue_top_rt_rq+0xf4/0x150
[  445.357839]  ? cpufreq_dbs_governor_start+0x170/0x170
[  445.368974]  sched_rt_rq_enqueue+0x65/0x80
[  445.378000]  sched_rt_period_timer+0x156/0x360
[  445.387792]  ? sched_rt_rq_enqueue+0x80/0x80
[  445.397204]  __hrtimer_run_queues+0xfa/0x260
[  445.406614]  hrtimer_interrupt+0xcb/0x220
[  445.415451]  smp_apic_timer_interrupt+0x62/0x120
[  445.425629]  apic_timer_interrupt+0xf/0x20
[  445.434654]  
[  445.439269] RIP: 0010:cpuidle_enter_state+0x9d/0x2b0
[  445.450212] RSP: 0018:ac274642fec0 EFLAGS: 0246 ORIG_RAX: 
ff13
[  445.466908] RAX: 9af37fb21bc0 RBX: 0067952f9fb7 RCX: 001f
[  445.482645] RDX: 0067952f9fb7 RSI: 389cb571 RDI: 
[  445.498380] RBP: 0004 R08: bc01 R09: c04c
[  445.514116] R10: ac274642fea0 R11: 0eed R12: cc273fb10ff0
[  445.529853] R13: bd1813b8 R14:  R15: 0067923bdc5a
[  445.545595]  do_idle+0x183/0x1e0
[  445.552703]  cpu_startup_entry+0x5f/0x70
[  445.561350]  start_secondary+0x192/0x1d0
[  445.569995]  secondary_startup_64+0xa5/0xb0
[  445.579212] Code: 00 48 0f a3 05 9f 85 17 01 0f 83 74 ff ff ff e9 d4 b6 fe 
ff 0f 1f 40 00 48 c7 c7 f0 fd e6 bc c6 05 2e 08 14 01 01 e8 cd 2b fc ff <0f> 0b 
c3 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 83 ea 08
[  445.620864] ---[ end trace 0183b70f54a7f4ba ]---

We can get rid of it be the "traditional" means of adding an update_rq_clock() 
call
after acquiring the rq->lock in do_sched_rt_period_timer().

The case for the rt task throttling (which this workload also hits) can be 
ignored in
that the skip_update call is actually bogus and quite the contrary (the request 
bits
are removed/reverted). By setting RQCF_UPDATED we really don't care if the skip 
is
happening or not and will therefore make the assert_clock_updated() check happy.

Signed-off-by: Davidlohr Bueso 
---
 kernel/sched/rt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 86b77987435e..ad13e6242481 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -839,6 +839,8 @@ static int do_sched_rt_period_timer(struct rt_bandwidth 
*rt_b, int overrun)
continue;
 
raw_spin_lock(>lock);
+   update_rq_clock(rq);
+
if (rt_rq->rt_time) {
u64 runtime;
 
-- 
2.13.6