From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754702AbbIBMlc (ORCPT ); Wed, 2 Sep 2015 08:41:32 -0400 Received: from mail-la0-f44.google.com ([209.85.215.44]:32981 "EHLO mail-la0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754085AbbIBMlb (ORCPT ); Wed, 2 Sep 2015 08:41:31 -0400 MIME-Version: 1.0 In-Reply-To: References: <1439569394-11974-1-git-send-email-morten.rasmussen@arm.com> <1439569394-11974-3-git-send-email-morten.rasmussen@arm.com> From: Vincent Guittot Date: Wed, 2 Sep 2015 14:41:09 +0200 Message-ID: Subject: Re: [PATCH 2/6] sched/fair: Convert arch_scale_cpu_capacity() from weak function to #define To: Morten Rasmussen Cc: Peter Zijlstra , "mingo@redhat.com" , Daniel Lezcano , Dietmar Eggemann , Yuyang Du , Michael Turquette , "rjw@rjwysocki.net" , Juri Lelli , Sai Charan Gurrappadi , pang.xunlei@zte.com.cn, linux-kernel Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2 September 2015 at 11:31, Vincent Guittot wrote: > Hi Morten, > > On 14 August 2015 at 18:23, Morten Rasmussen wrote: >> Bring arch_scale_cpu_capacity() in line with the recent change of its >> arch_scale_freq_capacity() sibling in commit dfbca41f3479 ("sched: >> Optimize freq invariant accounting") from weak function to #define to >> allow inlining of the function. >> >> While at it, remove the ARCH_CAPACITY sched_feature as well. With the >> change to #define there isn't a straightforward way to allow runtime >> switch between an arch implementation and the default implementation of >> arch_scale_cpu_capacity() using sched_feature. The default was to use >> the arch-specific implementation, but only the arm architecture provides >> one and that is essentially equivalent to the default implementation. >> >> cc: Ingo Molnar >> cc: Peter Zijlstra >> >> Signed-off-by: Morten Rasmussen >> --- >> kernel/sched/fair.c | 22 +--------------------- >> kernel/sched/features.h | 5 ----- >> kernel/sched/sched.h | 11 +++++++++++ >> 3 files changed, 12 insertions(+), 26 deletions(-) >> >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >> index 1626410..c72223a 100644 >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -6016,19 +6016,6 @@ static inline int get_sd_load_idx(struct sched_domain *sd, >> return load_idx; >> } >> >> -static unsigned long default_scale_cpu_capacity(struct sched_domain *sd, int cpu) >> -{ >> - if ((sd->flags & SD_SHARE_CPUCAPACITY) && (sd->span_weight > 1)) >> - return sd->smt_gain / sd->span_weight; >> - >> - return SCHED_CAPACITY_SCALE; >> -} >> - >> -unsigned long __weak arch_scale_cpu_capacity(struct sched_domain *sd, int cpu) >> -{ >> - return default_scale_cpu_capacity(sd, cpu); >> -} >> - >> static unsigned long scale_rt_capacity(int cpu) >> { >> struct rq *rq = cpu_rq(cpu); >> @@ -6058,16 +6045,9 @@ static unsigned long scale_rt_capacity(int cpu) >> >> static void update_cpu_capacity(struct sched_domain *sd, int cpu) >> { >> - unsigned long capacity = SCHED_CAPACITY_SCALE; >> + unsigned long capacity = arch_scale_cpu_capacity(sd, cpu); >> struct sched_group *sdg = sd->groups; >> >> - if (sched_feat(ARCH_CAPACITY)) >> - capacity *= arch_scale_cpu_capacity(sd, cpu); >> - else >> - capacity *= default_scale_cpu_capacity(sd, cpu); >> - >> - capacity >>= SCHED_CAPACITY_SHIFT; >> - >> cpu_rq(cpu)->cpu_capacity_orig = capacity; >> >> capacity *= scale_rt_capacity(cpu); >> diff --git a/kernel/sched/features.h b/kernel/sched/features.h >> index 83a50e7..6565eac 100644 >> --- a/kernel/sched/features.h >> +++ b/kernel/sched/features.h >> @@ -36,11 +36,6 @@ SCHED_FEAT(CACHE_HOT_BUDDY, true) >> */ >> SCHED_FEAT(WAKEUP_PREEMPTION, true) >> >> -/* >> - * Use arch dependent cpu capacity functions >> - */ >> -SCHED_FEAT(ARCH_CAPACITY, true) >> - >> SCHED_FEAT(HRTICK, false) >> SCHED_FEAT(DOUBLE_TICK, false) >> SCHED_FEAT(LB_BIAS, true) >> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h >> index 22ccc55..7e6f250 100644 >> --- a/kernel/sched/sched.h >> +++ b/kernel/sched/sched.h >> @@ -1402,6 +1402,17 @@ unsigned long arch_scale_freq_capacity(struct sched_domain *sd, int cpu) >> } >> #endif >> >> +#ifndef arch_scale_cpu_capacity >> +static __always_inline >> +unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu) >> +{ >> + if ((sd->flags & SD_SHARE_CPUCAPACITY) && (sd->span_weight > 1)) >> + return sd->smt_gain / sd->span_weight; >> + >> + return SCHED_CAPACITY_SCALE; >> +} >> +#endif >> + > > So you change the way to declare arch_scale_cpu_capacity but i don't > see the update of the arm arch which declare a > arch_scale_cpu_capacity to reflect this change in your series. You mentioned in the cover letter that the arch part will be posted separately later but i'm not sure that it's a good idea to separate the change of an interface and the update of the archs that implement it. IMHO, it's better to keep all modification together in one patchset Regards, Vincent > > Regards, > Vincent > >> static inline void sched_rt_avg_update(struct rq *rq, u64 rt_delta) >> { >> rq->rt_avg += rt_delta * arch_scale_freq_capacity(NULL, cpu_of(rq)); >> -- >> 1.9.1 >>