On Mon, Jun 08, 2015 at 04:27:49PM +0200, Oleg Nesterov wrote:
> On 06/08, Peter Zijlstra wrote:
> >
> > On Mon, Jun 08, 2015 at 11:14:17AM +0200, Peter Zijlstra wrote:
> > > > Finally. Suppose that timer->function() returns HRTIMER_RESTART
> > > > and hrtimer_active() is called right after __run_hrtimer() sets
> > > > cpu_base->running = NULL. I can't understand why hrtimer_active()
> > > > can't miss ENQUEUED in this case. We have wmb() in between, yes,
> > > > but then hrtimer_active() should do something like
> > > >
> > > > active = cpu_base->running == timer;
> > > > if (!active) {
> > > > rmb();
> > > > active = state != HRTIMER_STATE_INACTIVE;
> > > > }
> > > >
> > > > No?
> > >
> > > Hmm, good point. Let me think about that. It would be nice to be able to
> > > avoid more memory barriers.
>
> Yes, but otoh, can't we avoid seqcount_t altogether?
>
> To remind, we assume that
>
> - "false positive" is fine. If we observe ENQUEUED or ->running
> we can safely return true. It doesn't matter if the timer becomes
> "inactive" right after return.
>
> - we need to fix migrate_hrtimer_list() and __hrtimer_start_range_ns()
> to preserve ENQUEUED. This fixes the races with hrtimer_is_queued()
> and hrtimer_active() we currently have.
>
> Now, can't we simply do
>
> __run_hrtimer()
> {
>
> cpu_base->running = timer;
>
> wmb(); // 1
>
> __remove_hrtimer(INACTIVE); // clears ENQUEUED
>
> fn(); // autorearm can set ENQUEUED
> again
>
> wmb(); // 2
>
> cpu_base->running = NULL; // XXX
> }
>
> hrtimer_active(timer)
> {
> if (timer->state & ENQUEUED)
> return true;
>
> rmb(); // pairs with 1
>
>
> // We do not care if we race with __hrtimer_start_range_ns().
> // The running timer can't change its base.
> // If it was ENQUEUED, we rely on the previous check.
>
> base = timer->base->cpu_base;
> read_barrier_depends();
> if (base->running == timer)
> return true;
>
> rmb(); // pairs with 2
>
> // Avoid the race with auto-rearming timer. If we see the
> // result of XXX above we should also see ENQUEUED if it
> // was set by __run_hrtimer() or timer->function().
> //
> // We do not care if another thread does hrtimer_start()
> // and we miss ENQUEUED. In this case we can the "inactive"
> // window anyway, we can pretend that hrtimer_start() was
> // called after XXX above. So we can equally pretend that
> // hrtimer_active() was called in this window.
> //
> if (timer->state & ENQUEUED)
> return true;
>
> return false;
> }
>
> Most probably I missed something... I'll try to think more, but perhaps
> you see a hole immediately?
This is something I proposed earlier; Kirill said:
lkml.kernel.org/r/2134411433408...@web8j.yandex.ru
Which I read like the below, imagine our timer expires periodically and
rearms itself:
acquire
cpu_base->running = timer;
wmb
timer->state = INACTIVE;
release
[R] timer->state (== INACTIVE)
fn()
acquire
timer->state = ACTIVE
wmb
cpu_base->running = NULL
release
[R] cpu_base->running (== NULL)
acquire
cpu_base->running = timer;
wmb
timer->state = INACTIVE;
release
[R] timer->state (== INACTIVE)
fn()
acquire
timer->state = ACTIVE
wmb
cpu_base->running = NULL
release
And we have a false negative.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
