From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935717AbcHBRII (ORCPT ); Tue, 2 Aug 2016 13:08:08 -0400 Received: from foss.arm.com ([217.140.101.70]:38622 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932820AbcHBLwp (ORCPT ); Tue, 2 Aug 2016 07:52:45 -0400 Date: Tue, 2 Aug 2016 12:52:07 +0100 From: Juri Lelli To: Tommaso Cucinotta Cc: Luca Abeni , Juri Lelli , Peter Zijlstra , Ingo Molnar , linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 4/4] Split cpudl_set() into cpudl_set() and cpudl_clear(). Message-ID: <20160802115207.GB22472@e106622-lin> References: <1468921493-10054-1-git-send-email-tommaso.cucinotta@sssup.it> <1468921493-10054-5-git-send-email-tommaso.cucinotta@sssup.it> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1468921493-10054-5-git-send-email-tommaso.cucinotta@sssup.it> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Cc: Juri Lelli > Cc: Luca Abeni > Reviewed-by: Luca Abeni > Signed-off-by: Tommaso Cucinotta > --- > 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