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.
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>
---
hw/timer/hpet.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index 97687697c9..d822ca1cd0 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -74,6 +74,7 @@ struct HPETState {
MemoryRegion iomem;
uint64_t hpet_offset;
bool hpet_offset_saved;
+ unsigned state_version;
qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
uint32_t flags;
uint8_t rtc_irq_level;
@@ -430,17 +431,44 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
trace_hpet_ram_read(addr);
addr &= ~4;
- QEMU_LOCK_GUARD(&s->lock);
if ((addr <= 0xff) && (addr == HPET_COUNTER)) {
- if (hpet_enabled(s)) {
- cur_tick = hpet_get_ticks(s);
- } else {
+ 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);
}
trace_hpet_ram_read_reading_counter(addr & 4, cur_tick);
return cur_tick >> shift;
}
+ QEMU_LOCK_GUARD(&s->lock);
/*address range of all global regs*/
if (addr <= 0xff) {
switch (addr) {
@@ -500,6 +528,11 @@ 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);
s->config = new_val;
if (activating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
/* Enable main counter and interrupt generation. */
@@ -518,6 +551,13 @@ 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);
+
/* i8254 and RTC output pins are disabled
* when HPET is in legacy mode */
if (activating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
--
2.47.1On 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?
How bad it is to spin on read waiting for the writer to finish?
Thanks,
>
> 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>
> ---
> hw/timer/hpet.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> index 97687697c9..d822ca1cd0 100644
> --- a/hw/timer/hpet.c
> +++ b/hw/timer/hpet.c
> @@ -74,6 +74,7 @@ struct HPETState {
> MemoryRegion iomem;
> uint64_t hpet_offset;
> bool hpet_offset_saved;
> + unsigned state_version;
> qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
> uint32_t flags;
> uint8_t rtc_irq_level;
> @@ -430,17 +431,44 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
> trace_hpet_ram_read(addr);
> addr &= ~4;
>
> - QEMU_LOCK_GUARD(&s->lock);
> if ((addr <= 0xff) && (addr == HPET_COUNTER)) {
> - if (hpet_enabled(s)) {
> - cur_tick = hpet_get_ticks(s);
> - } else {
> + 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);
> }
> trace_hpet_ram_read_reading_counter(addr & 4, cur_tick);
> return cur_tick >> shift;
> }
>
> + QEMU_LOCK_GUARD(&s->lock);
> /*address range of all global regs*/
> if (addr <= 0xff) {
> switch (addr) {
> @@ -500,6 +528,11 @@ 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);
> s->config = new_val;
> if (activating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
> /* Enable main counter and interrupt generation. */
> @@ -518,6 +551,13 @@ 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);
> +
> /* i8254 and RTC output pins are disabled
> * when HPET is in legacy mode */
> if (activating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
> --
> 2.47.1
>
--
Peter Xu
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.
I've tried seqlock as well, the difference wrt seqlock is few LOC only
it didn't make HPET code any simpler.
> 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)
> Thanks,
>
> >
> > 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>
> > ---
> > hw/timer/hpet.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 44 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
> > index 97687697c9..d822ca1cd0 100644
> > --- a/hw/timer/hpet.c
> > +++ b/hw/timer/hpet.c
> > @@ -74,6 +74,7 @@ struct HPETState {
> > MemoryRegion iomem;
> > uint64_t hpet_offset;
> > bool hpet_offset_saved;
> > + unsigned state_version;
> > qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
> > uint32_t flags;
> > uint8_t rtc_irq_level;
> > @@ -430,17 +431,44 @@ static uint64_t hpet_ram_read(void *opaque, hwaddr addr,
> > trace_hpet_ram_read(addr);
> > addr &= ~4;
> >
> > - QEMU_LOCK_GUARD(&s->lock);
> > if ((addr <= 0xff) && (addr == HPET_COUNTER)) {
> > - if (hpet_enabled(s)) {
> > - cur_tick = hpet_get_ticks(s);
> > - } else {
> > + 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);
> > }
> > trace_hpet_ram_read_reading_counter(addr & 4, cur_tick);
> > return cur_tick >> shift;
> > }
> >
> > + QEMU_LOCK_GUARD(&s->lock);
> > /*address range of all global regs*/
> > if (addr <= 0xff) {
> > switch (addr) {
> > @@ -500,6 +528,11 @@ 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);
> > s->config = new_val;
> > if (activating_bit(old_val, new_val, HPET_CFG_ENABLE)) {
> > /* Enable main counter and interrupt generation. */
> > @@ -518,6 +551,13 @@ 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);
> > +
> > /* i8254 and RTC output pins are disabled
> > * when HPET is in legacy mode */
> > if (activating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
> > --
> > 2.47.1
> >
>
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
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 */
>
>
On Fri, Aug 01, 2025 at 10:06:45AM +0200, Igor Mammedov wrote: > docs put qatomic_inc() in 'Sequentially consistent' category, > hence no manual barrier. True.. I somehow read it as a _set(). Maybe it's another way to prove memory barriers are error prone, by making a mistake myself. :) Thanks, -- Peter Xu
© 2016 - 2025 Red Hat, Inc.