On Fri, 22 May 2020 at 22:33, Robert Foley <robert.fo...@linaro.org> wrote: > On Fri, 22 May 2020 at 13:44, Peter Maydell <peter.mayd...@linaro.org> wrote: > > Every target's has_work function seems to access > > cs->interrupt_request without using atomic_read() : > > why does Arm need to do something special here? > > > > More generally, the only place that currently > > uses atomic_read() on the interrupt_request field > > is cpu_handle_interrupt(), so if this field needs > > special precautions to access then a lot of code > > needs updating. > > TSan flagged this case as a potential data race. It does not mean > necessarily that there is an issue here, just that two threads were > accessing the data > without TSan detecting the synchronization. TSan gives a few options > to silence the > warning, such as changing the locking, making it atomic, or adding > various types > of annotations to tell TSan to ignore it. So in this case we had a > few options, such as > to change it to atomic or to simply annotate it and silence it. > > We started our TSan testing using arm, and have been working to iron out the > TSan warnings there, and there alone initially. Assuming that we are OK > with making this particular change, to silence the TSan warning, > then certainly it is a good point that we need to consider changing the > other places that access this field, since they will all see similar > TSan warnings.
So is this: (a) a TSan false positive, because we've analysed the use of this struct field and know it's not a race because [details], but which we're choosing to silence in this way (b) an actual race for which the correct fix is to make the accesses atomic because [details] ? Either way, the important part is the analysis which fills in the "[details]" part, which should be in the commit message... thanks -- PMM