Messages in this thread |  | | Date | Mon, 8 Jun 2015 16:27:49 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH 08/14] hrtimer: Allow hrtimer::function() to free the timer |
| |
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?
Oleg.
|  |