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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 31 Jul 2025 10:02:06 -0400
Peter Xu <peterx@redhat.com> wrote:

> 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.

I'll try it for the next respin

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

docs put qatomic_inc() in 'Sequentially consistent' category,
hence no manual barrier.
before that I've used weak qatomic_store_release(), but
qatomic_inc() should do increment and store that in one go.

> > > 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.

it's mostly need for comments that goes away.
but you are right,
it's very not likely to happen. so busywait vs lock probably won't matter. 

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

indeed check is not really 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 */
> 
>