On Thu, 31 Jul 2025 10:02:06 -0400
Peter Xu <pet...@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 <pet...@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 */
>
>
