Re: [PATCH] sched/deadline: Use bools for the state flags

[Luca Abeni]

Hello,

On Tue, 21 Nov 2017 11:44:05 +0100 (CET)
Jiri Kosina <jikos@xxxxxxxxxx> wrote:

> From: Jiri Kosina <jkosina@xxxxxxx>
> 
> Commit
> 
> 	799ba82de01e ("sched/deadline: Use C bitfields for the state flags")
> 
> converted state flags into one-bit signed int. Signed one-bit type can be 
> either 0 or -1, which is going to cause a problem once 1 is assigned to it 
> and then the value later tested against 1.

I think a different fix has just been committed to tip:
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?id=aa5222e92f8000ed3c1c38dddf11c83222aadfb3


				Luca
> 
> The current code is okay, as all the checks are (non-)zero check, but I 
> believe that we'd rather be safe than sorry here; remove the fragility by 
> converting the state flags to bool.
> 
> This also silences annoying sparse complaints about this very issue when 
> compiling any code that includes sched.h.
> 
> Fixes: 799ba82de01e ("sched/deadline: Use C bitfields for the state flags")
> Cc: luca abeni <luca.abeni@xxxxxxxxxxxxxxx>
> Cc: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Cc: Daniel Bristot de Oliveira <bristot@xxxxxxxxxx>
> Cc: Juri Lelli <juri.lelli@xxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> Cc: Mike Galbraith <efault@xxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Signed-off-by: Jiri Kosina <jkosina@xxxxxxx>
> ---
>  include/linux/sched.h   |  8 ++++----
>  kernel/sched/core.c     |  8 ++++----
>  kernel/sched/deadline.c | 32 ++++++++++++++++----------------
>  3 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index a5dc7c98b0a2..b19fa1b96726 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -473,10 +473,10 @@ struct sched_dl_entity {
>  	 * conditions between the inactive timer handler and the wakeup
>  	 * code.
>  	 */
> -	int				dl_throttled      : 1;
> -	int				dl_boosted        : 1;
> -	int				dl_yielded        : 1;
> -	int				dl_non_contending : 1;
> +	bool				dl_throttled;
> +	bool				dl_boosted;
> +	bool				dl_yielded;
> +	bool				dl_non_contending;
>  
>  	/*
>  	 * Bandwidth enforcement timer. Each -deadline task has its
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 75554f366fd3..f9bbf0f55e0e 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3738,20 +3738,20 @@ void rt_mutex_setprio(struct task_struct *p, struct task_struct *pi_task)
>  	if (dl_prio(prio)) {
>  		if (!dl_prio(p->normal_prio) ||
>  		    (pi_task && dl_entity_preempt(&pi_task->dl, &p->dl))) {
> -			p->dl.dl_boosted = 1;
> +			p->dl.dl_boosted = true;
>  			queue_flag |= ENQUEUE_REPLENISH;
>  		} else
> -			p->dl.dl_boosted = 0;
> +			p->dl.dl_boosted = false;
>  		p->sched_class = &dl_sched_class;
>  	} else if (rt_prio(prio)) {
>  		if (dl_prio(oldprio))
> -			p->dl.dl_boosted = 0;
> +			p->dl.dl_boosted = false;
>  		if (oldprio < prio)
>  			queue_flag |= ENQUEUE_HEAD;
>  		p->sched_class = &rt_sched_class;
>  	} else {
>  		if (dl_prio(oldprio))
> -			p->dl.dl_boosted = 0;
> +			p->dl.dl_boosted = false;
>  		if (rt_prio(oldprio))
>  			p->rt.timeout = 0;
>  		p->sched_class = &fair_sched_class;
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 2473736c7616..78cb00ae8b2f 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -133,7 +133,7 @@ void dl_change_utilization(struct task_struct *p, u64 new_bw)
>  	rq = task_rq(p);
>  	if (p->dl.dl_non_contending) {
>  		sub_running_bw(p->dl.dl_bw, &rq->dl);
> -		p->dl.dl_non_contending = 0;
> +		p->dl.dl_non_contending = false;
>  		/*
>  		 * If the timer handler is currently running and the
>  		 * timer cannot be cancelled, inactive_task_timer()
> @@ -251,7 +251,7 @@ static void task_non_contending(struct task_struct *p)
>  		return;
>  	}
>  
> -	dl_se->dl_non_contending = 1;
> +	dl_se->dl_non_contending = true;
>  	get_task_struct(p);
>  	hrtimer_start(timer, ns_to_ktime(zerolag_time), HRTIMER_MODE_REL);
>  }
> @@ -271,7 +271,7 @@ static void task_contending(struct sched_dl_entity *dl_se, int flags)
>  		add_rq_bw(dl_se->dl_bw, dl_rq);
>  
>  	if (dl_se->dl_non_contending) {
> -		dl_se->dl_non_contending = 0;
> +		dl_se->dl_non_contending = false;
>  		/*
>  		 * If the timer handler is currently running and the
>  		 * timer cannot be cancelled, inactive_task_timer()
> @@ -676,9 +676,9 @@ static void replenish_dl_entity(struct sched_dl_entity *dl_se,
>  	}
>  
>  	if (dl_se->dl_yielded)
> -		dl_se->dl_yielded = 0;
> +		dl_se->dl_yielded = false;
>  	if (dl_se->dl_throttled)
> -		dl_se->dl_throttled = 0;
> +		dl_se->dl_throttled = false;
>  }
>  
>  /*
> @@ -1051,7 +1051,7 @@ static inline void dl_check_constrained_dl(struct sched_dl_entity *dl_se)
>  	    dl_time_before(rq_clock(rq), dl_next_period(dl_se))) {
>  		if (unlikely(dl_se->dl_boosted || !start_dl_timer(p)))
>  			return;
> -		dl_se->dl_throttled = 1;
> +		dl_se->dl_throttled = true;
>  		if (dl_se->runtime > 0)
>  			dl_se->runtime = 0;
>  	}
> @@ -1154,7 +1154,7 @@ static void update_curr_dl(struct rq *rq)
>  
>  throttle:
>  	if (dl_runtime_exceeded(dl_se) || dl_se->dl_yielded) {
> -		dl_se->dl_throttled = 1;
> +		dl_se->dl_throttled = true;
>  		__dequeue_task_dl(rq, curr, 0);
>  		if (unlikely(dl_se->dl_boosted || !start_dl_timer(curr)))
>  			enqueue_task_dl(rq, curr, ENQUEUE_REPLENISH);
> @@ -1206,7 +1206,7 @@ static enum hrtimer_restart inactive_task_timer(struct hrtimer *timer)
>  		if (p->state == TASK_DEAD && dl_se->dl_non_contending) {
>  			sub_running_bw(p->dl.dl_bw, dl_rq_of_se(&p->dl));
>  			sub_rq_bw(p->dl.dl_bw, dl_rq_of_se(&p->dl));
> -			dl_se->dl_non_contending = 0;
> +			dl_se->dl_non_contending = false;
>  		}
>  
>  		raw_spin_lock(&dl_b->lock);
> @@ -1216,14 +1216,14 @@ static enum hrtimer_restart inactive_task_timer(struct hrtimer *timer)
>  
>  		goto unlock;
>  	}
> -	if (dl_se->dl_non_contending == 0)
> +	if (!dl_se->dl_non_contending)
>  		goto unlock;
>  
>  	sched_clock_tick();
>  	update_rq_clock(rq);
>  
>  	sub_running_bw(dl_se->dl_bw, &rq->dl);
> -	dl_se->dl_non_contending = 0;
> +	dl_se->dl_non_contending = false;
>  unlock:
>  	task_rq_unlock(rq, p, &rf);
>  	put_task_struct(p);
> @@ -1492,7 +1492,7 @@ static void yield_task_dl(struct rq *rq)
>  	 * it and the bandwidth timer will wake it up and will give it
>  	 * new scheduling parameters (thanks to dl_yielded=1).
>  	 */
> -	rq->curr->dl.dl_yielded = 1;
> +	rq->curr->dl.dl_yielded = true;
>  
>  	update_rq_clock(rq);
>  	update_curr_dl(rq);
> @@ -1565,7 +1565,7 @@ static void migrate_task_rq_dl(struct task_struct *p)
>  	raw_spin_lock(&rq->lock);
>  	if (p->dl.dl_non_contending) {
>  		sub_running_bw(p->dl.dl_bw, &rq->dl);
> -		p->dl.dl_non_contending = 0;
> +		p->dl.dl_non_contending = false;
>  		/*
>  		 * If the timer handler is currently running and the
>  		 * timer cannot be cancelled, inactive_task_timer()
> @@ -2232,7 +2232,7 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
>  	 * while SCHED_OTHER in the meanwhile.
>  	 */
>  	if (p->dl.dl_non_contending)
> -		p->dl.dl_non_contending = 0;
> +		p->dl.dl_non_contending = false;
>  
>  	/*
>  	 * Since this might be the only -deadline task on the rq,
> @@ -2563,9 +2563,9 @@ void __dl_clear_params(struct task_struct *p)
>  	dl_se->dl_bw = 0;
>  	dl_se->dl_density = 0;
>  
> -	dl_se->dl_throttled = 0;
> -	dl_se->dl_yielded = 0;
> -	dl_se->dl_non_contending = 0;
> +	dl_se->dl_throttled = false;
> +	dl_se->dl_yielded = false;
> +	dl_se->dl_non_contending = false;
>  }
>  
>  bool dl_param_changed(struct task_struct *p, const struct sched_attr *attr)
>