Re: [PATCH v2 5/6] hpet: make main counter read lock-less

[Peter Xu <peterx@redhat.com>]

On Thu, Jul 31, 2025 at 10:32:10AM +0200, Igor Mammedov wrote:
> On Wed, 30 Jul 2025 18:15:03 -0400
> Peter Xu <peterx@redhat.com> wrote:
> 
> > On Wed, Jul 30, 2025 at 02:39:33PM +0200, Igor Mammedov wrote:
> > > Make access to main HPET counter lock-less when enable/disable
> > > state isn't changing (which is the most of the time).
> > > 
> > > A read will fallback to locked access if the state change happens
> > > in the middle of read or read happens in the middle of the state
> > > change.
> > > 
> > > This basically uses the same approach as cpu_get_clock(),
> > > modulo instead of busy wait it piggibacks to taking device lock
> > > to wait until HPET reaches consistent state.  
> > 
> > The open-coded seqlock will slightly add complexity of the hpet code.  Is
> > it required? IOW, is it common to have concurrent writters while reading?
> 
> Write path has to be lock protected for correctness sake even though
> concurrent writers are not likely.

Right.  Though we have seqlock_write_lock() for that, IIUC (even though
maybe in hpet's use case we don't need it..).

> 
> I've tried seqlock as well, the difference wrt seqlock is few LOC only
> it didn't make HPET code any simpler.

I tried to do this and it looks still worthwhile to do, but maybe I missed
something alone the lines. Please have a look if so.  That is still a lot
of LOC saved, meanwhile IMHO the important part is mem barriers are just
tricky to always hard-code in users, so I thought it would always be nice
to reuse the lock APIs whenever possible.

One example is, IIUC this current patch may have missed the mem barriers
when boosting state_version in hpet_ram_write().

> 
> > How bad it is to spin on read waiting for the writer to finish?
> that will waste CPU cycles, and on large NUMA system it will generate
> more cross node traffic. (i.e. it would scale badly, though TBH
> I don't have numbers. I think measuring it would be hard as it
> would drown in the noise.)
> 
> hence I've opted for a more effective option, to halt readers
> until update is done. (at the cost of latency spike when that
> unlikely event happens)

If it is extremely unlikely (IIUC, disabling HPET while someone is using /
reading the counter.. should never happen in normal production?), would
spinning read also be fine?  Maybe that's also why I can save more LOCs in
the diff below.

In the diff I also removed a "addr <= 0xff" check, that might belong to a
prior patch that I thought is not needed.

Thanks,

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index d822ca1cd0..09a84d19f3 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -39,6 +39,7 @@
 #include "system/address-spaces.h"
 #include "qom/object.h"
 #include "qemu/lockable.h"
+#include "qemu/seqlock.h"
 #include "trace.h"
 
 struct hpet_fw_config hpet_fw_cfg = {.count = UINT8_MAX};
@@ -74,7 +75,7 @@ struct HPETState {
     MemoryRegion iomem;
     uint64_t hpet_offset;
     bool hpet_offset_saved;
-    unsigned state_version;
+    QemuSeqLock state_version;
     qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
     uint32_t flags;
     uint8_t rtc_irq_level;
@@ -431,39 +432,17 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
     trace_hpet_ram_read(addr);
     addr &= ~4;
 
-    if ((addr <= 0xff) && (addr == HPET_COUNTER)) {
+    if (addr == HPET_COUNTER) {
         unsigned version;
-        bool release_lock = false;
-redo:
-        version = qatomic_load_acquire(&s->state_version);
-        if (unlikely(version & 1)) {
-                /*
-                 * Updater is running, state can be inconsistent
-                 * wait till it's done before reading counter
-                 */
-                release_lock = true;
-                qemu_mutex_lock(&s->lock);
-        }
-
-        if (unlikely(!hpet_enabled(s))) {
-            cur_tick = s->hpet_counter;
-        } else {
-            cur_tick = hpet_get_ticks(s);
-        }
-
-        /*
-         * ensure counter math happens before we check version again
-         */
-        smp_rmb();
-        if (unlikely(version != qatomic_load_acquire(&s->state_version))) {
-            /*
-             * counter state has changed, re-read counter again
-             */
-            goto redo;
-        }
-        if (unlikely(release_lock)) {
-            qemu_mutex_unlock(&s->lock);
-        }
+        /* Write update is extremely rare, so spinning is fine */
+        do {
+            version = seqlock_read_begin(&s->state_version);
+            if (unlikely(!hpet_enabled(s))) {
+                cur_tick = s->hpet_counter;
+            } else {
+                cur_tick = hpet_get_ticks(s);
+            }
+        } while (seqlock_read_retry(&s->state_version, version));
         trace_hpet_ram_read_reading_counter(addr & 4, cur_tick);
         return cur_tick >> shift;
     }
@@ -528,11 +507,7 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
             old_val = s->config;
             new_val = deposit64(old_val, shift, len, value);
             new_val = hpet_fixup_reg(new_val, old_val, HPET_CFG_WRITE_MASK);
-            /*
-             * Odd versions mark the critical section, any readers will be
-             * forced into lock protected read if they come in the middle of it
-             */
-            qatomic_inc(&s->state_version);
+            seqlock_write_begin(&s->state_version);
             s->config = new_val;
             if (activating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
                 /* Enable main counter and interrupt generation. */
@@ -551,12 +526,7 @@ static void hpet_ram_write(void *opaque, hwaddr addr,
                     hpet_del_timer(&s->timer[i]);
                 }
             }
-            /*
-             * even versions mark the end of critical section,
-             * any readers started before config change, but were still executed
-             * during the change, will be forced to re-read counter state
-             */
-            qatomic_inc(&s->state_version);
+            seqlock_write_end(&s->state_version);
 
             /* i8254 and RTC output pins are disabled
              * when HPET is in legacy mode */


-- 
Peter Xu