Hi,
you should add "sched/deadline:" to the subject.
On 19/07/16 11:44, Tommaso Cucinotta wrote:
I'd repeat what the subject says, so that you can refer that from the
changelog with "These".
> These 2 exercise independent code paths and need different arguments.
> Now you call
It's more "after this change" that now, IMHO.
> cpudl_clear(cp, cpu)
> cpudl_set(cp, cpu, dl)
> instead of
> cpudl_set(cp, cpu, 0 /* dl */, 0 /* is_valid */)
> cpudl_set(cp, cpu, dl, 1 /* is_valid */)
>
> Cc: Peter Zijlstra <pet...@infradead.org>
> Cc: Juri Lelli <juri.le...@arm.com>
> Cc: Luca Abeni <luca.ab...@unitn.it>
> Reviewed-by: Luca Abeni <luca.ab...@unitn.it>
> Signed-off-by: Tommaso Cucinotta <tommaso.cucino...@sssup.it>
> ---
> kernel/sched/cpudeadline.c | 71
> +++++++++++++++++++++++++++++-----------------
> kernel/sched/cpudeadline.h | 3 +-
> kernel/sched/deadline.c | 10 +++----
> 3 files changed, 52 insertions(+), 32 deletions(-)
>
> diff --git a/kernel/sched/cpudeadline.c b/kernel/sched/cpudeadline.c
> index 60f933a..0f276bf 100644
> --- a/kernel/sched/cpudeadline.c
> +++ b/kernel/sched/cpudeadline.c
> @@ -147,16 +147,15 @@ out:
> }
>
> /*
> - * cpudl_set - update the cpudl max-heap
> + * cpudl_clear - remove a cpu from the cpudl max-heap
> * @cp: the cpudl max-heap context
> * @cpu: the target cpu
> - * @dl: the new earliest deadline for this cpu
> *
> * Notes: assumes cpu_rq(cpu)->lock is locked
We should probably add (in a separate patch) a lockdep_assert for this.
> *
> * Returns: (void)
> */
> -void cpudl_set(struct cpudl *cp, int cpu, u64 dl, int is_valid)
> +void cpudl_clear(struct cpudl *cp, int cpu)
> {
> int old_idx, new_cpu;
> unsigned long flags;
> @@ -164,17 +163,16 @@ void cpudl_set(struct cpudl *cp, int cpu, u64 dl, int
> is_valid)
> WARN_ON(!cpu_present(cpu));
>
> raw_spin_lock_irqsave(&cp->lock, flags);
> +
> old_idx = cp->elements[cpu].idx;
> - if (!is_valid) {
> + if (old_idx == IDX_INVALID) {
> + /*
> + * Nothing to remove if old_idx was invalid.
> + * This could happen if a rq_offline_dl is
> + * called for a CPU without -dl tasks running.
> + */
> + } else {
> /* remove item */
> - if (old_idx == IDX_INVALID) {
> - /*
> - * Nothing to remove if old_idx was invalid.
> - * This could happen if a rq_offline_dl is
> - * called for a CPU without -dl tasks running.
> - */
> - goto out;
> - }
> cp->size--;
> cp->elements[cpu].idx = IDX_INVALID;
> if (old_idx != cp->size) {
> @@ -184,24 +182,45 @@ void cpudl_set(struct cpudl *cp, int cpu, u64 dl, int
> is_valid)
> cp->elements[new_cpu].idx = old_idx;
> cpudl_heapify(cp, old_idx);
> }
> -
> cpumask_set_cpu(cpu, cp->free_cpus);
> + }
> +
> + raw_spin_unlock_irqrestore(&cp->lock, flags);
> +}
> +
> +/*
> + * cpudl_set - update the cpudl max-heap
> + * @cp: the cpudl max-heap context
> + * @cpu: the target cpu
> + * @dl: the new earliest deadline for this cpu
> + *
> + * Notes: assumes cpu_rq(cpu)->lock is locked
> + *
> + * Returns: (void)
> + */
> +void cpudl_set(struct cpudl *cp, int cpu, u64 dl)
> +{
> + int old_idx;
> + unsigned long flags;
> +
> + WARN_ON(!cpu_present(cpu));
> +
> + raw_spin_lock_irqsave(&cp->lock, flags);
> +
> + old_idx = cp->elements[cpu].idx;
> + if (old_idx == IDX_INVALID) {
> + int sz1 = cp->size++;
You also change the temp variable name. I think you might want to fix
that in one of the previous patches once for all.
> + cp->elements[sz1].dl = dl;
> + cp->elements[sz1].cpu = cpu;
> + cp->elements[cpu].idx = sz1;
> + cpudl_heapify_up(cp, sz1);
> +
> + cpumask_clear_cpu(cpu, cp->free_cpus);
> } else {
> - if (old_idx == IDX_INVALID) {
> - int size1 = cp->size++;
> - cp->elements[size1].dl = dl;
> - cp->elements[size1].cpu = cpu;
> - cp->elements[cpu].idx = size1;
> - cpudl_heapify_up(cp, size1);
> -
> - cpumask_clear_cpu(cpu, cp->free_cpus);
> - } else {
> - cp->elements[old_idx].dl = dl;
> - cpudl_heapify(cp, old_idx);
> - }
> + cp->elements[old_idx].dl = dl;
> + cpudl_heapify(cp, old_idx);
> }
>
> -out:
> raw_spin_unlock_irqrestore(&cp->lock, flags);
> }
>
> diff --git a/kernel/sched/cpudeadline.h b/kernel/sched/cpudeadline.h
> index fcbdf83..f7da8c5 100644
> --- a/kernel/sched/cpudeadline.h
> +++ b/kernel/sched/cpudeadline.h
> @@ -23,7 +23,8 @@ struct cpudl {
> #ifdef CONFIG_SMP
> int cpudl_find(struct cpudl *cp, struct task_struct *p,
> struct cpumask *later_mask);
> -void cpudl_set(struct cpudl *cp, int cpu, u64 dl, int is_valid);
> +void cpudl_set(struct cpudl *cp, int cpu, u64 dl);
> +void cpudl_clear(struct cpudl *cp, int cpu);
> int cpudl_init(struct cpudl *cp);
> void cpudl_set_freecpu(struct cpudl *cp, int cpu);
> void cpudl_clear_freecpu(struct cpudl *cp, int cpu);
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index fcb7f02..f2e8f47 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -795,7 +795,7 @@ static void inc_dl_deadline(struct dl_rq *dl_rq, u64
> deadline)
> if (dl_rq->earliest_dl.curr == 0 ||
> dl_time_before(deadline, dl_rq->earliest_dl.curr)) {
> dl_rq->earliest_dl.curr = deadline;
> - cpudl_set(&rq->rd->cpudl, rq->cpu, deadline, 1);
> + cpudl_set(&rq->rd->cpudl, rq->cpu, deadline);
> }
> }
>
> @@ -810,14 +810,14 @@ static void dec_dl_deadline(struct dl_rq *dl_rq, u64
> deadline)
> if (!dl_rq->dl_nr_running) {
> dl_rq->earliest_dl.curr = 0;
> dl_rq->earliest_dl.next = 0;
> - cpudl_set(&rq->rd->cpudl, rq->cpu, 0, 0);
> + cpudl_clear(&rq->rd->cpudl, rq->cpu);
> } else {
> struct rb_node *leftmost = dl_rq->rb_leftmost;
> struct sched_dl_entity *entry;
>
> entry = rb_entry(leftmost, struct sched_dl_entity, rb_node);
> dl_rq->earliest_dl.curr = entry->deadline;
> - cpudl_set(&rq->rd->cpudl, rq->cpu, entry->deadline, 1);
> + cpudl_set(&rq->rd->cpudl, rq->cpu, entry->deadline);
> }
> }
>
> @@ -1668,7 +1668,7 @@ static void rq_online_dl(struct rq *rq)
>
> cpudl_set_freecpu(&rq->rd->cpudl, rq->cpu);
> if (rq->dl.dl_nr_running > 0)
> - cpudl_set(&rq->rd->cpudl, rq->cpu, rq->dl.earliest_dl.curr, 1);
> + cpudl_set(&rq->rd->cpudl, rq->cpu, rq->dl.earliest_dl.curr);
> }
>
> /* Assumes rq->lock is held */
> @@ -1677,7 +1677,7 @@ static void rq_offline_dl(struct rq *rq)
> if (rq->dl.overloaded)
> dl_clear_overload(rq);
>
> - cpudl_set(&rq->rd->cpudl, rq->cpu, 0, 0);
> + cpudl_clear(&rq->rd->cpudl, rq->cpu);
> cpudl_clear_freecpu(&rq->rd->cpudl, rq->cpu);
> }
>
> --
Apart from the minor nitpicks above, the change looks good to me and it
shouldn't introduce any functional changes (maybe worth stating it in
the changelog).
Best,
- Juri
