Re: [PATCH v4 5/8] hpet: make main counter read lock-less

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 25 Aug 2025 22:55:43 +0800
Zhao Liu <zhao1.liu@intel.com> wrote:

> On Thu, Aug 14, 2025 at 06:05:57PM +0200, Igor Mammedov wrote:
> > Date: Thu, 14 Aug 2025 18:05:57 +0200
> > From: Igor Mammedov <imammedo@redhat.com>
> > Subject: [PATCH v4 5/8] hpet: make main counter read lock-less
> > 
> > Make access to main HPET counter lock-less.
> > 
> > In unlikely event of an update in progress, readers will busy wait
> > untill update is finished.
> > 
> > As result micro benchmark of concurrent reading of HPET counter
> > with large number of vCPU shows over 80% better (less) latency.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > ---
> > v3:
> >   * make reader busy wait during update and reuse existing seqlock API
> >        Peter Xu <peterx@redhat.com>
> > ---
> >  hw/timer/hpet.c | 26 ++++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 6 deletions(-)  
>  
> ...
> 
> > -    QEMU_LOCK_GUARD(&s->lock);
> >      if (addr == HPET_COUNTER) {
> > -        if (hpet_enabled(s)) {
> > -            cur_tick = hpet_get_ticks(s);
> > -        } else {
> > -            cur_tick = s->hpet_counter;
> > -        }
> > +        unsigned version;
> > +
> > +        /*
> > +         * Write update is rare, so busywait here is unlikely to happen
> > +         */
> > +        do {
> > +            version = seqlock_read_begin(&s->state_version);
> > +            if (unlikely(!hpet_enabled(s))) {  
> 
> is there any particular consideration for rearranging the order of the
> conditional branches here (and not directly using likely(hpet_enable()))?

not really, I suppose it should be the same either way.

> 
> > +                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;
> >      }  
> 
> Nice imprvoment!
> 
> Reviewed-by: Zhao Liu <zhao1.liu@intel.com>
> 

thanks!