On Mon, Aug 27, 2012 at 04:47:13PM -0300, Rafael Aquini wrote: > On Sun, Aug 26, 2012 at 10:42:44AM +0300, Michael S. Tsirkin wrote: > > > > Reading two atomics and doing math? Result can even be negative. > > I did not look at use closely but it looks suspicious. > Doc on atomic_read says: > " > The read is atomic in that the return value is guaranteed to be one of the > values initialized or modified with the interface operations if a proper > implicit or explicit memory barrier is used after possible runtime > initialization by any other thread and the value is modified only with the > interface operations. > " > > There's no runtime init by other thread than balloon's itself at device register, > and the operations (inc, dec) are made by the proper interface operations > only when protected by the spinlock pages_lock. It does not look suspicious, IMHO. Any use of multiple atomics is suspicious. Please just avoid it if you can. What's wrong with locking? > I'm failing to see how it could become a negative on that case, since you cannot > isolate more pages than what was previoulsy inflated to balloon's list. There is no order guarantee. So in A - B you can read B long after both A and B has been incremented. Maybe it is safe in this case but it needs careful documentation to explain how ordering works. Much easier to keep it all simple. > > > It's already the case everywhere except __wait_on_isolated_pages, > > so just fix that, and then we can keep using int instead of atomics. > > > Sorry, I quite didn't get you here. fix what? It's in the text you removed above. Access values under lock. > > > That's 1K on stack - and can become more if we increase > > VIRTIO_BALLOON_ARRAY_PFNS_MAX. Probably too much - this is the reason > > we use vb->pfns. > > > If we want to use vb->pfns we'll have to make leak_balloon mutual exclusive with > page migration (as it was before), but that will inevictably bring us back to > the discussion on breaking the loop when isolated pages make leak_balloon find > less pages than it wants to release at each leak round. > I don't think this is an issue. The issue was busy waiting in that case. -- MST _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
