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