Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331087 > unrolled thread

Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted bandwidth

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Juri Lelli <juri.lelli@arm.com> - 2016-02-10 12:40 +0100
    Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth luca abeni <luca.abeni@unitn.it> - 2016-02-10 12:50 +0100
      Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Juri Lelli <juri.lelli@arm.com> - 2016-02-10 13:00 +0100
        Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth luca abeni <luca.abeni@unitn.it> - 2016-02-19 14:50 +0100
          Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Steven Rostedt <rostedt@goodmis.org> - 2016-02-19 15:30 +0100
            Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Steven Rostedt <rostedt@goodmis.org> - 2016-02-19 16:00 +0100
            Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth luca abeni <luca.abeni@unitn.it> - 2016-02-19 16:00 +0100
              Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth luca abeni <luca.abeni@unitn.it> - 2016-02-22 12:10 +0100
        [PATCH 0/3] cleanup per rq tracking of admitted bandwidth  Luca Abeni <luca.abeni@unitn.it> - 2016-02-22 12:00 +0100
          [PATCH 2/4] Move the remaining __dl_{sub,add}_ac() calls from core.c to deadline.c Luca Abeni <luca.abeni@unitn.it> - 2016-02-22 12:00 +0100
          [PATCH 1/4] Move some calls to __dl_{sub,add}_ac() from core.c to deadline.c Luca Abeni <luca.abeni@unitn.it> - 2016-02-22 12:00 +0100
          [PATCH 3/4] Remove dl_new Luca Abeni <luca.abeni@unitn.it> - 2016-02-22 12:00 +0100
    Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth luca abeni <luca.abeni@unitn.it> - 2016-02-10 13:50 +0100
      Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Juri Lelli <juri.lelli@arm.com> - 2016-02-10 14:50 +0100
    Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Steven Rostedt <rostedt@goodmis.org> - 2016-02-10 15:40 +0100
      Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Juri Lelli <juri.lelli@arm.com> - 2016-02-10 17:30 +0100
        Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Juri Lelli <juri.lelli@arm.com> - 2016-02-11 13:20 +0100
          Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth luca abeni <luca.abeni@unitn.it> - 2016-02-11 13:30 +0100
            Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Juri Lelli <juri.lelli@arm.com> - 2016-02-11 13:30 +0100
              Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Juri Lelli <juri.lelli@arm.com> - 2016-02-11 13:50 +0100
                Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth luca abeni <luca.abeni@unitn.it> - 2016-02-11 14:10 +0100
                  Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Steven Rostedt <rostedt@goodmis.org> - 2016-02-11 15:30 +0100
                    Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Juri Lelli <juri.lelli@arm.com> - 2016-02-11 18:10 +0100
                      Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Peter Zijlstra <peterz@infradead.org> - 2016-02-12 18:10 +0100
                        Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Juri Lelli <juri.lelli@arm.com> - 2016-02-12 18:20 +0100
                    Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth Luca Abeni <luca.abeni@unitn.it> - 2016-02-11 22:50 +0100
              Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted  bandwidth luca abeni <luca.abeni@unitn.it> - 2016-02-11 13:50 +0100

Page 1 of 2  [1] 2  Next page →

#1331087 — Re: [PATCH 1/2] sched/deadline: add per rq tracking of admitted bandwidth

Hi,

I've updated this patch since, with a bit more testing and talking with
Luca in private, I realized that the previous version didn't manage
switching back and forth from SCHED_DEADLINE correctly. Thanks a lot
Luca for your feedback (even if not visible on the list).

I updated the testing branch accordingly and added a test to my tests
that stresses switch-in/switch-out.

I don't particularly like the fact that we break the scheduling classes
abstraction in __dl_overflow(), so I think a little bit of refactoring
is still needed. But that can also happen afterwards, if we fix the
problem with root domains.

Best,

- Juri

--->8---

From 62f70ca3051672dce209e8355cf5eddc9d825c2a Mon Sep 17 00:00:00 2001
From: Juri Lelli <juri.lelli@arm.com>
Date: Sat, 6 Feb 2016 12:41:09 +0000
Subject: [PATCH 1/2] sched/deadline: add per rq tracking of admitted bandwidth

Currently SCHED_DEADLINE scheduling policy tracks bandwidth of tasks
that passed admission control at root_domain level only. This creates
problems when such data structure(s) are destroyed, when we reconfigure
scheduling domains for example.

This is part one of two changes required to fix the problem. In this
patch we add per-rq tracking of admitted bandwidth. Tasks bring with
them their bandwidth contribution when they enter the system and are
enqueued for the first time. Contributions are then moved around when
migrations happen and removed when tasks die.

Per-rq admitted bandwidth information will be leveraged in the next
commit to save/restore per-rq bandwidth contribution towards the root
domain (using the rq_{online,offline} mechanism).

Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Reported-by: Wanpeng Li <wanpeng.li@hotmail.com>
Reported-by: Steven Rostedt <rostedt@goodmis.org>
[ original patch by ]
Signed-off-by: Luca Abeni <luca.abeni@unitn.it>
Signed-off-by: Juri Lelli <juri.lelli@arm.com>
---
 kernel/sched/core.c     |  6 +++++-
 kernel/sched/deadline.c | 16 +++++++++++++++-
 kernel/sched/sched.h    | 22 ++++++++++++++++++++++
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 9503d59..0ee0ec2 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2432,7 +2432,7 @@ static int dl_overflow(struct task_struct *p, int policy,
 	u64 new_bw = dl_policy(policy) ? to_ratio(period, runtime) : 0;
 	int cpus, err = -1;
 
-	if (new_bw == p->dl.dl_bw)
+	if (task_has_dl_policy(p) && new_bw == p->dl.dl_bw)
 		return 0;
 
 	/*
@@ -2445,14 +2445,18 @@ static int dl_overflow(struct task_struct *p, int policy,
 	if (dl_policy(policy) && !task_has_dl_policy(p) &&
 	    !__dl_overflow(dl_b, cpus, 0, new_bw)) {
 		__dl_add(dl_b, new_bw);
+		__dl_add_ac(task_rq(p), new_bw);
 		err = 0;
 	} else if (dl_policy(policy) && task_has_dl_policy(p) &&
 		   !__dl_overflow(dl_b, cpus, p->dl.dl_bw, new_bw)) {
 		__dl_clear(dl_b, p->dl.dl_bw);
+		__dl_sub_ac(task_rq(p), p->dl.dl_bw);
 		__dl_add(dl_b, new_bw);
+		__dl_add_ac(task_rq(p), new_bw);
 		err = 0;
 	} else if (!dl_policy(policy) && task_has_dl_policy(p)) {
 		__dl_clear(dl_b, p->dl.dl_bw);
+		__dl_sub_ac(task_rq(p), p->dl.dl_bw);
 		err = 0;
 	}
 	raw_spin_unlock(&dl_b->lock);
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index cd64c97..8ac0fee 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -83,6 +83,7 @@ void init_dl_rq(struct dl_rq *dl_rq)
 #else
 	init_dl_bw(&dl_rq->dl_bw);
 #endif
+	dl_rq->ac_bw = 0;
 }
 
 #ifdef CONFIG_SMP
@@ -278,8 +279,10 @@ static struct rq *dl_task_offline_migration(struct rq *rq, struct task_struct *p
 	 * By now the task is replenished and enqueued; migrate it.
 	 */
 	deactivate_task(rq, p, 0);
+	__dl_sub_ac(rq, p->dl.dl_bw);
 	set_task_cpu(p, later_rq->cpu);
 	activate_task(later_rq, p, 0);
+	__dl_add_ac(later_rq, p->dl.dl_bw);
 
 	if (!fallback)
 		resched_curr(later_rq);
@@ -597,7 +600,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
 
 	/*
 	 * The task might have changed its scheduling policy to something
-	 * different than SCHED_DEADLINE (through switched_fromd_dl()).
+	 * different than SCHED_DEADLINE (through switched_from_dl()).
 	 */
 	if (!dl_task(p)) {
 		__dl_clear_params(p);
@@ -955,6 +958,9 @@ static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 		return;
 	}
 
+	if (p->on_rq == TASK_ON_RQ_MIGRATING)
+		__dl_add_ac(rq, p->dl.dl_bw);
+
 	/*
 	 * If p is throttled, we do nothing. In fact, if it exhausted
 	 * its budget it needs a replenishment and, since it now is on
@@ -980,6 +986,8 @@ static void dequeue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 {
 	update_curr_dl(rq);
 	__dequeue_task_dl(rq, p, flags);
+	if (p->on_rq == TASK_ON_RQ_MIGRATING)
+		__dl_sub_ac(rq, p->dl.dl_bw);
 }
 
 /*
@@ -1219,6 +1227,8 @@ static void task_dead_dl(struct task_struct *p)
 {
 	struct dl_bw *dl_b = dl_bw_of(task_cpu(p));
 
+	__dl_sub_ac(task_rq(p), p->dl.dl_bw);
+
 	/*
 	 * Since we are TASK_DEAD we won't slip out of the domain!
 	 */
@@ -1511,8 +1521,10 @@ retry:
 	}
 
 	deactivate_task(rq, next_task, 0);
+	__dl_sub_ac(rq, next_task->dl.dl_bw);
 	set_task_cpu(next_task, later_rq->cpu);
 	activate_task(later_rq, next_task, 0);
+	__dl_add_ac(later_rq, next_task->dl.dl_bw);
 	ret = 1;
 
 	resched_curr(later_rq);
@@ -1599,8 +1611,10 @@ static void pull_dl_task(struct rq *this_rq)
 			resched = true;
 
 			deactivate_task(src_rq, p, 0);
+			__dl_sub_ac(src_rq, p->dl.dl_bw);
 			set_task_cpu(p, this_cpu);
 			activate_task(this_rq, p, 0);
+			__dl_add_ac(this_rq, p->dl.dl_bw);
 			dmin = p->dl.deadline;
 
 			/* Is there any other task even earlier? */
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 10f1637..242907f 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -519,6 +519,14 @@ struct dl_rq {
 #else
 	struct dl_bw dl_bw;
 #endif
+
+	/*
+	 * ac_bw keeps track of per rq admitted bandwidth. It only changes
+	 * when a new task is admitted, it dies, it changes scheduling policy
+	 * or is migrated to another rq. It is used to correctly save/resore
+	 * total_bw on root_domain changes.
+	 */
+	u64 ac_bw;
 };
 
 #ifdef CONFIG_SMP
@@ -720,6 +728,20 @@ DECLARE_PER_CPU_SHARED_ALIGNED(struct rq, runqueues);
 #define cpu_curr(cpu)		(cpu_rq(cpu)->curr)
 #define raw_rq()		raw_cpu_ptr(&runqueues)
 
+static inline
+void __dl_sub_ac(struct rq *rq, u64 tsk_bw)
+{
+	WARN_ON(rq->dl.ac_bw < tsk_bw);
+
+	rq->dl.ac_bw -= tsk_bw;
+}
+
+static inline
+void __dl_add_ac(struct rq *rq, u64 tsk_bw)
+{
+	rq->dl.ac_bw += tsk_bw;
+}
+
 static inline u64 __rq_clock_broken(struct rq *rq)
 {
 	return READ_ONCE(rq->clock);
-- 
2.7.0

[toc] | [next] | [standalone]

#1331091

Hi all,

On Wed, 10 Feb 2016 11:32:58 +0000
Juri Lelli <juri.lelli@arm.com> wrote:
[...]
> @@ -2445,14 +2445,18 @@ static int dl_overflow(struct task_struct *p,
> int policy, if (dl_policy(policy) && !task_has_dl_policy(p) &&
>  	    !__dl_overflow(dl_b, cpus, 0, new_bw)) {
>  		__dl_add(dl_b, new_bw);
> +		__dl_add_ac(task_rq(p), new_bw);
>  		err = 0;
>  	} else if (dl_policy(policy) && task_has_dl_policy(p) &&
>  		   !__dl_overflow(dl_b, cpus, p->dl.dl_bw, new_bw)) {
>  		__dl_clear(dl_b, p->dl.dl_bw);
> +		__dl_sub_ac(task_rq(p), p->dl.dl_bw);
>  		__dl_add(dl_b, new_bw);
> +		__dl_add_ac(task_rq(p), new_bw);
>  		err = 0;
>  	} else if (!dl_policy(policy) && task_has_dl_policy(p)) {
>  		__dl_clear(dl_b, p->dl.dl_bw);
> +		__dl_sub_ac(task_rq(p), p->dl.dl_bw);

Instead of adding __dl_add_ac() and __dl_sub_ac) calls here, maybe they
can be added in switched_to_dl() and switched_from_dl()?

I'll test this idea locally, and I'll send an updated patch if it works.



			Luca

[toc] | [prev] | [next] | [standalone]

#1331095

On 10/02/16 12:43, Luca Abeni wrote:
> Hi all,
> 

Hi Luca,

> On Wed, 10 Feb 2016 11:32:58 +0000
> Juri Lelli <juri.lelli@arm.com> wrote:
> [...]
> > @@ -2445,14 +2445,18 @@ static int dl_overflow(struct task_struct *p,
> > int policy, if (dl_policy(policy) && !task_has_dl_policy(p) &&
> >  	    !__dl_overflow(dl_b, cpus, 0, new_bw)) {
> >  		__dl_add(dl_b, new_bw);
> > +		__dl_add_ac(task_rq(p), new_bw);
> >  		err = 0;
> >  	} else if (dl_policy(policy) && task_has_dl_policy(p) &&
> >  		   !__dl_overflow(dl_b, cpus, p->dl.dl_bw, new_bw)) {
> >  		__dl_clear(dl_b, p->dl.dl_bw);
> > +		__dl_sub_ac(task_rq(p), p->dl.dl_bw);
> >  		__dl_add(dl_b, new_bw);
> > +		__dl_add_ac(task_rq(p), new_bw);
> >  		err = 0;
> >  	} else if (!dl_policy(policy) && task_has_dl_policy(p)) {
> >  		__dl_clear(dl_b, p->dl.dl_bw);
> > +		__dl_sub_ac(task_rq(p), p->dl.dl_bw);
> 
> Instead of adding __dl_add_ac() and __dl_sub_ac) calls here, maybe they
> can be added in switched_to_dl() and switched_from_dl()?
> 

That might work too yes. I think there is value if we are able to move
all __dl_{add,sub}_ac calls in deadline.c. Actually, we should probably
move __dl_{add,clear} there as well, so that changes to rq and
root_domain happen at the same time.

> I'll test this idea locally, and I'll send an updated patch if it works.
> 

Thanks! Will wait for it :).

Best,

- Juri

[toc] | [prev] | [next] | [standalone]

#1338199

[Multipart message — attachments visible in raw view] — view raw

Hi all,

On Wed, 10 Feb 2016 11:58:12 +0000
Juri Lelli <juri.lelli@arm.com> wrote:
[...]
> > >  	} else if (!dl_policy(policy) && task_has_dl_policy(p)) {
> > >  		__dl_clear(dl_b, p->dl.dl_bw);
> > > +		__dl_sub_ac(task_rq(p), p->dl.dl_bw);
> > 
> > Instead of adding __dl_add_ac() and __dl_sub_ac) calls here, maybe
> > they can be added in switched_to_dl() and switched_from_dl()?
> > 
> 
> That might work too yes. I think there is value if we are able to move
> all __dl_{add,sub}_ac calls in deadline.c. Actually, we should
> probably move __dl_{add,clear} there as well, so that changes to rq
> and root_domain happen at the same time.
> 
> > I'll test this idea locally, and I'll send an updated patch if it
> > works.
> > 
> 
> Thanks! Will wait for it :).

I know there are still open issues with select_fallback_rq() and
similar stuff, but as promised I worked on moving the __dl_*_ac() calls
from core.c to deadline.c (which is orthogonal respect to the
select_fallback_rq() thing). I post these patches so that people can
see how the code would look like after moving __dl_*_ac() calls to
deadline.c and can provide some comments; the patches are not submitted
for inclusion (yet).


So, the first attached patch (to be applied over Juri's patch) just
moves two __dl_sub_ac() and __dl_add_ac() invocations from
dl_overflow() to deadline.c, using the switched_from_dl() and
switched_to_dl() methods. This should cover the cases of tasks moving
from SCHED_{OTHER,RT} to SCHED_DEADLINE and vice-versa. The case in
which the deadline parameters of a task are changed still needs some
__dl_* calls in dl_overflow().

These calls are moved to deadline.c by the second attached patch, which
uses prio_changed_dl()... Here, prio_changed_dl() needs to know the
"old" task utilization, while it is given  the "old task
priority" (which, for a deadline task is equal to the current
priority)... So, I changed the meaning of the third parameter of
prio_changed_dl(), from "old priority" to "old utilization".
I know that changing the meaning of a parameter between different
scheduling classes might be a bad idea... But the "prio_changed" method
seems to be designed for priority-based schedulers only, and does not
provide good information to the SCHED_DEADLINE scheduling class... The
alternative is to change the prototype of the function, adding an
"oldbw" (or, better, "oldperiod" and "oldruntime") parameter.
Let me know what you think about this.

Notice that the second patch also contains two hunks that can be
extracted from it (if needed):
1) remove the call to switched_to_dl() from prio_changed_dl(): this
   call seems to be useless
2) change __sched_setscheduler() to invoke prio_changed for deadline
   tasks changing their parameters. Currently, this method is not
   called (because when changing parameters deadline tasks do not
   change their priority, so new_effective_prio == oldprio). I suspect
   calling prio_changed is the correct behaviour; of course, this can
   be done in different ways, let me know your opinion.

I tested these two patches in various ways, and I do not see
regressions respect to Juri's patch (but I expect issues with
select_fallback_rq()... BTW, if anyone can provide some testcases for
it, I can try to fix that case too).

Finally, when playing with switched_to_dl() I realized that it can be
used to remove the dl_new field. So, I wrote patch 0003 (attached),
which seems to be working correctly, but I am probably missing some
important tests. Let me know what you think about it: I think it might
be a nice simplification of the code, but as usual I might be missing
something :)



			Thanks,
				Luca

[toc] | [prev] | [next] | [standalone]

#1338230

On Fri, 19 Feb 2016 14:43:47 +0100
luca abeni <luca.abeni@unitn.it> wrote:


> So, the first attached patch (to be applied over Juri's patch) just
> moves two __dl_sub_ac() and __dl_add_ac() invocations from
> dl_overflow() to deadline.c, using the switched_from_dl() and
> switched_to_dl() methods. This should cover the cases of tasks moving
> from SCHED_{OTHER,RT} to SCHED_DEADLINE and vice-versa. The case in
> which the deadline parameters of a task are changed still needs some
> __dl_* calls in dl_overflow().

Hi Luca,

Please send the patches in a patch series. It is very hard to review
patches that are attachments. And our scripts are made to apply patches
from mailing lists. Having attachments just makes the job harder.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1338257

On Fri, 19 Feb 2016 15:53:06 +0100
luca abeni <luca.abeni@unitn.it> wrote:


> Sorry about that; I was in hurry, and I tried to do the quickest
> thing... I'll resend the patches in a more appropriate way on Monday.

Thanks! No rush.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1338263

On Fri, 19 Feb 2016 09:20:08 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 19 Feb 2016 14:43:47 +0100
> luca abeni <luca.abeni@unitn.it> wrote:
> 
> 
> > So, the first attached patch (to be applied over Juri's patch) just
> > moves two __dl_sub_ac() and __dl_add_ac() invocations from
> > dl_overflow() to deadline.c, using the switched_from_dl() and
> > switched_to_dl() methods. This should cover the cases of tasks
> > moving from SCHED_{OTHER,RT} to SCHED_DEADLINE and vice-versa. The
> > case in which the deadline parameters of a task are changed still
> > needs some __dl_* calls in dl_overflow().
> 
> Hi Luca,
> 
> Please send the patches in a patch series. It is very hard to review
> patches that are attachments. And our scripts are made to apply
> patches from mailing lists. Having attachments just makes the job
> harder.

Sorry about that; I was in hurry, and I tried to do the quickest
thing... I'll resend the patches in a more appropriate way on Monday.


			Luca

[toc] | [prev] | [next] | [standalone]

#1339345

On Fri, 19 Feb 2016 15:53:06 +0100
luca abeni <luca.abeni@unitn.it> wrote:
[...]
> > Please send the patches in a patch series. It is very hard to review
> > patches that are attachments. And our scripts are made to apply
> > patches from mailing lists. Having attachments just makes the job
> > harder.
> 
> Sorry about that; I was in hurry, and I tried to do the quickest
> thing... I'll resend the patches in a more appropriate way on Monday.

Ok; I just re-sent the patches with git sent-email. Hopefully, I
managed to send them as a reply to Juri's original patch, so that the
context of the discussion is not lost.

I just realized that the subjects of the emails say "[*/4]" instead of
"[*/3]" because of a stupid error of mine; I hope this is not a problem
(the patches are sent only for discussion). If this is a problem, I can
resend, of course.


			Thanks (and sorry for the mess),
				Luca

[toc] | [prev] | [next] | [standalone]

#1339339 — [PATCH 0/3] cleanup per rq tracking of admitted bandwidth

I know there are still open issues with select_fallback_rq() and
similar stuff, but as promised I worked on moving the __dl_*_ac() calls
from core.c to deadline.c (which is orthogonal respect to the
select_fallback_rq() thing). I post these patches so that people can
see how the code would look like after moving __dl_*_ac() calls to
deadline.c and can provide some comments; the patches are not submitted
for inclusion (yet).

All the following patches have to be applied over Juri's patch.


So, patch 0001 just moves two __dl_sub_ac() and __dl_add_ac()
invocations from dl_overflow() to deadline.c, using the
switched_from_dl() and switched_to_dl() methods. This should cover
the cases of tasks moving from SCHED_{OTHER,RT} to SCHED_DEADLINE
and vice-versa. The case in which the deadline parameters of a
task are changed still needs some __dl_* calls in dl_overflow().

These calls are moved to deadline.c patch 0002, which uses
prio_changed_dl()... Here, prio_changed_dl() needs to know the
"old" task utilization, while it is given  the "old task
priority" (which, for a deadline task is equal to the current
priority)... So, I changed the meaning of the third parameter of
prio_changed_dl(), from "old priority" to "old utilization".
I know that changing the meaning of a parameter between different
scheduling classes might be a bad idea... But the "prio_changed" method
seems to be designed for priority-based schedulers only, and does not
provide good information to the SCHED_DEADLINE scheduling class... The
alternative is to change the prototype of the function, adding an
"oldbw" (or, better, "oldperiod" and "oldruntime") parameter.
Let me know what you think about this.

Notice that patch 0002 also contains two hunks that can be extracted
from it (if needed):
1) remove the call to switched_to_dl() from prio_changed_dl(): this
   call seems to be useless
2) change __sched_setscheduler() to invoke prio_changed for deadline
   tasks changing their parameters. Currently, this method is not
   called (because when changing parameters deadline tasks do not
   change their priority, so new_effective_prio == oldprio). I suspect
   calling prio_changed is the correct behaviour; of course, this can
   be done in different ways, let me know your opinion.

I tested patches 0001+0002 in various ways, and I do not see
regressions respect to Juri's patch (but I expect issues with
select_fallback_rq()... BTW, if anyone can provide some testcases for
it, I can try to fix that case too).

Finally, when playing with switched_to_dl() I realized that it can be
used to remove the dl_new field. So, I wrote patch 0003, which seems
to be working correctly, but I am probably missing some important
tests. Let me know what you think about it: I think it might be
a nice simplification of the code, but as usual I might be missing
something :)


Luca Abeni (4):
  Move some calls to __dl_{sub,add}_ac() from core.c to deadline.c
  Move the remaining __dl_{sub,add}_ac() calls from core.c to deadline.c
  Remove dl_new

 kernel/sched/core.c     | 13 ++++++-------
 kernel/sched/deadline.c | 43 +++++++++++++++++--------------------------
 kernel/sched/sched.h    |  5 +++++
 3 files changed, 28 insertions(+), 33 deletions(-)

-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1339340 — [PATCH 2/4] Move the remaining __dl_{sub,add}_ac() calls from core.c to deadline.c

To move these calls from dl_overflow() to deadline.c, we must change
the meaning of the third parameter of prio_changed_dl().
Instead of passing the "old priority" (which is always equal to the current
one, for SCHED_DEADLINE) we pass the old utilization. An alternative approach
is to change the prototype of the "prio_changed" method of the scheduling
class.
---
 kernel/sched/core.c     | 10 ++++++----
 kernel/sched/deadline.c | 10 +++++++---
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index a4f08d1..5dc12db 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2449,9 +2449,7 @@ static int dl_overflow(struct task_struct *p, int policy,
 	} else if (dl_policy(policy) && task_has_dl_policy(p) &&
 		   !__dl_overflow(dl_b, cpus, p->dl.dl_bw, new_bw)) {
 		__dl_clear(dl_b, p->dl.dl_bw);
-		__dl_sub_ac(task_rq(p), p->dl.dl_bw);
 		__dl_add(dl_b, new_bw);
-		__dl_add_ac(task_rq(p), new_bw);
 		err = 0;
 	} else if (!dl_policy(policy) && task_has_dl_policy(p)) {
 		__dl_clear(dl_b, p->dl.dl_bw);
@@ -3522,6 +3520,7 @@ void rt_mutex_setprio(struct task_struct *p, int prio)
 		} else
 			p->dl.dl_boosted = 0;
 		p->sched_class = &dl_sched_class;
+		oldprio = 0;
 	} else if (rt_prio(prio)) {
 		if (dl_prio(oldprio))
 			p->dl.dl_boosted = 0;
@@ -3891,7 +3890,7 @@ static int __sched_setscheduler(struct task_struct *p,
 {
 	int newprio = dl_policy(attr->sched_policy) ? MAX_DL_PRIO - 1 :
 		      MAX_RT_PRIO - 1 - attr->sched_priority;
-	int retval, oldprio, oldpolicy = -1, queued, running;
+	int retval, oldprio, oldbw, oldpolicy = -1, queued, running;
 	int new_effective_prio, policy = attr->sched_policy;
 	unsigned long flags;
 	const struct sched_class *prev_class;
@@ -4069,6 +4068,7 @@ change:
 
 	p->sched_reset_on_fork = reset_on_fork;
 	oldprio = p->prio;
+	oldbw = p->dl.dl_bw;
 
 	if (pi) {
 		/*
@@ -4081,6 +4081,8 @@ change:
 		new_effective_prio = rt_mutex_get_effective_prio(p, newprio);
 		if (new_effective_prio == oldprio) {
 			__setscheduler_params(p, attr);
+			if (p->sched_class == &dl_sched_class)
+				p->sched_class->prio_changed(rq, p, oldbw);
 			task_rq_unlock(rq, p, &flags);
 			return 0;
 		}
@@ -4110,7 +4112,7 @@ change:
 		enqueue_task(rq, p, enqueue_flags);
 	}
 
-	check_class_changed(rq, p, prev_class, oldprio);
+	check_class_changed(rq, p, prev_class, ((prev_class == &dl_sched_class) && (p->sched_class == &dl_sched_class)) ? oldbw : oldprio);
 	preempt_disable(); /* avoid rq from going away on us */
 	task_rq_unlock(rq, p, &flags);
 
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 4cc713a..959e7b7 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1757,8 +1757,13 @@ static void switched_to_dl(struct rq *rq, struct task_struct *p)
  * a push or pull operation might be needed.
  */
 static void prio_changed_dl(struct rq *rq, struct task_struct *p,
-			    int oldprio)
+			    int oldbw)
 {
+	if (oldbw) {
+		__dl_sub_ac(rq, oldbw);
+		__dl_add_ac(rq, p->dl.dl_bw);
+	}
+
 	if (task_on_rq_queued(p) || rq->curr == p) {
 #ifdef CONFIG_SMP
 		/*
@@ -1785,8 +1790,7 @@ static void prio_changed_dl(struct rq *rq, struct task_struct *p,
 		 */
 		resched_curr(rq);
 #endif /* CONFIG_SMP */
-	} else
-		switched_to_dl(rq, p);
+	}
 }
 
 const struct sched_class dl_sched_class = {
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1339341 — [PATCH 1/4] Move some calls to __dl_{sub,add}_ac() from core.c to deadline.c

This moves some deadline-specific calls from core.c (dl_overflow())
switched_from_dl() and switched_to_dl().
Some __dl_{sub,add}_ac() calls are left in dl_overflow(), to handle
the case in which the deadline parameters of a task are changed without
changing the scheduling class.
---
 kernel/sched/core.c     | 2 --
 kernel/sched/deadline.c | 3 +++
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 0ee0ec2..a4f08d1 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2445,7 +2445,6 @@ static int dl_overflow(struct task_struct *p, int policy,
 	if (dl_policy(policy) && !task_has_dl_policy(p) &&
 	    !__dl_overflow(dl_b, cpus, 0, new_bw)) {
 		__dl_add(dl_b, new_bw);
-		__dl_add_ac(task_rq(p), new_bw);
 		err = 0;
 	} else if (dl_policy(policy) && task_has_dl_policy(p) &&
 		   !__dl_overflow(dl_b, cpus, p->dl.dl_bw, new_bw)) {
@@ -2456,7 +2455,6 @@ static int dl_overflow(struct task_struct *p, int policy,
 		err = 0;
 	} else if (!dl_policy(policy) && task_has_dl_policy(p)) {
 		__dl_clear(dl_b, p->dl.dl_bw);
-		__dl_sub_ac(task_rq(p), p->dl.dl_bw);
 		err = 0;
 	}
 	raw_spin_unlock(&dl_b->lock);
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 8ac0fee..4cc713a 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1710,6 +1710,8 @@ void __init init_sched_dl_class(void)
 
 static void switched_from_dl(struct rq *rq, struct task_struct *p)
 {
+	__dl_sub_ac(task_rq(p), p->dl.dl_bw);
+
 	/*
 	 * Start the deadline timer; if we switch back to dl before this we'll
 	 * continue consuming our current CBS slice. If we stay outside of
@@ -1736,6 +1738,7 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
  */
 static void switched_to_dl(struct rq *rq, struct task_struct *p)
 {
+	__dl_add_ac(rq, p->dl.dl_bw);
 	if (task_on_rq_queued(p) && rq->curr != p) {
 #ifdef CONFIG_SMP
 		if (p->nr_cpus_allowed > 1 && rq->dl.overloaded)
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1339343 — [PATCH 3/4] Remove dl_new

switched_to_dl() can be used instead
---
 kernel/sched/core.c     |  1 -
 kernel/sched/deadline.c | 28 +++++-----------------------
 2 files changed, 5 insertions(+), 24 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 5dc12db..4246b1b 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2183,7 +2183,6 @@ void __dl_clear_params(struct task_struct *p)
 	dl_se->dl_bw = 0;
 
 	dl_se->dl_throttled = 0;
-	dl_se->dl_new = 1;
 	dl_se->dl_yielded = 0;
 }
 
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 959e7b7..12cb934 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -355,8 +355,6 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
 	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
 	struct rq *rq = rq_of_dl_rq(dl_rq);
 
-	WARN_ON(!dl_se->dl_new || dl_se->dl_throttled);
-
 	/*
 	 * We use the regular wall clock time to set deadlines in the
 	 * future; in fact, we must consider execution overheads (time
@@ -364,7 +362,6 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se,
 	 */
 	dl_se->deadline = rq_clock(rq) + pi_se->dl_deadline;
 	dl_se->runtime = pi_se->dl_runtime;
-	dl_se->dl_new = 0;
 }
 
 /*
@@ -503,15 +500,6 @@ static void update_dl_entity(struct sched_dl_entity *dl_se,
 	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
 	struct rq *rq = rq_of_dl_rq(dl_rq);
 
-	/*
-	 * The arrival of a new instance needs special treatment, i.e.,
-	 * the actual scheduling parameters have to be "renewed".
-	 */
-	if (dl_se->dl_new) {
-		setup_new_dl_entity(dl_se, pi_se);
-		return;
-	}
-
 	if (dl_time_before(dl_se->deadline, rq_clock(rq)) ||
 	    dl_entity_overflow(dl_se, pi_se, rq_clock(rq))) {
 		dl_se->deadline = rq_clock(rq) + pi_se->dl_deadline;
@@ -608,16 +596,6 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
 	}
 
 	/*
-	 * This is possible if switched_from_dl() raced against a running
-	 * callback that took the above !dl_task() path and we've since then
-	 * switched back into SCHED_DEADLINE.
-	 *
-	 * There's nothing to do except drop our task reference.
-	 */
-	if (dl_se->dl_new)
-		goto unlock;
-
-	/*
 	 * The task might have been boosted by someone else and might be in the
 	 * boosting/deboosting path, its not throttled.
 	 */
@@ -920,7 +898,7 @@ enqueue_dl_entity(struct sched_dl_entity *dl_se,
 	 * parameters of the task might need updating. Otherwise,
 	 * we want a replenishment of its runtime.
 	 */
-	if (dl_se->dl_new || flags & ENQUEUE_WAKEUP)
+	if (flags & ENQUEUE_WAKEUP)
 		update_dl_entity(dl_se, pi_se);
 	else if (flags & ENQUEUE_REPLENISH)
 		replenish_dl_entity(dl_se, pi_se);
@@ -1738,6 +1716,10 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
  */
 static void switched_to_dl(struct rq *rq, struct task_struct *p)
 {
+	if (p->dl.deadline <= rq_clock(rq)) {
+		setup_new_dl_entity(&p->dl, &p->dl);
+	}
+
 	__dl_add_ac(rq, p->dl.dl_bw);
 	if (task_on_rq_queued(p) && rq->curr != p) {
 #ifdef CONFIG_SMP
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1331121

Hi,

On Wed, 10 Feb 2016 11:32:58 +0000
Juri Lelli <juri.lelli@arm.com> wrote:
[...]
> From 62f70ca3051672dce209e8355cf5eddc9d825c2a Mon Sep 17 00:00:00 2001
> From: Juri Lelli <juri.lelli@arm.com>
> Date: Sat, 6 Feb 2016 12:41:09 +0000
> Subject: [PATCH 1/2] sched/deadline: add per rq tracking of admitted
> bandwidth
> 
> Currently SCHED_DEADLINE scheduling policy tracks bandwidth of tasks
> that passed admission control at root_domain level only. This creates
> problems when such data structure(s) are destroyed, when we
> reconfigure scheduling domains for example.
> 
> This is part one of two changes required to fix the problem. In this
> patch we add per-rq tracking of admitted bandwidth. Tasks bring with
> them their bandwidth contribution when they enter the system and are
> enqueued for the first time. Contributions are then moved around when
> migrations happen and removed when tasks die.

I think this patch actually does two different things (addressing two
separate problems):
1) it introduces the tracking of per-rq utilization (used in your next
   patch to address the root domain issues)
2) it fixes a bug in the current utilization tracking mechanism.
   Currently, a task doing
	while(1) {
		switch to SCHED_DEADLINE
		switch to SCHED_OTHER
	}
   brings dl_b->total_bw below 0. Thanks to Juri for showing me this
   problem (and how to reproduce it) in a private email.
   This happens because when the task switches back from SCHED_DEADLINE
   to SCHED_OTHER, switched_from_dl() does not clear its deadline
   parameters (they will be cleared by the deadline timer when it
   fires). But dl_overflow() removes its utilization from
   dl_b->total_bw. When the task switches back to SCHED_DEADLINE, the
   if (new_bw == p->dl.dl_bw) check in dl_overflow() prevents
   __dl_add() from being called, and so when the task switches back to
   SCHED_OTHER dl_b->total_bw becomes negative.

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 9503d59..0ee0ec2 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2432,7 +2432,7 @@ static int dl_overflow(struct task_struct *p,
> int policy, u64 new_bw = dl_policy(policy) ? to_ratio(period,
> runtime) : 0; int cpus, err = -1;
>  
> -	if (new_bw == p->dl.dl_bw)
> +	if (task_has_dl_policy(p) && new_bw == p->dl.dl_bw)
>  		return 0;

This hunk actually fixes issue 2) mentioned above, so I think it should
be committed in a short time (independently from the rest of the
patch). And maybe is a good candidate for backporting to stable kernels?



			Thanks,
				Luca

[toc] | [prev] | [next] | [standalone]

#1331179

On 10/02/16 13:48, Luca Abeni wrote:
> Hi,
> 
> On Wed, 10 Feb 2016 11:32:58 +0000
> Juri Lelli <juri.lelli@arm.com> wrote:
> [...]
> > From 62f70ca3051672dce209e8355cf5eddc9d825c2a Mon Sep 17 00:00:00 2001
> > From: Juri Lelli <juri.lelli@arm.com>
> > Date: Sat, 6 Feb 2016 12:41:09 +0000
> > Subject: [PATCH 1/2] sched/deadline: add per rq tracking of admitted
> > bandwidth
> > 
> > Currently SCHED_DEADLINE scheduling policy tracks bandwidth of tasks
> > that passed admission control at root_domain level only. This creates
> > problems when such data structure(s) are destroyed, when we
> > reconfigure scheduling domains for example.
> > 
> > This is part one of two changes required to fix the problem. In this
> > patch we add per-rq tracking of admitted bandwidth. Tasks bring with
> > them their bandwidth contribution when they enter the system and are
> > enqueued for the first time. Contributions are then moved around when
> > migrations happen and removed when tasks die.
> 
> I think this patch actually does two different things (addressing two
> separate problems):
> 1) it introduces the tracking of per-rq utilization (used in your next
>    patch to address the root domain issues)
> 2) it fixes a bug in the current utilization tracking mechanism.
>    Currently, a task doing
> 	while(1) {
> 		switch to SCHED_DEADLINE
> 		switch to SCHED_OTHER
> 	}
>    brings dl_b->total_bw below 0. Thanks to Juri for showing me this
>    problem (and how to reproduce it) in a private email.
>    This happens because when the task switches back from SCHED_DEADLINE
>    to SCHED_OTHER, switched_from_dl() does not clear its deadline
>    parameters (they will be cleared by the deadline timer when it
>    fires). But dl_overflow() removes its utilization from
>    dl_b->total_bw. When the task switches back to SCHED_DEADLINE, the
>    if (new_bw == p->dl.dl_bw) check in dl_overflow() prevents
>    __dl_add() from being called, and so when the task switches back to
>    SCHED_OTHER dl_b->total_bw becomes negative.
> 

Yep. That is also the reason why I think, whatever refactoring we are
goind to do, we should keep per-rq and root_domain accounting done
together.

> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 9503d59..0ee0ec2 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -2432,7 +2432,7 @@ static int dl_overflow(struct task_struct *p,
> > int policy, u64 new_bw = dl_policy(policy) ? to_ratio(period,
> > runtime) : 0; int cpus, err = -1;
> >  
> > -	if (new_bw == p->dl.dl_bw)
> > +	if (task_has_dl_policy(p) && new_bw == p->dl.dl_bw)
> >  		return 0;
> 
> This hunk actually fixes issue 2) mentioned above, so I think it should
> be committed in a short time (independently from the rest of the
> patch). And maybe is a good candidate for backporting to stable kernels?
> 

Yes, this is a sensible fix per se. I can split it and send it
separately.

Best,

- Juri

[toc] | [prev] | [next] | [standalone]

#1331219

On Wed, 10 Feb 2016 11:32:58 +0000
Juri Lelli <juri.lelli@arm.com> wrote:

> Hi,
> 
> I've updated this patch since, with a bit more testing and talking with
> Luca in private, I realized that the previous version didn't manage
> switching back and forth from SCHED_DEADLINE correctly. Thanks a lot
> Luca for your feedback (even if not visible on the list).
> 
> I updated the testing branch accordingly and added a test to my tests
> that stresses switch-in/switch-out.
> 
> I don't particularly like the fact that we break the scheduling classes
> abstraction in __dl_overflow(), so I think a little bit of refactoring
> is still needed. But that can also happen afterwards, if we fix the
> problem with root domains.
> 
> Best,
> 
> - Juri
> 
> --->8---  
> 
> >From 62f70ca3051672dce209e8355cf5eddc9d825c2a Mon Sep 17 00:00:00 2001  
> From: Juri Lelli <juri.lelli@arm.com>
> Date: Sat, 6 Feb 2016 12:41:09 +0000
> Subject: [PATCH 1/2] sched/deadline: add per rq tracking of admitted bandwidth
> 


I applied this patch and patch 2 and hit this:

[ 2298.134284] ------------[ cut here ]------------
[ 2298.138933] WARNING: CPU: 4 PID: 0 at /home/rostedt/work/git/linux-trace.git/kernel/sched/sched.h:735 task_dead_dl+0xc5/0xd0()
[ 2298.150350] Modules linked in: ip6table_filter ip6_tables ebtable_nat ebtables xt_CHECKSUM iptable_mangle bridge stp llc bluetooth lockd grace snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core x86_pkg_temp_thermal snd_seq vhost_net snd_seq_device tun vhost macvtap macvlan coretemp iTCO_wdt snd_pcm hp_wmi rfkill kvm_intel sparse_keymap iTCO_vendor_support snd_timer snd acpi_cpufreq kvm i2c_i801 mei_me mei soundcore lpc_ich mfd_core irqbypass wmi serio_raw uinput i915 i2c_algo_bit e1000e drm_kms_helper crc32_pclmul ptp crc32c_intel drm pps_core i2c_core video sunrpc
[ 2298.207495] CPU: 4 PID: 0 Comm: swapper/4 Not tainted 4.5.0-rc1-test+ #204
[ 2298.214392] Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v02.05 05/07/2012
[ 2298.223371]  ffffffff81abc680 ffff880119433d68 ffffffff81411c3f 0000000000000000
[ 2298.230904]  ffffffff81abc680 ffff880119433da0 ffffffff810acf66 ffff88011eb16f40
[ 2298.238435]  ffff88001ee16200 ffffffff81fd4a00 00000000000aaaaa 0000000000000001
[ 2298.245958] Call Trace:
[ 2298.248431]  [<ffffffff81411c3f>] dump_stack+0x50/0xb1
[ 2298.253597]  [<ffffffff810acf66>] warn_slowpath_common+0x86/0xc0
[ 2298.259627]  [<ffffffff810ad05a>] warn_slowpath_null+0x1a/0x20
[ 2298.265490]  [<ffffffff810f92a5>] task_dead_dl+0xc5/0xd0
[ 2298.270828]  [<ffffffff810d833f>] finish_task_switch+0x16f/0x310
[ 2298.276871]  [<ffffffff810fa7f3>] ? pick_next_task_dl+0xb3/0x250
[ 2298.282906]  [<ffffffff817f07a3>] __schedule+0x3d3/0x9e0
[ 2298.288252]  [<ffffffff817f1001>] schedule+0x41/0xc0
[ 2298.293242]  [<ffffffff817f12c8>] schedule_preempt_disabled+0x18/0x30
[ 2298.299712]  [<ffffffff810fc974>] cpu_startup_entry+0x74/0x4e0
[ 2298.305573]  [<ffffffff8105d16f>] start_secondary+0x2bf/0x330
[ 2298.311347] ---[ end trace 732d16efabe456f1 ]---

It's the warning you added in __dl_sub_ac().

Things appear to still get all screwed up with cpusets and root
domains, but I can still recover with playing with load_balance.


-- Steve

[toc] | [prev] | [next] | [standalone]

#1331311

On 10/02/16 09:37, Steven Rostedt wrote:
> On Wed, 10 Feb 2016 11:32:58 +0000
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
> > Hi,
> > 
> > I've updated this patch since, with a bit more testing and talking with
> > Luca in private, I realized that the previous version didn't manage
> > switching back and forth from SCHED_DEADLINE correctly. Thanks a lot
> > Luca for your feedback (even if not visible on the list).
> > 
> > I updated the testing branch accordingly and added a test to my tests
> > that stresses switch-in/switch-out.
> > 
> > I don't particularly like the fact that we break the scheduling classes
> > abstraction in __dl_overflow(), so I think a little bit of refactoring
> > is still needed. But that can also happen afterwards, if we fix the
> > problem with root domains.
> > 
> > Best,
> > 
> > - Juri
> > 
> > --->8---  
> > 
> > >From 62f70ca3051672dce209e8355cf5eddc9d825c2a Mon Sep 17 00:00:00 2001  
> > From: Juri Lelli <juri.lelli@arm.com>
> > Date: Sat, 6 Feb 2016 12:41:09 +0000
> > Subject: [PATCH 1/2] sched/deadline: add per rq tracking of admitted bandwidth
> > 
> 
> 
> I applied this patch and patch 2 and hit this:
> 
> [ 2298.134284] ------------[ cut here ]------------
> [ 2298.138933] WARNING: CPU: 4 PID: 0 at /home/rostedt/work/git/linux-trace.git/kernel/sched/sched.h:735 task_dead_dl+0xc5/0xd0()
> [ 2298.150350] Modules linked in: ip6table_filter ip6_tables ebtable_nat ebtables xt_CHECKSUM iptable_mangle bridge stp llc bluetooth lockd grace snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core x86_pkg_temp_thermal snd_seq vhost_net snd_seq_device tun vhost macvtap macvlan coretemp iTCO_wdt snd_pcm hp_wmi rfkill kvm_intel sparse_keymap iTCO_vendor_support snd_timer snd acpi_cpufreq kvm i2c_i801 mei_me mei soundcore lpc_ich mfd_core irqbypass wmi serio_raw uinput i915 i2c_algo_bit e1000e drm_kms_helper crc32_pclmul ptp crc32c_intel drm pps_core i2c_core video sunrpc
> [ 2298.207495] CPU: 4 PID: 0 Comm: swapper/4 Not tainted 4.5.0-rc1-test+ #204
> [ 2298.214392] Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 v02.05 05/07/2012
> [ 2298.223371]  ffffffff81abc680 ffff880119433d68 ffffffff81411c3f 0000000000000000
> [ 2298.230904]  ffffffff81abc680 ffff880119433da0 ffffffff810acf66 ffff88011eb16f40
> [ 2298.238435]  ffff88001ee16200 ffffffff81fd4a00 00000000000aaaaa 0000000000000001
> [ 2298.245958] Call Trace:
> [ 2298.248431]  [<ffffffff81411c3f>] dump_stack+0x50/0xb1
> [ 2298.253597]  [<ffffffff810acf66>] warn_slowpath_common+0x86/0xc0
> [ 2298.259627]  [<ffffffff810ad05a>] warn_slowpath_null+0x1a/0x20
> [ 2298.265490]  [<ffffffff810f92a5>] task_dead_dl+0xc5/0xd0
> [ 2298.270828]  [<ffffffff810d833f>] finish_task_switch+0x16f/0x310
> [ 2298.276871]  [<ffffffff810fa7f3>] ? pick_next_task_dl+0xb3/0x250
> [ 2298.282906]  [<ffffffff817f07a3>] __schedule+0x3d3/0x9e0
> [ 2298.288252]  [<ffffffff817f1001>] schedule+0x41/0xc0
> [ 2298.293242]  [<ffffffff817f12c8>] schedule_preempt_disabled+0x18/0x30
> [ 2298.299712]  [<ffffffff810fc974>] cpu_startup_entry+0x74/0x4e0
> [ 2298.305573]  [<ffffffff8105d16f>] start_secondary+0x2bf/0x330
> [ 2298.311347] ---[ end trace 732d16efabe456f1 ]---
> 
> It's the warning you added in __dl_sub_ac().
> 

OK. There are still holes where we fail to properly update per-rq bw. It
seems (by running you test) that we fail to move the per-rq bw when we
move the root_domain bw due css_set_move_task(). So, the final
task_dead_dl() tries to remove bw from where there isn't.

I'm trying to see how we can close this hole.

Thanks,

- Juri

[toc] | [prev] | [next] | [standalone]

#1331893

On 10/02/16 16:27, Juri Lelli wrote:
> On 10/02/16 09:37, Steven Rostedt wrote:
> > On Wed, 10 Feb 2016 11:32:58 +0000
> > Juri Lelli <juri.lelli@arm.com> wrote:
> > 

[...]

> > 
> > I applied this patch and patch 2 and hit this:
> > 

[...]

> > 
> > It's the warning you added in __dl_sub_ac().
> > 
> 
> OK. There are still holes where we fail to properly update per-rq bw. It
> seems (by running you test) that we fail to move the per-rq bw when we
> move the root_domain bw due css_set_move_task(). So, the final
> task_dead_dl() tries to remove bw from where there isn't.
> 
> I'm trying to see how we can close this hole.
> 

So, just to give an update from yesterday (kind of tricky this one :/).

I think we still have (at least) two problems:

 - select_task_rq_dl, if we select a different target
 - select_task_rq might make use of select_fallback_rq, if cpus_allowed
   changed after the task went to sleep

Second case is what creates the problem here, as we don't update
task_rq(p) and fallback_cpu ac_bw. I was thinking we might do so, maybe
adding fallback_cpu in task_struct, from migrate_task_rq_dl() (it has to
be added yes), but I fear that we should hold both rq locks :/.

Luca, did you already face this problem (if I got it right) and thought
of a way to fix it? I'll go back and stare a bit more at those paths.

Best,

- Juri

[toc] | [prev] | [next] | [standalone]

#1331904

Hi Juri,

On Thu, 11 Feb 2016 12:12:57 +0000
Juri Lelli <juri.lelli@arm.com> wrote:
[...]
> I think we still have (at least) two problems:
> 
>  - select_task_rq_dl, if we select a different target
>  - select_task_rq might make use of select_fallback_rq, if
> cpus_allowed changed after the task went to sleep
> 
> Second case is what creates the problem here, as we don't update
> task_rq(p) and fallback_cpu ac_bw. I was thinking we might do so,
> maybe adding fallback_cpu in task_struct, from migrate_task_rq_dl()
> (it has to be added yes), but I fear that we should hold both rq
> locks :/.
> 
> Luca, did you already face this problem (if I got it right) and
> thought of a way to fix it? I'll go back and stare a bit more at
> those paths.
In my patch I took care of the first case (modifying
select_task_rq_dl() to move the utilization from the "old rq" to the
"new rq"), but I never managed to trigger select_fallback_rq() in my
tests, so I overlooked that case.


				Luca

[toc] | [prev] | [next] | [standalone]

#1331912

On 11/02/16 13:22, Luca Abeni wrote:
> Hi Juri,
> 
> On Thu, 11 Feb 2016 12:12:57 +0000
> Juri Lelli <juri.lelli@arm.com> wrote:
> [...]
> > I think we still have (at least) two problems:
> > 
> >  - select_task_rq_dl, if we select a different target
> >  - select_task_rq might make use of select_fallback_rq, if
> > cpus_allowed changed after the task went to sleep
> > 
> > Second case is what creates the problem here, as we don't update
> > task_rq(p) and fallback_cpu ac_bw. I was thinking we might do so,
> > maybe adding fallback_cpu in task_struct, from migrate_task_rq_dl()
> > (it has to be added yes), but I fear that we should hold both rq
> > locks :/.
> > 
> > Luca, did you already face this problem (if I got it right) and
> > thought of a way to fix it? I'll go back and stare a bit more at
> > those paths.
> In my patch I took care of the first case (modifying
> select_task_rq_dl() to move the utilization from the "old rq" to the
> "new rq"), but I never managed to trigger select_fallback_rq() in my
> tests, so I overlooked that case.
> 

Right, I was thinking to do the same. And you did that after grabbing
both locks, right?

[toc] | [prev] | [next] | [standalone]

#1331921

On 11/02/16 13:40, Luca Abeni wrote:
> On Thu, 11 Feb 2016 12:27:54 +0000
> Juri Lelli <juri.lelli@arm.com> wrote:
> 
> > On 11/02/16 13:22, Luca Abeni wrote:
> > > Hi Juri,
> > > 
> > > On Thu, 11 Feb 2016 12:12:57 +0000
> > > Juri Lelli <juri.lelli@arm.com> wrote:
> > > [...]
> > > > I think we still have (at least) two problems:
> > > > 
> > > >  - select_task_rq_dl, if we select a different target
> > > >  - select_task_rq might make use of select_fallback_rq, if
> > > > cpus_allowed changed after the task went to sleep
> > > > 
> > > > Second case is what creates the problem here, as we don't update
> > > > task_rq(p) and fallback_cpu ac_bw. I was thinking we might do so,
> > > > maybe adding fallback_cpu in task_struct, from
> > > > migrate_task_rq_dl() (it has to be added yes), but I fear that we
> > > > should hold both rq locks :/.
> > > > 
> > > > Luca, did you already face this problem (if I got it right) and
> > > > thought of a way to fix it? I'll go back and stare a bit more at
> > > > those paths.
> > > In my patch I took care of the first case (modifying
> > > select_task_rq_dl() to move the utilization from the "old rq" to the
> > > "new rq"), but I never managed to trigger select_fallback_rq() in my
> > > tests, so I overlooked that case.
> > > 
> > 
> > Right, I was thinking to do the same. And you did that after grabbing
> > both locks, right?
> 
> Not sure if I did everything correctly, but my code in
> select_task_rq_dl() currently looks like this (you can obviously
> ignore the "migrate_active" and "*_running_bw()" parts, and focus on
> the "*_rq_bw()" stuff):
> [...]
>         if (rq != cpu_rq(cpu)) {
>                 int migrate_active;
> 
>                 raw_spin_lock(&rq->lock);
>                 migrate_active = hrtimer_active(&p->dl.inactive_timer);
>                 if (migrate_active) {
>                         hrtimer_try_to_cancel(&p->dl.inactive_timer);
>                         sub_running_bw(&p->dl, &rq->dl);
>                 }
>                 sub_rq_bw(&p->dl, &rq->dl);
>                 raw_spin_unlock(&rq->lock);
>                 rq = cpu_rq(cpu);

Can't something happen here? My problem is that I use per-rq bw tracking
to save/restore root_domain state. So, I fear that a root_domain update
can happen while we are in the middle of moving bw from one cpu to
another.

>                 raw_spin_lock(&rq->lock);
>                 add_rq_bw(&p->dl, &rq->dl);
>                 if (migrate_active)
>                         add_running_bw(&p->dl, &rq->dl);
>                 raw_spin_unlock(&rq->lock);
>         }
> [...]
> 
> lockdep is not screaming, and I am not able to trigger any race
> condition or strange behaviour (I am currently at more than 24h of
> continuous stress-testing, but maybe my testcase is not so good in
> finding races here :)
> 

Thanks for sharing what you have!

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web