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 16:10:45 +0000	[thread overview]
Message-ID: <20200212161045.GA7475@arm.com> (raw)
In-Reply-To: <93472f17-6465-641d-ea82-3230b5697ffd@arm.com>

Hi Suzuki,

On Wednesday 12 Feb 2020 at 11:30:44 (+0000), Suzuki Kuruppassery Poulose wrote:
> > +static int __init set_disable_amu(char *str)
> > +{
> > +	int value = 0;
> > +
> > +	disable_amu = get_option(&str, &value) ? !!value : true;
> 
> minor nit: You could simply use strtobool(str) here, which accepts:
> 
> disable_amu= [0/1/on/off/y/n]
>

Yes, this was intentional as I wanted "disable_amu" to be a valid option
as well, not only "disable_amu=<option>".

If you don't mind I'd like to keep it like this. Currently the use of
AMU is enabled by default, and the most common kernel parameter to
disable it would be "disable_amu". Allowing "disable_amu=0" is just in
case we change the default in the kernel to not support AMU and we'd
like platforms to be able to enable it. 

> 
> > +
> > +	return 0;
> > +}
> > +early_param("disable_amu", set_disable_amu);
> > +
> > +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,
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.

For better handling I could have a cpumask_available check before the
allocation just in case the capability type changes in the future, or to
at least not rely on assumptions based on the type of the capability.

The reason this is dynamic is that I wanted to avoid the memory being
allocated when disable_amu is true - as Valentin mentioned in a comment
in the meantime "the static allocation is done against NR_CPUS whereas
the dynamic one is done against nr_cpu_ids".

Would this be alright?

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 182e05ca3410..4cee6b147ddd 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1222,7 +1222,11 @@ static bool has_amu(const struct arm64_cpu_capabilities *cap,
         * The enable function will also print a detection message.
         */
 
-       if (!disable_amu && !zalloc_cpumask_var(&amu_cpus, GFP_KERNEL)) {
+       if (disable_amu)
+               return false;
+
+       if (!cpumask_available(amu_cpus) &&
+           !zalloc_cpumask_var(&amu_cpus, GFP_KERNEL)) {
                pr_err("Activity Monitors Unit (AMU): fail to allocate memory");
                disable_amu = true;
        }

Otherwise I can go for static allocation.

Thank you,
Ionela.

  parent reply	other threads:[~2020-02-12 16:10 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 [this message]
2020-02-12 16:20       ` Suzuki Kuruppassery Poulose
2020-02-12 18:20         ` Ionela Voinescu
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=20200212161045.GA7475@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).