Re: [RFC PATCH 4/4] Split cpudl_set() into cpudl_set() and cpudl_clear().

[Juri Lelli <juri.lelli@arm.com>]

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 <peterz@infradead.org>
> Cc: Juri Lelli <juri.lelli@arm.com>
> Cc: Luca Abeni <luca.abeni@unitn.it>
> Reviewed-by: Luca Abeni <luca.abeni@unitn.it>
> Signed-off-by: Tommaso Cucinotta <tommaso.cucinotta@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