On Mon, Aug 25, 2025 at 05:19:12PM +0200, Igor Mammedov wrote:
> Date: Mon, 25 Aug 2025 17:19:12 +0200
> From: Igor Mammedov <imamm...@redhat.com>
> Subject: Re: [PATCH v5 6/8] add cpu_test_interrupt()/cpu_set_interrupt()
> helpers and use them tree wide
> X-Mailer: Claws Mail 4.3.1 (GTK 3.24.49; x86_64-redhat-linux-gnu)
>
> On Mon, 25 Aug 2025 23:28:22 +0800
> Zhao Liu <zhao1....@intel.com> wrote:
>
> > Hi Igor,
> >
> > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
> > > index 5eaf41a566..1dee9d4c76 100644
> > > --- a/include/hw/core/cpu.h
> > > +++ b/include/hw/core/cpu.h
> > > @@ -942,6 +942,31 @@ CPUState *cpu_by_arch_id(int64_t id);
> > >
> > > void cpu_interrupt(CPUState *cpu, int mask);
> > >
> > > +/**
> > > + * cpu_test_interrupt:
> > > + * @cpu: The CPU to check interrupt(s) on.
> > > + * @mask: The interrupts to check.
> > > + *
> > > + * Checks if any of interrupts in @mask are pending on @cpu.
> > > + */
> > > +static inline bool cpu_test_interrupt(CPUState *cpu, int mask)
> > > +{
> > > + return qatomic_load_acquire(&cpu->interrupt_request) & mask;
> > > +}
> > > +
> > > +/**
> > > + * cpu_set_interrupt:
> > > + * @cpu: The CPU to set pending interrupt(s) on.
> > > + * @mask: The interrupts to set.
> > > + *
> > > + * Sets interrupts in @mask as pending on @cpu.
> > > + */
> > > +static inline void cpu_set_interrupt(CPUState *cpu, int mask)
> > > +{
> > > + qatomic_store_release(&cpu->interrupt_request,
> > > + cpu->interrupt_request | mask);
> >
> > It seems the read access of cpu->interrupt_request is not atomic, should
> > we also protect it by qatomic_read(cpu->interrupt_request)? like
> >
> > qatomic_store_release(&cpu->interrupt_request,
> > qatomic_read(cpu->interrupt_request) | mask)
>
> it's not necessary according to doc:
>
> - ``qatomic_store_release()``, which guarantees the STORE to appear to
>
> happen, ...,
> after all the LOAD or STORE operations specified before.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> that includes 'cpu->interrupt_request | mask' part
Yes, thanks for your explaination and patience.
> >
> > or futher,
> >
> > qatomic_fetch_or(&cpu->interrupt_request, mask)
> that would work as well but it also could be more expensive than
> qatomic_store_release()
Behind this helper, I mainly considerred the case of multiple writers:
thread 0 . thread 1
.
load: x .
OR: x | a .
.
. load: x
. OR: x | b
. store: x | b
.
store: x | a . (x | b is missed)
In the above case, "load" means the direct access:
cpu->interrupt_request w/o protection, and "store" is done by
qatomic_store_release.
The memory order is guaranteed, but the operation result of thread 1
seems lost. Only BQL or other mutex could avoid such case.
qatomic_store_release is already a great step to avoid issues outside
BQL, so I'm not sure if it's worth going further to ensure atomicity,
especifically for multiple writers (my initial understanding is that
iothread or callback may have multiple writers, but I'm also a bit
unsure.). The overhead is also indeed an issue.
Thanks,
Zhao
