Re: [PATCH v5 6/8] add cpu_test_interrupt()/cpu_set_interrupt() helpers and use them tree wide

[Zhao Liu <zhao1.liu@intel.com>]

> > 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.

I see. this makes sense and qatomic_store_release is enough. 

> 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)

I agree (and sorry for my misleading words; my initial thought is
qatomic_fetch_or has worse perfermance.)

Thanks,
Zhao