linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ionela Voinescu <ionela.voinescu@arm.com>
To: Valentin Schneider <valentin.schneider@arm.com>
Cc: catalin.marinas@arm.com, will@kernel.org, mark.rutland@arm.com,
	maz@kernel.org, suzuki.poulose@arm.com, sudeep.holla@arm.com,
	dietmar.eggemann@arm.com, peterz@infradead.org, mingo@redhat.com,
	ggherdovich@suse.cz, vincent.guittot@linaro.org,
	linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 6/6] arm64: use activity monitors for frequency invariance
Date: Wed, 29 Jan 2020 17:52:23 +0000	[thread overview]
Message-ID: <20200129175223.GA26494@arm.com> (raw)
In-Reply-To: <96fdead6-9896-5695-6744-413300d424f5@arm.com>

Hi Valentin,

On Wednesday 29 Jan 2020 at 17:13:53 (+0000), Valentin Schneider wrote:
> Only commenting on the bits that should be there regardless of using the
> workqueues or not;
> 
> On 18/12/2019 18:26, Ionela Voinescu wrote:
> > +static void cpu_amu_fie_init_workfn(struct work_struct *work)
> > +{
> > +	u64 core_cnt, const_cnt, ratio;
> > +	struct cpu_amu_work *amu_work;
> > +	int cpu = smp_processor_id();
> > +
> > +	if (!cpu_has_amu_feat()) {
> > +		pr_debug("CPU%d: counters are not supported.\n", cpu);
> > +		return;
> > +	}
> > +
> > +	core_cnt = read_sysreg_s(SYS_AMEVCNTR0_CORE_EL0);
> > +	const_cnt = read_sysreg_s(SYS_AMEVCNTR0_CONST_EL0);
> > +
> > +	if (unlikely(!core_cnt || !const_cnt)) {
> > +		pr_err("CPU%d: cycle counters are not enabled.\n", cpu);
> > +		return;
> > +	}
> > +
> > +	amu_work = container_of(work, struct cpu_amu_work, cpu_work);
> > +	if (unlikely(!(amu_work->cpuinfo_max_freq))) {
> > +		pr_err("CPU%d: invalid maximum frequency.\n", cpu);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * Pre-compute the fixed ratio between the frequency of the
> > +	 * constant counter and the maximum frequency of the CPU (hz).
> 
> I can't resist: s/hz/Hz/
> 
> > +	 */
> > +	ratio = (u64)arch_timer_get_rate() << (2 * SCHED_CAPACITY_SHIFT);
> > +	ratio = div64_u64(ratio, amu_work->cpuinfo_max_freq * 1000);
> 
> Nit: we're missing a comment somewhere that the unit of this is in kHz
> (which explains the * 1000).
> 

Will do! The previous comment that explained this was ".. while
ensuring max_freq is converted to HZ.", but I believed it as too
clear and replaced it with the obscure "(hz)". I'll revert :).

> > +	this_cpu_write(arch_max_freq_scale, (unsigned long)ratio);
> > +
> 
> Okay so what we get in the tick is:
> 
>   /\ core
>   --------
>   /\ const
> 
> And we want that to be SCHED_CAPACITY_SCALE when running at max freq. IOW we
> want to turn
> 
>   max_freq
>   ----------
>   const_freq
> 
> into SCHED_CAPACITY_SCALE, so we can just multiply that by:
> 
>   const_freq
>   ---------- * SCHED_CAPACITY_SCALE
>   max_freq
> 
> But what the ratio you are storing here is 
> 
>                           const_freq
>   arch_max_freq_scale =   ---------- * SCHED_CAPACITY_SCALE²
>                            max_freq
> 
> (because x << 2 * SCHED_CAPACITY_SHIFT == x << 20)
> 
> 
> In topology_freq_scale_tick() you end up doing
> 
>   /\ core   arch_max_freq_scale
>   ------- * --------------------
>   /\ const  SCHED_CAPACITY_SCALE
> 
> which gives us what we want (SCHED_CAPACITY_SCALE at max freq).
> 
> 
> Now, the reason why we multiply our ratio by the square of
> SCHED_CAPACITY_SCALE was not obvious to me, but you pointed me out that the
> frequency of the arch timer can be *really* low compared to the max CPU freq.
> 
> For instance on h960:
> 
>   [    0.000000] arch_timer: cp15 timer(s) running at 1.92MHz (phys)
> 
>   $ root@valsch-h960:~# cat /sys/devices/system/cpu/cpufreq/policy4/cpuinfo_max_freq 
>   2362000
> 
> So our ratio would be
> 
>   1'920'000 * 1024
>   ----------------
>     2'362'000'000
> 
> Which is ~0.83, so that becomes simply 0...
> 
> 
> I had a brief look at the Arm ARM, for the arch timer it says it is
> "typically in the range 1-50MHz", but then also gives an example with 20KHz
> in a low-power mode.
> 
> If we take say 5GHz max CPU frequency, our lower bound for the arch timer
> (with that SCHED_CAPACITY_SCALE² trick) is about ~4.768KHz. It's not *too*
> far from that 20KHz, but I'm not sure we would actually be executing stuff
> in that low-power mode.
> 
> Long story short, we're probably fine, but it would nice to shove some of
> the above into comments (especially the SCHED_CAPACITY_SCALE² trick)

Okay, I'll add some of this documentation as comments in the patches. I
thought about doing it but I was not sure it justified the line count.
But if it saves people at least the hassle to unpack this computation to
understand the logic, it will be worth it.

Thank you for the thorough review,
Ionela.

  reply	other threads:[~2020-01-29 17:52 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-18 18:26 [PATCH v2 0/6] arm64: ARMv8.4 Activity Monitors support Ionela Voinescu
2019-12-18 18:26 ` [PATCH v2 1/6] arm64: add support for the AMU extension v1 Ionela Voinescu
2020-01-23 17:04   ` Valentin Schneider
2020-01-23 18:32     ` Ionela Voinescu
2020-01-24 12:00       ` Valentin Schneider
2020-01-28 11:00         ` Ionela Voinescu
2020-01-28 16:34   ` Suzuki Kuruppassery Poulose
2020-01-29 16:42     ` Ionela Voinescu
2019-12-18 18:26 ` [PATCH v2 2/6] arm64: trap to EL1 accesses to AMU counters from EL0 Ionela Voinescu
2020-01-23 17:04   ` Valentin Schneider
2020-01-23 17:34     ` Ionela Voinescu
2019-12-18 18:26 ` [PATCH v2 3/6] arm64/kvm: disable access to AMU registers from kvm guests Ionela Voinescu
2020-01-27 15:33   ` Valentin Schneider
2020-01-28 15:48     ` Ionela Voinescu
2020-01-28 17:26     ` Suzuki Kuruppassery Poulose
2020-01-28 17:37       ` Valentin Schneider
2020-01-28 17:52         ` Ionela Voinescu
2019-12-18 18:26 ` [PATCH v2 4/6] Documentation: arm64: document support for the AMU extension Ionela Voinescu
2020-01-27 16:47   ` Valentin Schneider
2020-01-28 16:53     ` Ionela Voinescu
2020-01-28 18:36       ` Valentin Schneider
2020-01-30 15:04   ` Suzuki Kuruppassery Poulose
2020-01-30 16:45     ` Ionela Voinescu
2020-01-30 18:26       ` Suzuki K Poulose
2020-01-31  9:54         ` Ionela Voinescu
2019-12-18 18:26 ` [PATCH v2 5/6] TEMP: sched: add interface for counter-based frequency invariance Ionela Voinescu
2020-01-29 19:37   ` Peter Zijlstra
2020-01-30 15:33     ` Ionela Voinescu
2019-12-18 18:26 ` [PATCH v2 6/6] arm64: use activity monitors for " Ionela Voinescu
2020-01-23 11:49   ` Lukasz Luba
2020-01-23 17:07     ` Ionela Voinescu
2020-01-24  1:19       ` Lukasz Luba
2020-01-24 13:12         ` Ionela Voinescu
2020-01-24 15:17           ` Lukasz Luba
2020-01-28 17:36             ` Ionela Voinescu
2020-01-29 17:13   ` Valentin Schneider
2020-01-29 17:52     ` Ionela Voinescu [this message]
2020-01-29 23:39     ` Valentin Schneider
2020-01-30 15:49       ` Ionela Voinescu
2020-01-30 16:11         ` Valentin Schneider

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200129175223.GA26494@arm.com \
    --to=ionela.voinescu@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=ggherdovich@suse.cz \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=sudeep.holla@arm.com \
    --cc=suzuki.poulose@arm.com \
    --cc=valentin.schneider@arm.com \
    --cc=vincent.guittot@linaro.org \
    --cc=will@kernel.org \
    --subject='Re: [PATCH v2 6/6] arm64: use activity monitors for frequency invariance' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).