lkml.org 
[lkml]   [2016]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [RESEND PATCH v4] x86/hpet: Reduce HPET counter read contention
    On Aug 12, 2016 9:31 PM, "Waiman Long" <waiman.long@hpe.com> 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

    \
     
     \ /
      Last update: 2016-09-17 09:56    [W:4.317 / U:1.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site