All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Xu <peterx@redhat.com>
To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: qemu-devel@nongnu.org, Manish Mishra <manish.mishra@nutanix.com>,
	Juan Quintela <quintela@redhat.com>,
	ani@anisinha.ca,
	Leonardo Bras Soares Passos <lsoaresp@redhat.com>,
	"Daniel P . Berrange" <berrange@redhat.com>
Subject: Re: [PATCH 06/14] migration: Use atomic ops properly for page accountings
Date: Tue, 4 Oct 2022 15:23:58 -0400	[thread overview]
Message-ID: <YzyITnjOH63AbK2h@x1n> (raw)
In-Reply-To: <YzxmeMMEq52IDGit@work-vm>

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'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?

> 
> 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.

> 
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Thanks!

-- 
Peter Xu



  reply	other threads:[~2022-10-04 19:28 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-20 22:50 [PATCH 00/14] migration: Postcopy Preempt-Full Peter Xu
2022-09-20 22:50 ` [PATCH 01/14] migration: Add postcopy_preempt_active() Peter Xu
2022-09-20 22:50 ` [PATCH 02/14] migration: Cleanup xbzrle zero page cache update logic Peter Xu
2022-10-04 10:33   ` Dr. David Alan Gilbert
2022-09-20 22:50 ` [PATCH 03/14] migration: Trivial cleanup save_page_header() on same block check Peter Xu
2022-10-04 10:41   ` Dr. David Alan Gilbert
2022-09-20 22:50 ` [PATCH 04/14] migration: Remove RAMState.f references in compression code Peter Xu
2022-10-04 10:54   ` Dr. David Alan Gilbert
2022-10-04 14:36     ` Peter Xu
2022-09-20 22:52 ` [PATCH 05/14] migration: Yield bitmap_mutex properly when sending/sleeping Peter Xu
2022-10-04 13:55   ` Dr. David Alan Gilbert
2022-10-04 19:13     ` Peter Xu
2022-10-05 11:18       ` Dr. David Alan Gilbert
2022-10-05 13:40         ` Peter Xu
2022-10-05 19:48           ` Peter Xu
2022-09-20 22:52 ` [PATCH 06/14] migration: Use atomic ops properly for page accountings Peter Xu
2022-10-04 16:59   ` Dr. David Alan Gilbert
2022-10-04 19:23     ` Peter Xu [this message]
2022-10-05 11:38       ` Dr. David Alan Gilbert
2022-10-05 13:53         ` Peter Xu
2022-10-06 20:40           ` Peter Xu
2022-09-20 22:52 ` [PATCH 07/14] migration: Teach PSS about host page Peter Xu
2022-10-05 11:12   ` Dr. David Alan Gilbert
2022-09-20 22:52 ` [PATCH 08/14] migration: Introduce pss_channel Peter Xu
2022-10-05 13:03   ` Dr. David Alan Gilbert
2022-09-20 22:52 ` [PATCH 09/14] migration: Add pss_init() Peter Xu
2022-10-05 13:09   ` Dr. David Alan Gilbert
2022-09-20 22:52 ` [PATCH 10/14] migration: Make PageSearchStatus part of RAMState Peter Xu
2022-10-05 18:51   ` Dr. David Alan Gilbert
2022-10-05 19:41     ` Peter Xu
2022-10-06  8:36       ` Dr. David Alan Gilbert
2022-10-06  8:37   ` Dr. David Alan Gilbert
2022-09-20 22:52 ` [PATCH 11/14] migration: Move last_sent_block into PageSearchStatus Peter Xu
2022-10-06 16:59   ` Dr. David Alan Gilbert
2022-10-06 18:34     ` Peter Xu
2022-10-06 18:38       ` Dr. David Alan Gilbert
2022-09-20 22:52 ` [PATCH 12/14] migration: Send requested page directly in rp-return thread Peter Xu
2022-10-06 17:51   ` Dr. David Alan Gilbert
2022-09-20 22:52 ` [PATCH 13/14] migration: Remove old preempt code around state maintainance Peter Xu
2022-09-21  0:47   ` Peter Xu
2022-09-21 13:54     ` Peter Xu
2022-10-06 17:56       ` Dr. David Alan Gilbert
2022-09-20 22:52 ` [PATCH 14/14] migration: Drop rs->f Peter Xu
2022-10-06 17:57   ` Dr. David Alan Gilbert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YzyITnjOH63AbK2h@x1n \
    --to=peterx@redhat.com \
    --cc=ani@anisinha.ca \
    --cc=berrange@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=lsoaresp@redhat.com \
    --cc=manish.mishra@nutanix.com \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.