On Mon, 25 Aug 2025 22:55:43 +0800
Zhao Liu <zhao1....@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 <imamm...@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 <imamm...@redhat.com>
> > Reviewed-by: Peter Xu <pet...@redhat.com>
> > ---
> > v3:
> > * make reader busy wait during update and reuse existing seqlock API
> > Peter Xu <pet...@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....@intel.com>
>
thanks!
