On Mon, Dec 16, 2019 at 10:33:42AM -0500, Michael S. Tsirkin wrote: > On Mon, Dec 16, 2019 at 10:07:54AM -0500, Peter Xu wrote: > > On Mon, Dec 16, 2019 at 04:47:36AM -0500, Michael S. Tsirkin wrote: > > > On Sun, Dec 15, 2019 at 12:33:02PM -0500, Peter Xu wrote: > > > > On Thu, Dec 12, 2019 at 01:08:14AM +0100, Paolo Bonzini wrote: > > > > > >>> What depends on what here? Looks suspicious ... > > > > > >> > > > > > >> Hmm, I think maybe it can be removed because the entry pointer > > > > > >> reference below should be an ordering constraint already? > > > > > > > > > > entry->xxx depends on ring->reset_index. > > > > > > > > Yes that's true, but... > > > > > > > > entry = &ring->dirty_gfns[ring->reset_index & (ring->size - 1)]; > > > > /* barrier? */ > > > > next_slot = READ_ONCE(entry->slot); > > > > next_offset = READ_ONCE(entry->offset); > > > > > > > > ... I think entry->xxx depends on entry first, then entry depends on > > > > reset_index. So it seems fine because all things have a dependency? > > > > > > Is reset_index changed from another thread then? > > > If yes then you want to read reset_index with READ_ONCE. > > > That includes a dependency barrier. > > > > There're a few readers, but only this function will change it > > (kvm_dirty_ring_reset). Thanks, > > Then you don't need any barriers in this function. > readers need at least READ_ONCE. In our case even an old reset_index should not matter much here imho because the worst case is we read an old reset so we stop pushing to a ring when it's just being reset and at the same time it's soft-full (so an extra user exit even race happened). But I agree it's clearer to READ_ONCE() on readers. Thanks! -- Peter Xu
