On Wed, Oct 05, 2022 at 12:38:05PM +0100, Dr. David Alan Gilbert wrote: > * Peter Xu ([email protected]) wrote: > > On Tue, Oct 04, 2022 at 05:59:36PM +0100, Dr. David Alan Gilbert wrote: > > > * Peter Xu ([email protected]) wrote: > > > > To prepare for thread-safety on page accountings, at least below > > > > counters > > > > need to be accessed only atomically, they are: > > > > > > > > ram_counters.transferred > > > > ram_counters.duplicate > > > > ram_counters.normal > > > > ram_counters.postcopy_bytes > > > > > > > > There are a lot of other counters but they won't be accessed outside > > > > migration thread, then they're still safe to be accessed without atomic > > > > ops. > > > > > > > > Signed-off-by: Peter Xu <[email protected]> > > > > > > I think this is OK; I'm not sure whether the memset 0's of ram_counters > > > technically need changing. > > > > IMHO they're fine - what we need there should be thing like WRITE_ONCE() > > just to make sure no register caches (actually atomic_write() is normally > > implemented with WRITE_ONCE afaik). But I think that's already guaranteed > > by memset() as the function call does, so we should be 100% safe. > > I agree you're probably OK. > > > > I'd love to put a comment somewhere saying these fields need to be > > > atomically read, but their qapi defined so I don't think we can. > > > > How about I add a comment above ram_counters declarations in ram.c? > > Yeh. > > > > > > > Finally, we probably need to check these are happy on 32 bit builds, > > > sometimes it's a bit funny with atomic adds. > > > > Yeah.. I hope using qatomic_*() APIs can help me avoid any issues. Or > > anything concerning? I'd be happy to test on specific things if there are. > > I just remember hitting problems in the past; especially if we end up > with trying to do a 64 bit atomic on a platofmr that can only do 32???
I see what you meant... when I was looking in the existing callers of qatomic_add(), I do find that we seem to have Stat64 just for that !CONFIG_ATOMIC64 problem. I'll dig a bit on whether and how we can do that; the thing is these counters are in the qapi so I need to make sure it can support Stat64 somehow. Hmm.. -- Peter Xu
