[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 06/14] migration: Use atomic ops properly for page accounting
From: |
Peter Xu |
Subject: |
Re: [PATCH 06/14] migration: Use atomic ops properly for page accountings |
Date: |
Wed, 5 Oct 2022 09:53:57 -0400 |
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..
--
Peter Xu