linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ionela Voinescu <ionela.voinescu@arm.com>
To: Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com>
Cc: catalin.marinas@arm.com, will@kernel.org, mark.rutland@arm.com,
	maz@kernel.org, sudeep.holla@arm.com, lukasz.luba@arm.com,
	valentin.schneider@arm.com, rjw@rjwysocki.net,
	peterz@infradead.org, mingo@redhat.com,
	vincent.guittot@linaro.org, viresh.kumar@linaro.org,
	linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org
Subject: Re: [PATCH v3 1/7] arm64: add support for the AMU extension v1
Date: Wed, 12 Feb 2020 18:20:08 +0000	[thread overview]
Message-ID: <20200212182008.GA25421@arm.com> (raw)
In-Reply-To: <133890f7-59bb-63b9-0ca8-2294e3596058@arm.com>

Hi Suzuki,

On Wednesday 12 Feb 2020 at 16:20:56 (+0000), Suzuki Kuruppassery Poulose wrote:
> > > > +static bool has_amu(const struct arm64_cpu_capabilities *cap,
> > > > +		       int __unused)
> > > > +{
> > > > +	/*
> > > > +	 * The AMU extension is a non-conflicting feature: the kernel can
> > > > +	 * safely run a mix of CPUs with and without support for the
> > > > +	 * activity monitors extension. Therefore, if not disabled through
> > > > +	 * the kernel command line early parameter, enable the capability
> > > > +	 * to allow any late CPU to use the feature.
> > > > +	 *
> > > > +	 * With this feature enabled, the cpu_enable function will be called
> > > > +	 * for all CPUs that match the criteria, including secondary and
> > > > +	 * hotplugged, marking this feature as present on that respective CPU.
> > > > +	 * The enable function will also print a detection message.
> > > > +	 */
> > > > +
> > > > +	if (!disable_amu && !zalloc_cpumask_var(&amu_cpus, GFP_KERNEL)) {
> > > 
> > > This looks problematic. Don't we end up in allocating the memory during
> > > "each CPU" check and thus leaking memory ? Do we really need to allocate
> > > this dynamically ?
> > > 
> > 
> > Yes, it does make some assumptions. Given that the AMU capability is
> > a WEAK_LOCAL_CPU_FEATURE I relied on the match function being called
> > only once, when the return value is true. If the return value is false,
> 
> That is not correct. A WEAK_LOCAL_CPU_FEATURE is still SCOPE_LOCAL_CPU,
> implies it is run on all the booting CPUs (including the hotplugged
> ones). The WEAK is there to imply that its "permitted" or "optional"
> for a hotplugged CPU. So, eventually you will re-allocate this variable
> every single time a CPU turns up, where you could also loose the current
> state.
> 

> > which will result in it being called multiple times, it's either because
> > disable_amu == false, or it has become false due to a previous failed
> > allocation, in which case a new allocation will not be attempted.

First of all, I agree with you that this should be corrected.

But for completion (and my education) I retraced my steps in regards
to my assumption above. While cpu_enable is called for all CPUs - boot,
secondary, hotplugged, the matches function (in this case has_amu) is
not always called for all CPUs, and that's where the confusion came
from.

Looking over the update_cpu_capabilities function, that's called from
both setup_boot_cpu_capabilities and check_local_cpu_capabilities
(secondary CPUs) for SCOPE_LOCAL_CPU:

-----
static void update_cpu_capabilities(u16 scope_mask)
{
        int i;
        const struct arm64_cpu_capabilities *caps;

        scope_mask &= ARM64_CPUCAP_SCOPE_MASK;
        for (i = 0; i < ARM64_NCAPS; i++) {
                caps = cpu_hwcaps_ptrs[i];
                if (!caps || !(caps->type & scope_mask) ||
                    cpus_have_cap(caps->capability) ||
                    !caps->matches(caps, cpucap_default_scope(caps)))
                        continue;

--> The matches function is only called if !cpus_have_cap


                if (caps->desc)
                        pr_info("detected: %s\n", caps->desc);
                cpus_set_cap(caps->capability);

--> If matches returns true we mark it as present in cpu_hwcaps.

                if ((scope_mask & SCOPE_BOOT_CPU) && (caps->type & SCOPE_BOOT_CPU))
                        set_bit(caps->capability, boot_capabilities);
        }   
}
---

Therefore caps->matches (in this case has_amu) will only be called as
long as it returns false. This is where my assumption above came from.
Also, this is the reason it was working nicely in my testing, as I did
not test hotplug this time.

Where the has_amu code breaks is when we end up calling
verify_local_cpu_capabilities instead of update_cpu_capabilities after
sys_caps_initialised, which will happen for hotplugged CPUs.
In that case we call caps->matches for all CPUs. Also, if anyone in the
future ends up calling this_cpu_has_cap for the AMU capability.

I will fix this.

Thank you,
Ionela.


  reply	other threads:[~2020-02-12 18:20 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-11 18:45 [PATCH v3 0/7] arm64: ARMv8.4 Activity Monitors support Ionela Voinescu
2020-02-11 18:45 ` [PATCH v3 1/7] arm64: add support for the AMU extension v1 Ionela Voinescu
2020-02-12 11:30   ` Suzuki Kuruppassery Poulose
2020-02-12 14:54     ` Valentin Schneider
2020-02-12 16:10     ` Ionela Voinescu
2020-02-12 16:20       ` Suzuki Kuruppassery Poulose
2020-02-12 18:20         ` Ionela Voinescu [this message]
2020-02-12 19:24           ` Suzuki K Poulose
2020-02-12 20:19         ` Ionela Voinescu
2020-02-12 16:24       ` Vladimir Murzin
2020-02-12 18:27         ` Ionela Voinescu
2020-02-11 18:45 ` [PATCH v3 2/7] arm64: trap to EL1 accesses to AMU counters from EL0 Ionela Voinescu
2020-02-12 11:44   ` Suzuki Kuruppassery Poulose
2020-02-12 15:36   ` Valentin Schneider
2020-02-11 18:45 ` [PATCH v3 3/7] arm64/kvm: disable access to AMU registers from kvm guests Ionela Voinescu
2020-02-12 15:36   ` Valentin Schneider
2020-02-12 16:33   ` Suzuki Kuruppassery Poulose
2020-02-11 18:45 ` [PATCH v3 4/7] Documentation: arm64: document support for the AMU extension Ionela Voinescu
2020-02-12 15:36   ` Valentin Schneider
2020-02-11 18:45 ` [PATCH v3 5/7] cpufreq: add function to get the hardware max frequency Ionela Voinescu
2020-02-12  4:14   ` Viresh Kumar
2020-02-13 11:59   ` Valentin Schneider
2020-02-13 12:59     ` Ionela Voinescu
2020-02-13 15:22       ` Valentin Schneider
2020-02-11 18:45 ` [PATCH v3 6/7] arm64: use activity monitors for frequency invariance Ionela Voinescu
2020-02-12 18:59   ` Lukasz Luba
2020-02-13  9:47     ` Ionela Voinescu
2020-02-17 16:59   ` Valentin Schneider
2020-02-23 18:49     ` Ionela Voinescu
2020-02-11 18:45 ` [PATCH v3 7/7] clocksource/drivers/arm_arch_timer: validate arch_timer_rate Ionela Voinescu
2020-02-12  9:30   ` Valentin Schneider
2020-02-12 10:32     ` Ionela Voinescu
2020-02-12 10:01   ` Lukasz Luba
2020-02-12 10:12     ` Marc Zyngier
2020-02-12 10:54       ` Ionela Voinescu
2020-02-12 10:55       ` Lukasz Luba
2020-02-12 11:10         ` Marc Zyngier
2020-02-12 11:43           ` Lukasz Luba
2020-02-12 11:12         ` Valentin Schneider
2020-02-14  0:35   ` Thomas Gleixner
2020-02-14 15:45     ` Ionela Voinescu
2020-02-14 15:57       ` Ionela Voinescu

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=20200212182008.GA25421@arm.com \
    --to=ionela.voinescu@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=lukasz.luba@arm.com \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rjw@rjwysocki.net \
    --cc=sudeep.holla@arm.com \
    --cc=suzuki.poulose@arm.com \
    --cc=valentin.schneider@arm.com \
    --cc=vincent.guittot@linaro.org \
    --cc=viresh.kumar@linaro.org \
    --cc=will@kernel.org \
    --subject='Re: [PATCH v3 1/7] arm64: add support for the AMU extension v1' \
    /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).