On Tuesday 20 October 2015 02:05 AM, Alexey Brodkin wrote:
> Hi Vineet,
>
> Looking at a patch that Peter Z mentioned:
> http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/include/linux/perf_event.h?id=b0e878759452314676f
> bdd71df4ac67e7d08de5d
>
>
> And it says here http://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/include/linux/perf_event.h#n160
> --------->8----------
> /*
> * The last observed hardware counter value, updated with a
> * local64_cmpxchg() such that pmu::read() can be called nested.
> */
> local64_t prev_count;
> --------->8----------
>
> So now I think we may want to return to local64_cmpxchg() in arc_perf_event_update() as well.
> The reason is having no control over generic perf code we cannot really guarantee that pmu->read() won't
> happen at random moment right before we enter perf IRQ handler (where we execute arc_perf_event_update() directly).
>
> In other words arc_perf_event_update() is used in IRQ handler and outside it and chances are that
> function will be reentered at some point.
>
> Agree?
Let's check with Peter as I'm not sure how exactly the read call will nest for
same counter on same core ?
But if they do then indeed, then commit 1fe8bfa5ff3b (ARCv2: perf: implement
"event_set_period") needs to be partially reverted.
-Vineet
On Tue, Nov 17, 2015 at 09:14:59AM +0000, Vineet Gupta wrote:
> Let's check with Peter as I'm not sure how exactly the read call will
> nest for same counter on same core ?
Various possible ways, but the easiest is userspace doing a sys_read()
on the counter while the NMI happens.
On Tuesday 17 November 2015 04:37 PM, Peter Zijlstra wrote:
> On Tue, Nov 17, 2015 at 09:14:59AM +0000, Vineet Gupta wrote:
>> > Let's check with Peter as I'm not sure how exactly the read call will
>> > nest for same counter on same core ?
> Various possible ways, but the easiest is userspace doing a sys_read()
> on the counter while the NMI happens.
>
>
That means Alexey need to revert the hunk ?
static void arc_perf_event_update(struct perf_event *event,
struct hw_perf_event *hwc, int idx)
{
- uint64_t prev_raw_count, new_raw_count;
- int64_t delta;
-
- do {
- prev_raw_count = local64_read(&hwc->prev_count);
- new_raw_count = arc_pmu_read_counter(idx);
- } while (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
- new_raw_count) != prev_raw_count);
-
- delta = (new_raw_count - prev_raw_count) &
- ((1ULL << arc_pmu->counter_size) - 1ULL);
+ uint64_t prev_raw_count = local64_read(&hwc->prev_count);
+ uint64_t new_raw_count = arc_pmu_read_counter(idx);
+ int64_t delta = new_raw_count - prev_raw_count;
+ /*
+ * We don't afaraid of hwc->prev_count changing beneath our feet
+ * because there's no way for us to re-enter this function anytime.
+ */
+ local64_set(&hwc->prev_count, new_raw_count);
On Tue, Nov 17, 2015 at 04:53:04PM +0530, Vineet Gupta wrote:
> That means Alexey need to revert the hunk ?
Yes, I think so.
On Tue, Nov 17, 2015 at 01:24:01PM +0100, Peter Zijlstra wrote:
> On Tue, Nov 17, 2015 at 04:53:04PM +0530, Vineet Gupta wrote:
> > That means Alexey need to revert the hunk ?
>
> Yes, I think so.
This is assuming you now have these NMIs we talked about earlier. If all
you have are regular IRQs this is not possible, for we should be calling
->read() with IRQs disabled.
On Tuesday 17 November 2015 05:55 PM, Peter Zijlstra wrote:
> This is assuming you now have these NMIs we talked about earlier. If all
> you have are regular IRQs this is not possible, for we should be calling
> ->read() with IRQs disabled.
>
No we don't yet. The first stab at it fell flat on floor.
The NMI support from hardware is that is it provides different priorities, higher
one obviously able to interrupt lower one. However instructions like CLRI (disable
interrupts) will still lock out all interrupts.
Thus local_irq_save()/restore() and local_irq_enable()/disable() now need to be
contextual.
- When running in prio 0 mode, they only need to enable 0
- In prio 1, they need to enable both 0 and 1
For irq_save()/restore() this is achievable by doing an additional STATUS32 read
at the time of save and passing that value to restore - so there's an additional
overhead - but ignoring that for now.
Bummer is irq_disable()/enable() case: there's need to pass old prio state from
enable to disabled, so we need some sort of global state tracking - which in case
of SMP needs to be per cpu.... either keep something hot in a reg or pay the cost
of additional mem/cache line miss.
I've not investigated how other arches do that. PPC seems to be using some sort of
soft irq state anyways.
On Tue, Nov 17, 2015 at 06:23:21PM +0530, Vineet Gupta wrote:
> On Tuesday 17 November 2015 05:55 PM, Peter Zijlstra wrote:
>
> > This is assuming you now have these NMIs we talked about earlier. If all
> > you have are regular IRQs this is not possible, for we should be calling
> > ->read() with IRQs disabled.
> >
>
> No we don't yet. The first stab at it fell flat on floor.
>
> The NMI support from hardware is that is it provides different priorities, higher
> one obviously able to interrupt lower one. However instructions like CLRI (disable
> interrupts) will still lock out all interrupts.
>
> Thus local_irq_save()/restore() and local_irq_enable()/disable() now need to be
> contextual.
>
> - When running in prio 0 mode, they only need to enable 0
> - In prio 1, they need to enable both 0 and 1
>
> For irq_save()/restore() this is achievable by doing an additional STATUS32 read
> at the time of save and passing that value to restore - so there's an additional
> overhead - but ignoring that for now.
>
> Bummer is irq_disable()/enable() case: there's need to pass old prio state from
> enable to disabled, so we need some sort of global state tracking - which in case
> of SMP needs to be per cpu.... either keep something hot in a reg or pay the cost
> of additional mem/cache line miss.
>
> I've not investigated how other arches do that. PPC seems to be using some sort of
> soft irq state anyways.
Yeah, Sparc64 might be a better example, it more closely matches your
hardware. See
arch/sparc/include/asm/irqflags_64.h:arch_local_irq_save().
On Tuesday 17 November 2015 05:55 PM, Peter Zijlstra wrote:
> On Tue, Nov 17, 2015 at 01:24:01PM +0100, Peter Zijlstra wrote:
>> > On Tue, Nov 17, 2015 at 04:53:04PM +0530, Vineet Gupta wrote:
>>> > > That means Alexey need to revert the hunk ?
>> >
>> > Yes, I think so.
> This is assuming you now have these NMIs we talked about earlier. If all
> you have are regular IRQs this is not possible, for we should be calling
> ->read() with IRQs disabled.
Ok then there is not need for this change ATM. I'll queue up the revert in the
branch which brings NMI support !
Thx,
-Vineet