On Tue, 26 Aug 2025 15:45:32 +0800
Zhao Liu <zhao1....@intel.com> wrote:
> 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.
it looks like we are always holding BQL when setting interrupt.
However currently we also have places that check interrupts
without BQL but without using any atomics. This patch aims to ensure
that proper barriers are in place when checking for interrupts
and introduces release/acquire pair helpers for cpu->interrupt_request,
to ensure it's don consistently.
While overhead might be issue, it's better to have correcteness 1st.
(that's why blanket tree wide change to make sure we don't miss places that
set/test interrupts).
Then if performance issues were found somewhere, as was suggested
in previous reviews, we may opencode that place without barriers
with a mandatory comment/justification why it's okey doing so.
(well, at least that's the plan)
>
> Thanks,
> Zhao
>
