LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Valentin Schneider <valentin.schneider@arm.com>,
	linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org,
	Will Deacon <will@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Marc Zyngier <maz@kernel.org>,
	Mel Gorman <mgorman@techsingularity.net>
Subject: Re: [SPLAT 1/3] arm_pmu: Sleeping spinlocks down armpmu_alloc_atomic()
Date: Tue, 10 Aug 2021 21:56:45 +0200	[thread overview]
Message-ID: <87y299oyyq.ffs@tglx> (raw)
In-Reply-To: <20210810183914.2wd7hehdz7y4crla@linutronix.de>

On Tue, Aug 10 2021 at 20:39, Sebastian Andrzej Siewior wrote:
> On 2021-08-10 19:16:19 [+0200], Thomas Gleixner wrote:
>> On Tue, Aug 10 2021 at 15:54, Sebastian Andrzej Siewior wrote:
>> > On 2021-08-10 14:41:25 [+0100], Valentin Schneider wrote:
>> >> [    4.172817] BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:35
>> >
>> > Would it work to allocate the memory upfront and invoke the HP callback
>> > via smp_function_call()?
>> 
>> Huch? If the memory is allocated during the prepare stage then the
>> actual hotplug callback can just use it. So what's the SMP function call
>> for?
>
> You allocate the memory per-node (based on cpuid id) and you figure that
> out on the target CPU. That is the smp-function call for unless there
> another way to gather that information.
> That is done in
>  arm_pmu_acpi_find_alloc_pmu() -> read_cpuid_id()

There is no SMP function call involved. This is straight CPU hotplug
context on the upcoming CPU.

You cannot allocate upfront on the control CPU and do an SMP function
call to a not even started CPU. No idea what you are trying to do, but
this is going nowhere.

The reason why this atomic allocation is done is described in this
commit:

commit 0dc1a1851af1d593eee248b94c1277c7c7ccbbce
Author: Mark Rutland <mark.rutland@arm.com>
Date:   Mon Feb 5 16:41:58 2018 +0000

    arm_pmu: add armpmu_alloc_atomic()
    
    In ACPI systems, we don't know the makeup of CPUs until we hotplug them
    on, and thus have to allocate the PMU datastructures at hotplug time.
    Thus, we must use GFP_ATOMIC allocations.

I have no idea why ACPI does not expose topology and PMU information in
the first place, but do I really want to know?

So the easy way out is to preallocate one PMU and use that preallocation
when needed in the starting callback. Completely untested patch
below. Note, that because I'm lazy this will waste one preallocated PMU
at the end, but that's trivial enough to clean up.

Thanks,

        tglx
---
--- a/drivers/perf/arm_pmu.c
+++ b/drivers/perf/arm_pmu.c
@@ -861,12 +861,12 @@ static void cpu_pmu_destroy(struct arm_p
 					    &cpu_pmu->node);
 }
 
-static struct arm_pmu *__armpmu_alloc(gfp_t flags)
+struct arm_pmu *armpmu_alloc(void)
 {
 	struct arm_pmu *pmu;
 	int cpu;
 
-	pmu = kzalloc(sizeof(*pmu), flags);
+	pmu = kzalloc(sizeof(*pmu), GFP_KERNEL);
 	if (!pmu)
 		goto out;
 
@@ -916,17 +916,6 @@ static struct arm_pmu *__armpmu_alloc(gf
 	return NULL;
 }
 
-struct arm_pmu *armpmu_alloc(void)
-{
-	return __armpmu_alloc(GFP_KERNEL);
-}
-
-struct arm_pmu *armpmu_alloc_atomic(void)
-{
-	return __armpmu_alloc(GFP_ATOMIC);
-}
-
-
 void armpmu_free(struct arm_pmu *pmu)
 {
 	free_percpu(pmu->hw_events);
--- a/drivers/perf/arm_pmu_acpi.c
+++ b/drivers/perf/arm_pmu_acpi.c
@@ -185,6 +185,8 @@ static int arm_pmu_acpi_parse_irqs(void)
 	return err;
 }
 
+static struct arm_pmu *arm_pmu_prealloced;
+
 static struct arm_pmu *arm_pmu_acpi_find_alloc_pmu(void)
 {
 	unsigned long cpuid = read_cpuid_id();
@@ -199,15 +201,9 @@ static struct arm_pmu *arm_pmu_acpi_find
 		return pmu;
 	}
 
-	pmu = armpmu_alloc_atomic();
-	if (!pmu) {
-		pr_warn("Unable to allocate PMU for CPU%d\n",
-			smp_processor_id());
-		return NULL;
-	}
-
+	pmu = arm_pmu_prealloced;
+	arm_pmu_prealloced = NULL;
 	pmu->acpi_cpuid = cpuid;
-
 	return pmu;
 }
 
@@ -284,6 +280,19 @@ static int arm_pmu_acpi_cpu_starting(uns
 	return 0;
 }
 
+static int arm_pmu_acpi_cpu_prepare(unsigned int cpu)
+{
+	if (per_cpu(probed_pmus, cpu) || arm_pmu_prealloced)
+		return 0;
+
+	arm_pmu_prealloced = armpmu_alloc();
+	if (!arm_pmu_prealloced) {
+		pr_warn("Unable to allocate PMU for CPU%d\n", cpu);
+		return -ENOMEM;
+	}
+	return 0;
+}
+
 int arm_pmu_acpi_probe(armpmu_init_fn init_fn)
 {
 	int pmu_idx = 0;
@@ -338,7 +347,7 @@ int arm_pmu_acpi_probe(armpmu_init_fn in
 
 static int arm_pmu_acpi_init(void)
 {
-	int ret;
+	int ret, dynstate;
 
 	if (acpi_disabled)
 		return 0;
@@ -349,9 +358,17 @@ static int arm_pmu_acpi_init(void)
 	if (ret)
 		return ret;
 
+	dynstate = cpuhp_setup_state(CPUHP_BP_PREPARE_DYN,
+				     "perf/arm/pmu_acpi:prepare",
+				     arm_pmu_acpi_cpu_prepare, NULL);
+	if (dynstate < 0)
+		return dynstate;
+
 	ret = cpuhp_setup_state(CPUHP_AP_PERF_ARM_ACPI_STARTING,
 				"perf/arm/pmu_acpi:starting",
 				arm_pmu_acpi_cpu_starting, NULL);
+	if (ret)
+		cpuhp_remove_state(dynstate);
 
 	return ret;
 }
--- a/include/linux/perf/arm_pmu.h
+++ b/include/linux/perf/arm_pmu.h
@@ -165,7 +165,6 @@ static inline int arm_pmu_acpi_probe(arm
 
 /* Internal functions only for core arm_pmu code */
 struct arm_pmu *armpmu_alloc(void);
-struct arm_pmu *armpmu_alloc_atomic(void);
 void armpmu_free(struct arm_pmu *pmu);
 int armpmu_register(struct arm_pmu *pmu);
 int armpmu_request_irq(int irq, int cpu);

  reply	other threads:[~2021-08-10 19:56 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-10 13:41 [SPLAT 0/3] Sleeping spinlocks in troublesome contexts on v5.14-rc4-rt6 Valentin Schneider
2021-08-10 13:41 ` [SPLAT 1/3] arm_pmu: Sleeping spinlocks down armpmu_alloc_atomic() Valentin Schneider
2021-08-10 13:54   ` Sebastian Andrzej Siewior
2021-08-10 17:16     ` Thomas Gleixner
2021-08-10 18:39       ` Sebastian Andrzej Siewior
2021-08-10 19:56         ` Thomas Gleixner [this message]
2021-08-10 13:41 ` [SPLAT 2/3] irqchip/gic-v3-its: Sleeping spinlocks down gic_reserve_range() Valentin Schneider
2021-08-11  8:50   ` Marc Zyngier
2021-08-11 12:28     ` Thomas Gleixner
2021-08-11 15:14       ` Marc Zyngier
2021-08-17 15:16     ` Ard Biesheuvel
2021-08-10 13:41 ` [SPLAT 3/3] gpio: dwapb: Sleeping spinlocks down IRQ mapping Valentin Schneider
2021-08-10 20:33   ` Thomas Gleixner
2021-08-11 10:52   ` Andy Shevchenko

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=87y299oyyq.ffs@tglx \
    --to=tglx@linutronix.de \
    --cc=bigeasy@linutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=maz@kernel.org \
    --cc=mgorman@techsingularity.net \
    --cc=valentin.schneider@arm.com \
    --cc=will@kernel.org \
    --subject='Re: [SPLAT 1/3] arm_pmu: Sleeping spinlocks down armpmu_alloc_atomic()' \
    /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).