Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Code cleanup in linux/sched/ #597

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Code cleanup in linux/sched/ #597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions kernel/sched/core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 1 addition & 4 deletions kernel/sched/cpufreq.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,7 @@ void cpufreq_add_update_util_hook(int cpu, struct update_util_data *data,
void (*func)(struct update_util_data *data, u64 time,
unsigned int flags))
{
if (WARN_ON(!data || !func))
return;

if (WARN_ON(per_cpu(cpufreq_update_util_data, cpu)))
if (WARN_ON(!data || !func || per_cpu(cpufreq_update_util_data, cpu)))
return;

data->func = func;
Expand Down
22 changes: 8 additions & 14 deletions kernel/sched/cpupri.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it clearer having just one return statement instead of several exit points?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a tiny optimization that should reduce the amount of instructions used in that method imo (if the compiler doesn't optimize that already), instead of assigning cpupri variable a value and then return it, it will just return the value instead.

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;
}

/**
Expand Down Expand Up @@ -95,10 +91,8 @@ int cpupri_find(struct cpupri *cp, struct task_struct *p,
smp_rmb();

/* Need to do the rmb for every iteration */
if (skip)
continue;

if (cpumask_any_and(&p->cpus_allowed, vec->mask) >= nr_cpu_ids)
if (skip || cpumask_any_and(&p->cpus_allowed, vec->mask)
>= nr_cpu_ids)
continue;

if (lowest_mask) {
Expand Down Expand Up @@ -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;
}
Expand Down
31 changes: 12 additions & 19 deletions kernel/sched/rt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was actually checking if tg->rt_rq and tg->rt_se are NULL or not, not the elements in the array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how I managed to overlook that. I've received feedback from Peter Zijlstra describing this patch as "horrible" and I agree with that assessment. I'm waiting for a response from Peter regarding whether he thinks any part of the patch is salvageable or if the whole thing is better off being thrown out.

* or tg->rt_se[i] are NULL, since kfree(NULL)
* simply performs no operation
*/
kfree(tg->rt_rq[i]);
kfree(tg->rt_se[i]);
}

kfree(tg->rt_rq);
Expand Down Expand Up @@ -1015,10 +1017,7 @@ enqueue_top_rt_rq(struct rt_rq *rt_rq)

BUG_ON(&rq->rt != rt_rq);

if (rt_rq->rt_queued)
return;

if (rt_rq_throttled(rt_rq))
if (rt_rq->rt_queued || rt_rq_throttled(rt_rq))
return;

if (rt_rq->rt_nr_running) {
Expand Down Expand Up @@ -1211,10 +1210,7 @@ void dec_rt_tasks(struct sched_rt_entity *rt_se, struct rt_rq *rt_rq)
*/
static inline bool move_entity(unsigned int flags)
{
if ((flags & (DEQUEUE_SAVE | DEQUEUE_MOVE)) == DEQUEUE_SAVE)
return false;

return true;
return !((flags & (DEQUEUE_SAVE | DEQUEUE_MOVE)) == DEQUEUE_SAVE)
}

static void __delist_rt_entity(struct sched_rt_entity *rt_se, struct rt_prio_array *array)
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -2518,12 +2513,10 @@ static int tg_set_rt_bandwidth(struct task_group *tg,
/*
* Disallowing the root group RT runtime is BAD, it would disallow the
* kernel creating (and or operating) RT threads.
*
* No period doesn't make any sense.
*/
if (tg == &root_task_group && rt_runtime == 0)
return -EINVAL;

/* No period doesn't make any sense. */
if (rt_period == 0)
if ((tg == &root_task_group && !rt_runtime) || !rt_period)
return -EINVAL;

mutex_lock(&rt_constraints_mutex);
Expand Down