Re: [PATCH 06/14] migration: Use atomic ops properly for page accountings

[Peter Xu <peterx@redhat.com>]

On Wed, Oct 05, 2022 at 09:53:57AM -0400, Peter Xu wrote:
> On Wed, Oct 05, 2022 at 12:38:05PM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > On Tue, Oct 04, 2022 at 05:59:36PM +0100, Dr. David Alan Gilbert wrote:
> > > > * Peter Xu (peterx@redhat.com) 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 <peterx@redhat.com>
> > > > 
> > > > 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..

I think I can't directly change the qapi MigrationStats to make some of
them to Stat64 since for !ATOMIC_64 systems Stat64 actually takes more than
64 bits space (since we'll need to do the locking with Stat64.lock), so
it'll definitely break the ABI no matter what..

I don't have a better option but introduce another ram_counters_internal to
maintain the fields that need atomic access, declaring as a Stat64 array.
Then we only mirror those values to MigrationStats in QMP queries when
needed.  The mirror will not contain the lock itself so it'll keep the ABI.

Let me know if there's early comment for that, or I'll go with it.  I'll
definitely add some comment for ram_counters to explain the mirror counters
in that case.

Thanks,

-- 
Peter Xu