linux-kernel - Re: [PATCH] drivers/perf: arm-pmu: convert arm_pmu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20160803173645.GB1267@leverpostej>
Date:	Wed, 3 Aug 2016 18:36:45 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	Sudeep Holla <sudeep.holla@....com>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Stephen Boyd <sboyd@...eaurora.org>,
	Will Deacon <will.deacon@....com>
Subject: Re: [PATCH] drivers/perf: arm-pmu: convert arm_pmu_mutex to spinlock

Hi,

Sebastian, as a heads-up, the issue below issue may apply to over
hotplug state machine conversions if a mutex was added.

On Wed, Aug 03, 2016 at 06:08:55PM +0100, Sudeep Holla wrote:
> arm_pmu_mutex is never held long and we don't want to sleep while the
> lock is being held as it's executed in the context of hotplug notifiers.
> So it can be converted to a simple spinlock instead.
> 
> Without this patch we get the following warning:
> 
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:620
> in_atomic(): 1, irqs_disabled(): 128, pid: 0, name: swapper/2
> no locks held by swapper/2/0.
> irq event stamp: 381314
> hardirqs last  enabled at (381313): _raw_spin_unlock_irqrestore+0x7c/0x88
> hardirqs last disabled at (381314): cpu_die+0x28/0x48
> softirqs last  enabled at (381294): _local_bh_enable+0x28/0x50
> softirqs last disabled at (381293): irq_enter+0x58/0x78
> CPU: 2 PID: 0 Comm: swapper/2 Not tainted 4.7.0 #12
> Call trace:
>  dump_backtrace+0x0/0x220
>  show_stack+0x24/0x30
>  dump_stack+0xb4/0xf0
>  ___might_sleep+0x1d8/0x1f0
>  __might_sleep+0x5c/0x98
>  mutex_lock_nested+0x54/0x400
>  arm_perf_starting_cpu+0x34/0xb0
>  cpuhp_invoke_callback+0x88/0x3d8
>  notify_cpu_starting+0x78/0x98
>  secondary_start_kernel+0x108/0x1a8
> 
> This patch converts the mutex to spinlock to eliminate the above
> warnings. This constraints pmu->reset to be non-blocking call which is
> the case with all the ARM PMU backends.
> Fixes: 37b502f121ad ("arm/perf: Fix hotplug state machine conversion")
> Cc: Stephen Boyd <sboyd@...eaurora.org>
> Cc: Will Deacon <will.deacon@....com>
> Cc: Mark Rutland <mark.rutland@....com>
> Signed-off-by: Sudeep Holla <sudeep.holla@....com>

I haven't been able to come up with a race with hotplug that halted
forward progress, and otherwise this is at least as structurally correct
as the mutex, so FWIW:

Acked-by: Mark Rutland <mark.rutland@....com>

Mark.

> ---
>  drivers/perf/arm_pmu.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c
> index 6ccb994bdfcb..4c9a537a1265 100644
> --- a/drivers/perf/arm_pmu.c
> +++ b/drivers/perf/arm_pmu.c
> @@ -688,7 +688,7 @@ static int cpu_pmu_request_irq(struct arm_pmu *cpu_pmu, irq_handler_t handler)
>  	return 0;
>  }
> 
> -static DEFINE_MUTEX(arm_pmu_mutex);
> +static DEFINE_SPINLOCK(arm_pmu_lock);
>  static LIST_HEAD(arm_pmu_list);
> 
>  /*
> @@ -701,7 +701,7 @@ static int arm_perf_starting_cpu(unsigned int cpu)
>  {
>  	struct arm_pmu *pmu;
> 
> -	mutex_lock(&arm_pmu_mutex);
> +	spin_lock(&arm_pmu_lock);
>  	list_for_each_entry(pmu, &arm_pmu_list, entry) {
> 
>  		if (!cpumask_test_cpu(cpu, &pmu->supported_cpus))
> @@ -709,7 +709,7 @@ static int arm_perf_starting_cpu(unsigned int cpu)
>  		if (pmu->reset)
>  			pmu->reset(pmu);
>  	}
> -	mutex_unlock(&arm_pmu_mutex);
> +	spin_unlock(&arm_pmu_lock);
>  	return 0;
>  }
> 
> @@ -821,9 +821,9 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
>  	if (!cpu_hw_events)
>  		return -ENOMEM;
> 
> -	mutex_lock(&arm_pmu_mutex);
> +	spin_lock(&arm_pmu_lock);
>  	list_add_tail(&cpu_pmu->entry, &arm_pmu_list);
> -	mutex_unlock(&arm_pmu_mutex);
> +	spin_unlock(&arm_pmu_lock);
> 
>  	err = cpu_pm_pmu_register(cpu_pmu);
>  	if (err)
> @@ -859,9 +859,9 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
>  	return 0;
> 
>  out_unregister:
> -	mutex_lock(&arm_pmu_mutex);
> +	spin_lock(&arm_pmu_lock);
>  	list_del(&cpu_pmu->entry);
> -	mutex_unlock(&arm_pmu_mutex);
> +	spin_unlock(&arm_pmu_lock);
>  	free_percpu(cpu_hw_events);
>  	return err;
>  }
> @@ -869,9 +869,9 @@ static int cpu_pmu_init(struct arm_pmu *cpu_pmu)
>  static void cpu_pmu_destroy(struct arm_pmu *cpu_pmu)
>  {
>  	cpu_pm_pmu_unregister(cpu_pmu);
> -	mutex_lock(&arm_pmu_mutex);
> +	spin_lock(&arm_pmu_lock);
>  	list_del(&cpu_pmu->entry);
> -	mutex_unlock(&arm_pmu_mutex);
> +	spin_unlock(&arm_pmu_lock);
>  	free_percpu(cpu_pmu->hw_events);
>  }
> 
> --
> 2.7.4
>