All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexey Brodkin <Alexey.Brodkin@synopsys.com>
To: "peterz@infradead.org" <peterz@infradead.org>
Cc: "linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Alexey.Brodkin@synopsys.com" <Alexey.Brodkin@synopsys.com>,
	"Vineet.Gupta1@synopsys.com" <Vineet.Gupta1@synopsys.com>,
	"linux-snps-arc@lists.infradead.org"
	<linux-snps-arc@lists.infradead.org>
Subject: Re: Do we need to disable preemption in flush_tlb_range()?
Date: Thu, 15 Mar 2018 09:39:31 +0000	[thread overview]
Message-ID: <1521106770.11552.70.camel@synopsys.com> (raw)
In-Reply-To: <20180315082720.GT4064@hirez.programming.kicks-ass.net>

Hi Peter,

On Thu, 2018-03-15 at 09:27 +0100, Peter Zijlstra wrote:
> On Wed, Mar 14, 2018 at 01:19:01PM -0700, Vineet Gupta wrote:
> > +CC Peter since we have his attention ;-)
> 
> Yeah, timezone collision there, I typically sleep at 1am ;-)
> 
> > On 03/01/2018 07:13 AM, Alexey Brodkin wrote:
> > > Hi Vineet,
> > > 
> > > Just noticed that in comments for smp_call_function_many() it is said that
> > > preemption must be disabled during its execution. And that function gets executed
> > > among other ways like that:
> > > -------------------------->8-----------------------
> > >    flush_tlb_range()
> > >      -> on_each_cpu_mask()
> > >           -> smp_call_function_many()
> > > -------------------------->8-----------------------
> > 
> > In general I prefer not to - Peter what say you ?
> 
> The comment with smp_call_function_many() is correct, it relies on
> preemption being disabled in a number of ways. I would expect
> this_cpu_ptr() for example to complain when used with preemption
> enabled (CONFIG_DEBUG_PREEMPT).

I just tried CONFIG_DEBUG_PREEMPT and the only thing I got was that:
-------------------------->8-----------------------
ARC perf        : 8 counters (32 bits), 32 conditions, [overflow IRQ support]
BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1
caller is arc_pmu_device_probe+0x24e/0x29c
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.14.14+ #67

Stack Trace:
  arc_unwind_core.constprop.1+0xd0/0xf4
  dump_stack+0x64/0x7c
  debug_smp_processor_id+0xb8/0xbc
  arc_pmu_device_probe+0x24e/0x29c
  platform_drv_probe+0x26/0x5c
  really_probe+0x288/0x338
  __driver_attach+0xc4/0xc8
  bus_for_each_dev+0x38/0x70
  bus_add_driver+0x12a/0x18c
  driver_register+0x50/0xec
  do_one_initcall+0x32/0x108
  kernel_init_freeable+0xfe/0x188
-------------------------->8-----------------------

That happens because in PMU probe routine we want to
configure IRQ handlers on all other cores:
-------------------------->8-----------------------
  arc_pmu_device_probe() ->
    on_each_cpu(arc_cpu_pmu_irq_init, &irq, 1): preempt_disable() ->
      enable_percpu_irq(irq, IRQ_TYPE_NONE) ->
        smp_processor_id() with disabled preemption.
-------------------------->8-----------------------

Which poses another preemption related question - how do IRQ setup on
all cores properly? :)

-Alexey

WARNING: multiple messages have this Message-ID (diff)
From: Alexey.Brodkin@synopsys.com (Alexey Brodkin)
To: linux-snps-arc@lists.infradead.org
Subject: Do we need to disable preemption in flush_tlb_range()?
Date: Thu, 15 Mar 2018 09:39:31 +0000	[thread overview]
Message-ID: <1521106770.11552.70.camel@synopsys.com> (raw)
In-Reply-To: <20180315082720.GT4064@hirez.programming.kicks-ass.net>

Hi Peter,

On Thu, 2018-03-15@09:27 +0100, Peter Zijlstra wrote:
> On Wed, Mar 14, 2018@01:19:01PM -0700, Vineet Gupta wrote:
> > +CC Peter since we have his attention ;-)
> 
> Yeah, timezone collision there, I typically sleep at 1am ;-)
> 
> > On 03/01/2018 07:13 AM, Alexey Brodkin wrote:
> > > Hi Vineet,
> > > 
> > > Just noticed that in comments for smp_call_function_many() it is said that
> > > preemption must be disabled during its execution. And that function gets executed
> > > among other ways like that:
> > > -------------------------->8-----------------------
> > >    flush_tlb_range()
> > >      -> on_each_cpu_mask()
> > >           -> smp_call_function_many()
> > > -------------------------->8-----------------------
> > 
> > In general I prefer not to - Peter what say you ?
> 
> The comment with smp_call_function_many() is correct, it relies on
> preemption being disabled in a number of ways. I would expect
> this_cpu_ptr() for example to complain when used with preemption
> enabled (CONFIG_DEBUG_PREEMPT).

I just tried CONFIG_DEBUG_PREEMPT and the only thing I got was that:
-------------------------->8-----------------------
ARC perf        : 8 counters (32 bits), 32 conditions, [overflow IRQ support]
BUG: using smp_processor_id() in preemptible [00000000] code: swapper/0/1
caller is arc_pmu_device_probe+0x24e/0x29c
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.14.14+ #67

Stack Trace:
  arc_unwind_core.constprop.1+0xd0/0xf4
  dump_stack+0x64/0x7c
  debug_smp_processor_id+0xb8/0xbc
  arc_pmu_device_probe+0x24e/0x29c
  platform_drv_probe+0x26/0x5c
  really_probe+0x288/0x338
  __driver_attach+0xc4/0xc8
  bus_for_each_dev+0x38/0x70
  bus_add_driver+0x12a/0x18c
  driver_register+0x50/0xec
  do_one_initcall+0x32/0x108
  kernel_init_freeable+0xfe/0x188
-------------------------->8-----------------------

That happens because in PMU probe routine we want to
configure IRQ handlers on all other cores:
-------------------------->8-----------------------
  arc_pmu_device_probe() ->
    on_each_cpu(arc_cpu_pmu_irq_init, &irq, 1): preempt_disable() ->
      enable_percpu_irq(irq, IRQ_TYPE_NONE) ->
        smp_processor_id() with disabled preemption.
-------------------------->8-----------------------

Which poses another preemption related question - how do IRQ setup on
all cores properly? :)

-Alexey

  reply	other threads:[~2018-03-15  9:39 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-01 15:13 Do we need to disable preemption in flush_tlb_range()? Alexey Brodkin
2018-03-01 15:13 ` Alexey Brodkin
2018-03-14 19:15 ` Alexey Brodkin
2018-03-14 19:15   ` Alexey Brodkin
2018-03-14 20:19 ` Vineet Gupta
2018-03-14 20:19   ` Vineet Gupta
2018-03-15  8:27   ` Peter Zijlstra
2018-03-15  8:27     ` Peter Zijlstra
2018-03-15  8:27     ` Peter Zijlstra
2018-03-15  9:39     ` Alexey Brodkin [this message]
2018-03-15  9:39       ` Alexey Brodkin
2018-03-15 17:32       ` Vineet Gupta
2018-03-15 17:32         ` Vineet Gupta
2018-03-16 10:11       ` Peter Zijlstra
2018-03-16 10:11         ` Peter Zijlstra
2018-03-16 10:11         ` Peter Zijlstra
2018-03-16 15:01         ` Alexey Brodkin
2018-03-16 15:01           ` Alexey Brodkin

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=1521106770.11552.70.camel@synopsys.com \
    --to=alexey.brodkin@synopsys.com \
    --cc=Vineet.Gupta1@synopsys.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-snps-arc@lists.infradead.org \
    --cc=peterz@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.