Hi,
here are some fixes, cleanups and improvements for SCHED_DEADLINE.
Patch 0001 just removes a duplicate declaration from sched/sched.h.
Patch 0002 fixes a bug I introduced some time ago, when removing
the dl_new field from sched_dl_entity in
72f9f3fdc928dc3ecd223e801b32d930b662b6ed.
This is the same patch I already sent some time ago, because at the
end of the discussion it was not clear to me if some changes are needed
or not.
Patch 0003 renames the confusing __dl_clear() function, as suggested
by Peter (actually, this is Peter's patch... I hope I did the SoB thing
correctly)
Patch 0004 saves reduces the size of sched_dl_entity by using bitfields
instead of whole integers for boolean values.
Peter Zijlstra (1):
sched/deadline: rename __dl_clear() to __dl_sub()
luca abeni (3):
sched/sched.h: remove duplicate prototype of __dl_clear_params()
sched/deadline: fix switching to -deadline
sched/deadline: use C bitfields for the state flags
include/linux/sched.h | 8 ++++----
kernel/sched/deadline.c | 21 +++++++++------------
kernel/sched/sched.h | 3 +--
3 files changed, 14 insertions(+), 18 deletions(-)
--
2.7.4
Signed-off-by: luca abeni <[email protected]>
---
kernel/sched/sched.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 1043c8b..0b93e4b 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -255,7 +255,6 @@ extern int sched_dl_overflow(struct task_struct *p, int policy,
extern void __setparam_dl(struct task_struct *p, const struct sched_attr *attr);
extern void __getparam_dl(struct task_struct *p, struct sched_attr *attr);
extern bool __checkparam_dl(const struct sched_attr *attr);
-extern void __dl_clear_params(struct task_struct *p);
extern bool dl_param_changed(struct task_struct *p, const struct sched_attr *attr);
extern int dl_task_can_attach(struct task_struct *p,
const struct cpumask *cs_cpus_allowed);
--
2.7.4
Fix a bug introduced in 72f9f3fdc928dc3ecd223e801b32d930b662b6ed:
after that commit, when switching to -deadline if the scheduling
deadline of a task is in the past then switched_to_dl() calls
setup_new_entity() to properly initialize the scheduling deadline
and runtime.
The problem is that the task is enqueued _before_ having its parameters
initialized by setup_new_entity(), and this can cause problems.
For example, a task with its out-of-date deadline in the past will
potentially be enqueued as the highest priority one; however, its
adjusted deadline may not be the earliest one.
This patch fixes the problem by initializing the task's parameters before
enqueuing it.
Signed-off-by: luca abeni <[email protected]>
Reviewed-by: Daniel Bristot de Oliveira <[email protected]>
---
kernel/sched/deadline.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index d05bd94..d7f9e86 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1376,6 +1376,10 @@ enqueue_dl_entity(struct sched_dl_entity *dl_se,
update_dl_entity(dl_se, pi_se);
} else if (flags & ENQUEUE_REPLENISH) {
replenish_dl_entity(dl_se, pi_se);
+ } else if ((flags & ENQUEUE_RESTORE) &&
+ dl_time_before(dl_se->deadline,
+ rq_clock(rq_of_dl_rq(dl_rq_of_se(dl_se))))) {
+ setup_new_dl_entity(dl_se);
}
__enqueue_dl_entity(dl_se);
@@ -2267,13 +2271,6 @@ static void switched_to_dl(struct rq *rq, struct task_struct *p)
return;
}
- /*
- * If p is boosted we already updated its params in
- * rt_mutex_setprio()->enqueue_task(..., ENQUEUE_REPLENISH),
- * p's deadline being now already after rq_clock(rq).
- */
- if (dl_time_before(p->dl.deadline, rq_clock(rq)))
- setup_new_dl_entity(&p->dl);
if (rq->curr != p) {
#ifdef CONFIG_SMP
--
2.7.4
Ask the compiler to use a single bit for storing true / false values,
instead of wasting the size of a whole int value.
Tested with gcc 5.4.0 on x86_64, and the compiler produces the expected
Assembly (similar to the Assembly code generated when explicitly accessing
the bits with bitmasks, "&" and "|").
Signed-off-by: luca abeni <[email protected]>
---
include/linux/sched.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 68b3833..e03cc69 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -472,10 +472,10 @@ struct sched_dl_entity {
* conditions between the inactive timer handler and the wakeup
* code.
*/
- int dl_throttled;
- int dl_boosted;
- int dl_yielded;
- int dl_non_contending;
+ int dl_throttled : 1;
+ int dl_boosted : 1;
+ int dl_yielded : 1;
+ int dl_non_contending : 1;
/*
* Bandwidth enforcement timer. Each -deadline task has its
--
2.7.4
From: Peter Zijlstra <[email protected]>
__dl_sub() is more meaningful as a name, and is more consistent
with the naming of the dual function (__dl_add()).
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: luca abeni <[email protected]>
---
kernel/sched/deadline.c | 10 +++++-----
kernel/sched/sched.h | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index d7f9e86..bab11dd 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -242,7 +242,7 @@ static void task_non_contending(struct task_struct *p)
if (p->state == TASK_DEAD)
sub_rq_bw(p->dl.dl_bw, &rq->dl);
raw_spin_lock(&dl_b->lock);
- __dl_clear(dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
+ __dl_sub(dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
__dl_clear_params(p);
raw_spin_unlock(&dl_b->lock);
}
@@ -1211,7 +1211,7 @@ static enum hrtimer_restart inactive_task_timer(struct hrtimer *timer)
}
raw_spin_lock(&dl_b->lock);
- __dl_clear(dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
+ __dl_sub(dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
raw_spin_unlock(&dl_b->lock);
__dl_clear_params(p);
@@ -2182,7 +2182,7 @@ static void set_cpus_allowed_dl(struct task_struct *p,
* until we complete the update.
*/
raw_spin_lock(&src_dl_b->lock);
- __dl_clear(src_dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
+ __dl_sub(src_dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
raw_spin_unlock(&src_dl_b->lock);
}
@@ -2460,7 +2460,7 @@ int sched_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)) {
if (hrtimer_active(&p->dl.inactive_timer))
- __dl_clear(dl_b, p->dl.dl_bw, cpus);
+ __dl_sub(dl_b, p->dl.dl_bw, cpus);
__dl_add(dl_b, new_bw, cpus);
err = 0;
} else if (dl_policy(policy) && task_has_dl_policy(p) &&
@@ -2472,7 +2472,7 @@ int sched_dl_overflow(struct task_struct *p, int policy,
* But this would require to set the task's "inactive
* timer" when the task is not inactive.
*/
- __dl_clear(dl_b, p->dl.dl_bw, cpus);
+ __dl_sub(dl_b, p->dl.dl_bw, cpus);
__dl_add(dl_b, new_bw, cpus);
dl_change_utilization(p, new_bw);
err = 0;
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 0b93e4b..061ec3b 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -226,7 +226,7 @@ struct dl_bw {
static inline void __dl_update(struct dl_bw *dl_b, s64 bw);
static inline
-void __dl_clear(struct dl_bw *dl_b, u64 tsk_bw, int cpus)
+void __dl_sub(struct dl_bw *dl_b, u64 tsk_bw, int cpus)
{
dl_b->total_bw -= tsk_bw;
__dl_update(dl_b, (s32)tsk_bw / cpus);
--
2.7.4
On 09/07/2017 12:09 PM, luca abeni wrote:
> Ask the compiler to use a single bit for storing true / false values,
> instead of wasting the size of a whole int value.
> Tested with gcc 5.4.0 on x86_64, and the compiler produces the expected
> Assembly (similar to the Assembly code generated when explicitly accessing
> the bits with bitmasks, "&" and "|").
>
> Signed-off-by: luca abeni <[email protected]>
Reviewed-by: Daniel Bristot de Oliveira <[email protected]>
-- Daniel
> ---
> include/linux/sched.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 68b3833..e03cc69 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -472,10 +472,10 @@ struct sched_dl_entity {
> * conditions between the inactive timer handler and the wakeup
> * code.
> */
> - int dl_throttled;
> - int dl_boosted;
> - int dl_yielded;
> - int dl_non_contending;
> + int dl_throttled : 1;
> + int dl_boosted : 1;
> + int dl_yielded : 1;
> + int dl_non_contending : 1;
>
> /*
> * Bandwidth enforcement timer. Each -deadline task has its
>
On 09/07/2017 12:09 PM, luca abeni wrote:
> From: Peter Zijlstra <[email protected]>
>
> __dl_sub() is more meaningful as a name, and is more consistent
> with the naming of the dual function (__dl_add()).
>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Signed-off-by: luca abeni <[email protected]>
Reviewed-by: Daniel Bristot de Oliveira <[email protected]>
-- Daniel
> ---
> kernel/sched/deadline.c | 10 +++++-----
> kernel/sched/sched.h | 2 +-
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index d7f9e86..bab11dd 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -242,7 +242,7 @@ static void task_non_contending(struct task_struct *p)
> if (p->state == TASK_DEAD)
> sub_rq_bw(p->dl.dl_bw, &rq->dl);
> raw_spin_lock(&dl_b->lock);
> - __dl_clear(dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
> + __dl_sub(dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
> __dl_clear_params(p);
> raw_spin_unlock(&dl_b->lock);
> }
> @@ -1211,7 +1211,7 @@ static enum hrtimer_restart inactive_task_timer(struct hrtimer *timer)
> }
>
> raw_spin_lock(&dl_b->lock);
> - __dl_clear(dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
> + __dl_sub(dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
> raw_spin_unlock(&dl_b->lock);
> __dl_clear_params(p);
>
> @@ -2182,7 +2182,7 @@ static void set_cpus_allowed_dl(struct task_struct *p,
> * until we complete the update.
> */
> raw_spin_lock(&src_dl_b->lock);
> - __dl_clear(src_dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
> + __dl_sub(src_dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
> raw_spin_unlock(&src_dl_b->lock);
> }
>
> @@ -2460,7 +2460,7 @@ int sched_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)) {
> if (hrtimer_active(&p->dl.inactive_timer))
> - __dl_clear(dl_b, p->dl.dl_bw, cpus);
> + __dl_sub(dl_b, p->dl.dl_bw, cpus);
> __dl_add(dl_b, new_bw, cpus);
> err = 0;
> } else if (dl_policy(policy) && task_has_dl_policy(p) &&
> @@ -2472,7 +2472,7 @@ int sched_dl_overflow(struct task_struct *p, int policy,
> * But this would require to set the task's "inactive
> * timer" when the task is not inactive.
> */
> - __dl_clear(dl_b, p->dl.dl_bw, cpus);
> + __dl_sub(dl_b, p->dl.dl_bw, cpus);
> __dl_add(dl_b, new_bw, cpus);
> dl_change_utilization(p, new_bw);
> err = 0;
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 0b93e4b..061ec3b 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -226,7 +226,7 @@ struct dl_bw {
> static inline void __dl_update(struct dl_bw *dl_b, s64 bw);
>
> static inline
> -void __dl_clear(struct dl_bw *dl_b, u64 tsk_bw, int cpus)
> +void __dl_sub(struct dl_bw *dl_b, u64 tsk_bw, int cpus)
> {
> dl_b->total_bw -= tsk_bw;
> __dl_update(dl_b, (s32)tsk_bw / cpus);
>
Hi,
I wonder if a commit log, even an one line comment, explaining that this
function was already declared in the file could help to understand this
patch...
But, the patch is so trivial... that... not sure it is worth... anyway...
On 09/07/2017 12:09 PM, luca abeni wrote:
> Signed-off-by: luca abeni <[email protected]>
Reviewed-by: Daniel Bristot de Oliveira <[email protected]>
-- Daniel
> ---
> kernel/sched/sched.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 1043c8b..0b93e4b 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -255,7 +255,6 @@ extern int sched_dl_overflow(struct task_struct *p, int policy,
> extern void __setparam_dl(struct task_struct *p, const struct sched_attr *attr);
> extern void __getparam_dl(struct task_struct *p, struct sched_attr *attr);
> extern bool __checkparam_dl(const struct sched_attr *attr);
> -extern void __dl_clear_params(struct task_struct *p);
> extern bool dl_param_changed(struct task_struct *p, const struct sched_attr *attr);
> extern int dl_task_can_attach(struct task_struct *p,
> const struct cpumask *cs_cpus_allowed);
>