On Aug 12, 2016 9:31 PM, "Waiman Long" <[email protected]> wrote: > > On 08/12/2016 01:16 PM, Dave Hansen wrote: >> >> On 08/12/2016 10:01 AM, Waiman Long wrote: >>> >>> The reason for using a special lock is that I want both sequence number >>> update and locking to be done together atomically. They can be made >>> separate as is in the seqlock. However, that will make the code more >>> complex to make sure that all the threads see a consistent set of lock >>> state and sequence number. >> >> Why do we need a sequence number? The "cached" HPET itself could be used. >> >> I'm thinking something like below could use a spinlock instead of the >> doing a custom cmpxchg sequence. The spin_is_locked() should allow the >> contended "readers" to avoid using atomics. >> >> spinlock_t hpet_lock; >> u32 hpet_value; >> ... >> { >> u32 old_hpet = READ_ONCE(hpet_value); >> u32 new_hpet; >> >> // need to ensure that the spin_is_locked() is ordered after >> // the READ_ONCE(). >> smp_rmb(); >> // spin_is_locked() doesn't do atomics >> if (!spin_is_locked(&hpet_lock)&& spin_trylock(&hpet_lock)) { >> >> WRITE_ONCE(hpet_value, real_read_hpet()); >> spin_unlock(&hpet_lock); >> return hpet_value; >> } >> // Contended case. We spin here waiting for the guy who holds >> // the lock to write a new value to 'hpet_value'. >> // >> // We know that our old_hpet is older than our check for the >> // spinlock being locked. So, someone must either have already >> // updated it or be updating it. >> do { >> cpu_relax(); >> // We do not do a rmb() here. We don't need a guarantee >> // that this read is up-to-date, just that it will >> // _eventually_ see an up-to-date value. >> new_hpet = READ_ONCE(hpet_value); >> } while (old_hpet == new_hpet); >> return new_hpet; >> } > > > Yes, I think that work too. I will update my patch accordingly. Thanks for > the input.
Why is Dave more convincing than I was a couple months ago when I asked a similar question? :) I don't think this is right. If the HPET ever returns the same value twice in a row (unlikely because it's generally too slow to read, but it's plausible that someone will make a fast HPET some day), then this could deadlock. Also, does this code need to be NMI-safe? This implementation is deadlocky if it's called from an NMI. The original code was wait-free, right? That was a nice property, too. --Andy
