From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753424AbbIDH1H (ORCPT ); Fri, 4 Sep 2015 03:27:07 -0400 Received: from mail-la0-f52.google.com ([209.85.215.52]:36101 "EHLO mail-la0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752561AbbIDH1F (ORCPT ); Fri, 4 Sep 2015 03:27:05 -0400 MIME-Version: 1.0 In-Reply-To: <55E8A65F.2070903@arm.com> References: <1439569394-11974-1-git-send-email-morten.rasmussen@arm.com> <1439569394-11974-3-git-send-email-morten.rasmussen@arm.com> <55E8A65F.2070903@arm.com> From: Vincent Guittot Date: Fri, 4 Sep 2015 09:26:42 +0200 Message-ID: Subject: Re: [PATCH 2/6] sched/fair: Convert arch_scale_cpu_capacity() from weak function to #define To: Dietmar Eggemann Cc: Morten Rasmussen , Peter Zijlstra , "mingo@redhat.com" , Daniel Lezcano , 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 3 September 2015 at 21:58, Dietmar Eggemann wrote: > Hi Vincent, > > On 02/09/15 10: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. > > [...] > >> >> 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. > > We were reluctant to do this because this functionality makes only sense > for ARCH=arm big.Little systems w/ cortex-a{15|7} cores and only if the > clock-frequency property is set in the dts file. IMO, we should maintain the compatibility of current implementation instead of breaking the link and creating a dead code. Your proposal below fits the requirement > > Are you planning to push for a 'struct cpu_efficiency/clock-frequency > property' solution for ARCH=arm64 as well? I know that there has been some discussions aorund that but i didn't follow the thread in details > > I'm asking because for ARCH=arm64 systems today (JUNO, Hi6220) we use the > capacity value of the last entry of the capacity_state vector for the cores > (e.g. cortex-a{57|53). This is a struct of the eas feature ? Not sure that we should link the definition of the cpu capacity to an internal struct of a feature; DT seems a better way to define it. So if you want to revisit the way, we set the capacity of CPU for arm and/or arm64, I'm fully open to the discussion but this should happen in another thread than this one which has for only purpose the alignment of the arch_scale_cpu_capacity interface declaration with arch_scale_freq_capacity one. So, with the patch below that updates the arm definition of arch_scale_cpu_capacity, you can add my Acked-by: Vincent Guittot on this patch and the additional one below Regards, Vincent > > To connect the cpu invariant engine (scale_cpu_capacity() > [arch/arm/kernel/topology.c]) with the scheduler, something like this is > missing: > > diff --git a/arch/arm/include/asm/topology.h b/arch/arm/include/asm/topology.h > index 370f7a732900..17c6b3243196 100644 > --- a/arch/arm/include/asm/topology.h > +++ b/arch/arm/include/asm/topology.h > @@ -24,6 +24,10 @@ void init_cpu_topology(void); > void store_cpu_topology(unsigned int cpuid); > const struct cpumask *cpu_coregroup_mask(int cpu); > > +#define arch_scale_cpu_capacity scale_cpu_capacity > +struct sched_domain; > +extern unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu); > + > #else > > static inline void init_cpu_topology(void) { } > diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c > index 08b7847bf912..907e0d2d9b82 100644 > --- a/arch/arm/kernel/topology.c > +++ b/arch/arm/kernel/topology.c > @@ -42,7 +42,7 @@ > */ > static DEFINE_PER_CPU(unsigned long, cpu_scale); > > -unsigned long arch_scale_cpu_capacity(struct sched_domain *sd, int cpu) > +unsigned long scale_cpu_capacity(struct sched_domain *sd, int cpu) > { > return per_cpu(cpu_scale, cpu); > } > @@ -166,7 +166,7 @@ static void update_cpu_capacity(unsigned int cpu) > set_capacity_scale(cpu, cpu_capacity(cpu) / middle_capacity); > > pr_info("CPU%u: update cpu_capacity %lu\n", > - cpu, arch_scale_cpu_capacity(NULL, cpu)); > + cpu, scale_cpu_capacity(NULL, cpu)); > } > > -- Dietmar > > [...] >