All of lore.kernel.org
 help / color / mirror / Atom feed
From: marc.zyngier@arm.com (Marc Zyngier)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] irqchip/gicv3: silence noisy DEBUG_PER_CPU_MAPS warning
Date: Mon, 19 Sep 2016 18:38:40 +0100	[thread overview]
Message-ID: <57E022A0.9020501@arm.com> (raw)
In-Reply-To: <1474306155-3303-1-git-send-email-james.morse@arm.com>

Hi James,

On 19/09/16 18:29, James Morse wrote:
> gic_raise_softirq() walks the list of cpus using for_each_cpu(), it calls
> gic_compute_target_list() which advances the iterator by the number of
> CPUs in the cluster.
> 
> If gic_compute_target_list() reaches the last CPU it leaves the iterator
> pointing at the last CPU. This means the next time round the for_each_cpu()
> loop cpumask_next() will be called with an invalid CPU.
> 
> This triggers a warning when built with CONFIG_DEBUG_PER_CPU_MAPS:
> [    3.077738] GICv3: CPU1: found redistributor 1 region 0:0x000000002f120000
> [    3.077943] CPU1: Booted secondary processor [410fd0f0]
> [    3.078542] ------------[ cut here ]------------
> [    3.078746] WARNING: CPU: 1 PID: 0 at ../include/linux/cpumask.h:121 gic_raise_softirq+0x12c/0x170
> [    3.078812] Modules linked in:
> [    3.078869]
> [    3.078930] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.8.0-rc5+ #5188
> [    3.078994] Hardware name: Foundation-v8A (DT)
> [    3.079059] task: ffff80087a1a0080 task.stack: ffff80087a19c000
> [    3.079145] PC is at gic_raise_softirq+0x12c/0x170
> [    3.079226] LR is at gic_raise_softirq+0xa4/0x170
> [    3.079296] pc : [<ffff0000083ead24>] lr : [<ffff0000083eac9c>] pstate: 200001c9
> [    3.081139] Call trace:
> [    3.081202] Exception stack(0xffff80087a19fbe0 to 0xffff80087a19fd10)
> 
> [    3.082269] [<ffff0000083ead24>] gic_raise_softirq+0x12c/0x170
> [    3.082354] [<ffff00000808e614>] smp_send_reschedule+0x34/0x40
> [    3.082433] [<ffff0000080e80a0>] resched_curr+0x50/0x88
> [    3.082512] [<ffff0000080e89d0>] check_preempt_curr+0x60/0xd0
> [    3.082593] [<ffff0000080e8a60>] ttwu_do_wakeup+0x20/0xe8
> [    3.082672] [<ffff0000080e8bb8>] ttwu_do_activate+0x90/0xc0
> [    3.082753] [<ffff0000080ea9a4>] try_to_wake_up+0x224/0x370
> [    3.082836] [<ffff0000080eabc8>] default_wake_function+0x10/0x18
> [    3.082920] [<ffff000008103134>] __wake_up_common+0x5c/0xa0
> [    3.083003] [<ffff0000081031f4>] __wake_up_locked+0x14/0x20
> [    3.083086] [<ffff000008103f80>] complete+0x40/0x60
> [    3.083168] [<ffff00000808df7c>] secondary_start_kernel+0x15c/0x1d0
> [    3.083240] [<00000000808911a4>] 0x808911a4
> [    3.113401] Detected PIPT I-cache on CPU2
> 
> Avoid updating the iterator if the next call to cpumask_next() would
> cause the for_each_cpu() loop to exit.
> 
> There is no change to gic_raise_softirq()'s behaviour, (cpumask_next()s
> eventual call to _find_next_bit() will return early as start >= nbits),
> this patch just silences the warning.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  drivers/irqchip/irq-gic-v3.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index ede5672ab34d..9e37c447cc2c 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -548,6 +548,7 @@ static int gic_starting_cpu(unsigned int cpu)
>  static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
>  				   unsigned long cluster_id)
>  {
> +	int next_cpu;
>  	int cpu = *base_cpu;
>  	unsigned long mpidr = cpu_logical_map(cpu);
>  	u16 tlist = 0;
> @@ -562,9 +563,10 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
>  
>  		tlist |= 1 << (mpidr & 0xf);
>  
> -		cpu = cpumask_next(cpu, mask);
> -		if (cpu >= nr_cpu_ids)
> +		next_cpu = cpumask_next(cpu, mask);
> +		if (next_cpu >= nr_cpu_ids)
>  			goto out;
> +		cpu = next_cpu;
>  
>  		mpidr = cpu_logical_map(cpu);
>  
> 

It took me a while to convince myself that the original code was
correct, and it obviously wasn't... Oh well, paper bag time.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

Thomas, any chance you could pick that one for -rc8?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <marc.zyngier@arm.com>
To: James Morse <james.morse@arm.com>, linux-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org,
	Thomas Gleixner <tglx@linutronix.de>,
	Jason Cooper <jason@lakedaemon.net>
Subject: Re: [PATCH] irqchip/gicv3: silence noisy DEBUG_PER_CPU_MAPS warning
Date: Mon, 19 Sep 2016 18:38:40 +0100	[thread overview]
Message-ID: <57E022A0.9020501@arm.com> (raw)
In-Reply-To: <1474306155-3303-1-git-send-email-james.morse@arm.com>

Hi James,

On 19/09/16 18:29, James Morse wrote:
> gic_raise_softirq() walks the list of cpus using for_each_cpu(), it calls
> gic_compute_target_list() which advances the iterator by the number of
> CPUs in the cluster.
> 
> If gic_compute_target_list() reaches the last CPU it leaves the iterator
> pointing at the last CPU. This means the next time round the for_each_cpu()
> loop cpumask_next() will be called with an invalid CPU.
> 
> This triggers a warning when built with CONFIG_DEBUG_PER_CPU_MAPS:
> [    3.077738] GICv3: CPU1: found redistributor 1 region 0:0x000000002f120000
> [    3.077943] CPU1: Booted secondary processor [410fd0f0]
> [    3.078542] ------------[ cut here ]------------
> [    3.078746] WARNING: CPU: 1 PID: 0 at ../include/linux/cpumask.h:121 gic_raise_softirq+0x12c/0x170
> [    3.078812] Modules linked in:
> [    3.078869]
> [    3.078930] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.8.0-rc5+ #5188
> [    3.078994] Hardware name: Foundation-v8A (DT)
> [    3.079059] task: ffff80087a1a0080 task.stack: ffff80087a19c000
> [    3.079145] PC is at gic_raise_softirq+0x12c/0x170
> [    3.079226] LR is at gic_raise_softirq+0xa4/0x170
> [    3.079296] pc : [<ffff0000083ead24>] lr : [<ffff0000083eac9c>] pstate: 200001c9
> [    3.081139] Call trace:
> [    3.081202] Exception stack(0xffff80087a19fbe0 to 0xffff80087a19fd10)
> 
> [    3.082269] [<ffff0000083ead24>] gic_raise_softirq+0x12c/0x170
> [    3.082354] [<ffff00000808e614>] smp_send_reschedule+0x34/0x40
> [    3.082433] [<ffff0000080e80a0>] resched_curr+0x50/0x88
> [    3.082512] [<ffff0000080e89d0>] check_preempt_curr+0x60/0xd0
> [    3.082593] [<ffff0000080e8a60>] ttwu_do_wakeup+0x20/0xe8
> [    3.082672] [<ffff0000080e8bb8>] ttwu_do_activate+0x90/0xc0
> [    3.082753] [<ffff0000080ea9a4>] try_to_wake_up+0x224/0x370
> [    3.082836] [<ffff0000080eabc8>] default_wake_function+0x10/0x18
> [    3.082920] [<ffff000008103134>] __wake_up_common+0x5c/0xa0
> [    3.083003] [<ffff0000081031f4>] __wake_up_locked+0x14/0x20
> [    3.083086] [<ffff000008103f80>] complete+0x40/0x60
> [    3.083168] [<ffff00000808df7c>] secondary_start_kernel+0x15c/0x1d0
> [    3.083240] [<00000000808911a4>] 0x808911a4
> [    3.113401] Detected PIPT I-cache on CPU2
> 
> Avoid updating the iterator if the next call to cpumask_next() would
> cause the for_each_cpu() loop to exit.
> 
> There is no change to gic_raise_softirq()'s behaviour, (cpumask_next()s
> eventual call to _find_next_bit() will return early as start >= nbits),
> this patch just silences the warning.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  drivers/irqchip/irq-gic-v3.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index ede5672ab34d..9e37c447cc2c 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -548,6 +548,7 @@ static int gic_starting_cpu(unsigned int cpu)
>  static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
>  				   unsigned long cluster_id)
>  {
> +	int next_cpu;
>  	int cpu = *base_cpu;
>  	unsigned long mpidr = cpu_logical_map(cpu);
>  	u16 tlist = 0;
> @@ -562,9 +563,10 @@ static u16 gic_compute_target_list(int *base_cpu, const struct cpumask *mask,
>  
>  		tlist |= 1 << (mpidr & 0xf);
>  
> -		cpu = cpumask_next(cpu, mask);
> -		if (cpu >= nr_cpu_ids)
> +		next_cpu = cpumask_next(cpu, mask);
> +		if (next_cpu >= nr_cpu_ids)
>  			goto out;
> +		cpu = next_cpu;
>  
>  		mpidr = cpu_logical_map(cpu);
>  
> 

It took me a while to convince myself that the original code was
correct, and it obviously wasn't... Oh well, paper bag time.

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

Thomas, any chance you could pick that one for -rc8?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

  reply	other threads:[~2016-09-19 17:38 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-19 17:29 [PATCH] irqchip/gicv3: silence noisy DEBUG_PER_CPU_MAPS warning James Morse
2016-09-19 17:29 ` James Morse
2016-09-19 17:38 ` Marc Zyngier [this message]
2016-09-19 17:38   ` Marc Zyngier
2016-09-19 19:13 ` [tip:irq/urgent] irqchip/gicv3: Silence " tip-bot for James Morse
2016-09-19 23:48 ` tip-bot for James Morse

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=57E022A0.9020501@arm.com \
    --to=marc.zyngier@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.