Re: [PATCH] scheduler: conditional statement cleanup

[Peter Zijlstra]

On Fri, Sep 21, 2018 at 03:22:03PM -0500, PierceGriffiths wrote:
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 625bc9897f62..443a1f235cfd 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -617,12 +617,8 @@ bool sched_can_stop_tick(struct rq *rq)
>  	 * If there are more than one RR tasks, we need the tick to effect the
>  	 * actual RR behaviour.
>  	 */
> -	if (rq->rt.rr_nr_running) {
> -		if (rq->rt.rr_nr_running == 1)
> -			return true;
> -		else
> -			return false;
> -	}
> +	if (rq->rt.rr_nr_running)
> +		return rq->rt.rr_nr_running == 1;
>  
>  	/*
>  	 * If there's no RR tasks, but FIFO tasks, we can skip the tick, no

> diff --git a/kernel/sched/cpupri.c b/kernel/sched/cpupri.c
> index daaadf939ccb..152c133e8247 100644
> --- a/kernel/sched/cpupri.c
> +++ b/kernel/sched/cpupri.c
> @@ -29,20 +29,16 @@
>  #include "sched.h"
>  
>  /* Convert between a 140 based task->prio, and our 102 based cpupri */
> -static int convert_prio(int prio)
> +static int convert_prio(const int prio)
>  {
> -	int cpupri;
> -
>  	if (prio == CPUPRI_INVALID)
> -		cpupri = CPUPRI_INVALID;
> +		return CPUPRI_INVALID;
>  	else if (prio == MAX_PRIO)
> -		cpupri = CPUPRI_IDLE;
> +		return CPUPRI_IDLE;
>  	else if (prio >= MAX_RT_PRIO)
> -		cpupri = CPUPRI_NORMAL;
> +		return CPUPRI_NORMAL;
>  	else
> -		cpupri = MAX_RT_PRIO - prio + 1;
> -
> -	return cpupri;
> +		return MAX_RT_PRIO - prio + 1;

Code improves if you leave out the last else.

>  }
>  
>  /**

> @@ -222,7 +216,7 @@ int cpupri_init(struct cpupri *cp)
>  	return 0;
>  
>  cleanup:
> -	for (i--; i >= 0; i--)
> +	while (--i >= 0)
>  		free_cpumask_var(cp->pri_to_cpu[i].mask);
>  	return -ENOMEM;
>  }

> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 2e2955a8cf8f..acf1b94669ad 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -142,10 +142,12 @@ void free_rt_sched_group(struct task_group *tg)
>  		destroy_rt_bandwidth(&tg->rt_bandwidth);
>  
>  	for_each_possible_cpu(i) {
> -		if (tg->rt_rq)
> -			kfree(tg->rt_rq[i]);
> -		if (tg->rt_se)
> -			kfree(tg->rt_se[i]);
> +		/* Don't need to check if tg->rt_rq[i]
> +		 * or tg->rt_se[i] are NULL, since kfree(NULL)
> +		 * simply performs no operation
> +		 */

Don't bother with the comment tho (but if you do, know this is the wrong
comment style).

> +		kfree(tg->rt_rq[i]);
> +		kfree(tg->rt_se[i]);
>  	}
>  
>  	kfree(tg->rt_rq);

> @@ -1393,7 +1389,7 @@ select_task_rq_rt(struct task_struct *p, int cpu, int sd_flag, int flags)
>  
>  	/* For anything but wake ups, just return the task_cpu */
>  	if (sd_flag != SD_BALANCE_WAKE && sd_flag != SD_BALANCE_FORK)
> -		goto out;
> +		return cpu;
>  
>  	rq = cpu_rq(cpu);
>  
> @@ -1437,7 +1433,6 @@ select_task_rq_rt(struct task_struct *p, int cpu, int sd_flag, int flags)
>  	}
>  	rcu_read_unlock();
>  
> -out:
>  	return cpu;
>  }
>  

These changes are OK with minor edits, the rest just makes the code
harder to read.