From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: Peter Xu <peterx@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 11/14] migration: Move last_sent_block into PageSearchStatus
Date: Thu, 6 Oct 2022 17:59:51 +0100 [thread overview]
Message-ID: <Yz8Jhyd6b5DscLxr@work-vm> (raw)
In-Reply-To: <20220920225223.49052-1-peterx@redhat.com>
* Peter Xu (peterx@redhat.com) wrote:
> Since we use PageSearchStatus to represent a channel, it makes perfect
> sense to keep last_sent_block (aka, leverage RAM_SAVE_FLAG_CONTINUE) to be
> per-channel rather than global because each channel can be sending
> different pages on ramblocks.
>
> Hence move it from RAMState into PageSearchStatus.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> migration/ram.c | 71 ++++++++++++++++++++++++++++---------------------
> 1 file changed, 41 insertions(+), 30 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index dbe11e1ace..fdcb61a2c8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -89,6 +89,8 @@ XBZRLECacheStats xbzrle_counters;
> struct PageSearchStatus {
> /* The migration channel used for a specific host page */
> QEMUFile *pss_channel;
> + /* Last block from where we have sent data */
> + RAMBlock *last_sent_block;
> /* Current block being searched */
> RAMBlock *block;
> /* Current page to search from */
> @@ -368,8 +370,6 @@ struct RAMState {
> int uffdio_fd;
> /* Last block that we have visited searching for dirty pages */
> RAMBlock *last_seen_block;
> - /* Last block from where we have sent data */
> - RAMBlock *last_sent_block;
> /* Last dirty target page we have sent */
> ram_addr_t last_page;
> /* last ram version we have seen */
> @@ -677,16 +677,17 @@ exit:
> *
> * Returns the number of bytes written
> *
> - * @f: QEMUFile where to send the data
> + * @pss: current PSS channel status
> * @block: block that contains the page we want to send
> * @offset: offset inside the block for the page
> * in the lower bits, it contains flags
> */
> -static size_t save_page_header(RAMState *rs, QEMUFile *f, RAMBlock *block,
> +static size_t save_page_header(PageSearchStatus *pss, RAMBlock *block,
> ram_addr_t offset)
> {
> size_t size, len;
> - bool same_block = (block == rs->last_sent_block);
> + bool same_block = (block == pss->last_sent_block);
> + QEMUFile *f = pss->pss_channel;
>
> if (same_block) {
> offset |= RAM_SAVE_FLAG_CONTINUE;
> @@ -699,7 +700,7 @@ static size_t save_page_header(RAMState *rs, QEMUFile *f, RAMBlock *block,
> qemu_put_byte(f, len);
> qemu_put_buffer(f, (uint8_t *)block->idstr, len);
> size += 1 + len;
> - rs->last_sent_block = block;
> + pss->last_sent_block = block;
> }
> return size;
> }
> @@ -783,17 +784,19 @@ static void xbzrle_cache_zero_page(RAMState *rs, ram_addr_t current_addr)
> * -1 means that xbzrle would be longer than normal
> *
> * @rs: current RAM state
> + * @pss: current PSS channel
> * @current_data: pointer to the address of the page contents
> * @current_addr: addr of the page
> * @block: block that contains the page we want to send
> * @offset: offset inside the block for the page
> */
> -static int save_xbzrle_page(RAMState *rs, QEMUFile *file,
> +static int save_xbzrle_page(RAMState *rs, PageSearchStatus *pss,
> uint8_t **current_data, ram_addr_t current_addr,
> RAMBlock *block, ram_addr_t offset)
> {
> int encoded_len = 0, bytes_xbzrle;
> uint8_t *prev_cached_page;
> + QEMUFile *file = pss->pss_channel;
>
> if (!cache_is_cached(XBZRLE.cache, current_addr,
> ram_counters.dirty_sync_count)) {
> @@ -858,7 +861,7 @@ static int save_xbzrle_page(RAMState *rs, QEMUFile *file,
> }
>
> /* Send XBZRLE based compressed page */
> - bytes_xbzrle = save_page_header(rs, file, block,
> + bytes_xbzrle = save_page_header(pss, block,
> offset | RAM_SAVE_FLAG_XBZRLE);
> qemu_put_byte(file, ENCODING_FLAG_XBZRLE);
> qemu_put_be16(file, encoded_len);
> @@ -1289,19 +1292,19 @@ static void ram_release_page(const char *rbname, uint64_t offset)
> * Returns the size of data written to the file, 0 means the page is not
> * a zero page
> *
> - * @rs: current RAM state
> - * @file: the file where the data is saved
> + * @pss: current PSS channel
> * @block: block that contains the page we want to send
> * @offset: offset inside the block for the page
> */
> -static int save_zero_page_to_file(RAMState *rs, QEMUFile *file,
> +static int save_zero_page_to_file(PageSearchStatus *pss,
> RAMBlock *block, ram_addr_t offset)
> {
> uint8_t *p = block->host + offset;
> + QEMUFile *file = pss->pss_channel;
> int len = 0;
>
> if (buffer_is_zero(p, TARGET_PAGE_SIZE)) {
> - len += save_page_header(rs, file, block, offset | RAM_SAVE_FLAG_ZERO);
> + len += save_page_header(pss, block, offset | RAM_SAVE_FLAG_ZERO);
> qemu_put_byte(file, 0);
> len += 1;
> ram_release_page(block->idstr, offset);
> @@ -1314,14 +1317,14 @@ static int save_zero_page_to_file(RAMState *rs, QEMUFile *file,
> *
> * Returns the number of pages written.
> *
> - * @rs: current RAM state
> + * @pss: current PSS channel
> * @block: block that contains the page we want to send
> * @offset: offset inside the block for the page
> */
> -static int save_zero_page(RAMState *rs, QEMUFile *file, RAMBlock *block,
> +static int save_zero_page(PageSearchStatus *pss, RAMBlock *block,
> ram_addr_t offset)
> {
> - int len = save_zero_page_to_file(rs, file, block, offset);
> + int len = save_zero_page_to_file(pss, block, offset);
>
> if (len) {
> qatomic_inc(&ram_counters.duplicate);
> @@ -1374,16 +1377,18 @@ static bool control_save_page(PageSearchStatus *pss, RAMBlock *block,
> *
> * Returns the number of pages written.
> *
> - * @rs: current RAM state
> + * @pss: current PSS channel
> * @block: block that contains the page we want to send
> * @offset: offset inside the block for the page
> * @buf: the page to be sent
> * @async: send to page asyncly
> */
> -static int save_normal_page(RAMState *rs, QEMUFile *file, RAMBlock *block,
> +static int save_normal_page(PageSearchStatus *pss, RAMBlock *block,
> ram_addr_t offset, uint8_t *buf, bool async)
> {
> - ram_transferred_add(save_page_header(rs, file, block,
> + QEMUFile *file = pss->pss_channel;
> +
> + ram_transferred_add(save_page_header(pss, block,
> offset | RAM_SAVE_FLAG_PAGE));
> if (async) {
> qemu_put_buffer_async(file, buf, TARGET_PAGE_SIZE,
> @@ -1423,7 +1428,7 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss)
>
> XBZRLE_cache_lock();
> if (rs->xbzrle_enabled && !migration_in_postcopy()) {
> - pages = save_xbzrle_page(rs, pss->pss_channel, &p, current_addr,
> + pages = save_xbzrle_page(rs, pss, &p, current_addr,
> block, offset);
> if (!rs->last_stage) {
> /* Can't send this cached data async, since the cache page
> @@ -1435,8 +1440,7 @@ static int ram_save_page(RAMState *rs, PageSearchStatus *pss)
>
> /* XBZRLE overflow or normal page */
> if (pages == -1) {
> - pages = save_normal_page(rs, pss->pss_channel, block, offset,
> - p, send_async);
> + pages = save_normal_page(pss, block, offset, p, send_async);
> }
>
> XBZRLE_cache_unlock();
> @@ -1459,14 +1463,15 @@ static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
> ram_addr_t offset, uint8_t *source_buf)
> {
> RAMState *rs = ram_state;
> + PageSearchStatus *pss = &rs->pss[RAM_CHANNEL_PRECOPY];
> uint8_t *p = block->host + offset;
> int ret;
>
> - if (save_zero_page_to_file(rs, f, block, offset)) {
> + if (save_zero_page_to_file(pss, block, offset)) {
> return true;
> }
>
> - save_page_header(rs, f, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE);
> + save_page_header(pss, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE);
>
> /*
> * copy it to a internal buffer to avoid it being modified by VM
> @@ -2286,7 +2291,8 @@ static bool save_page_use_compression(RAMState *rs)
> * has been properly handled by compression, otherwise needs other
> * paths to handle it
> */
> -static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
> +static bool save_compress_page(RAMState *rs, PageSearchStatus *pss,
> + RAMBlock *block, ram_addr_t offset)
> {
> if (!save_page_use_compression(rs)) {
> return false;
> @@ -2302,7 +2308,7 @@ static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
> * We post the fist page as normal page as compression will take
> * much CPU resource.
> */
> - if (block != rs->last_sent_block) {
> + if (block != pss->last_sent_block) {
> flush_compressed_data(rs);
> return false;
> }
> @@ -2333,11 +2339,11 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss)
> return res;
> }
>
> - if (save_compress_page(rs, block, offset)) {
> + if (save_compress_page(rs, pss, block, offset)) {
> return 1;
> }
>
> - res = save_zero_page(rs, pss->pss_channel, block, offset);
> + res = save_zero_page(pss, block, offset);
> if (res > 0) {
> /* Must let xbzrle know, otherwise a previous (now 0'd) cached
> * page would be stale
> @@ -2469,7 +2475,7 @@ static void postcopy_preempt_choose_channel(RAMState *rs, PageSearchStatus *pss)
> * If channel switched, reset last_sent_block since the old sent block
> * may not be on the same channel.
> */
> - rs->last_sent_block = NULL;
> + pss->last_sent_block = NULL;
>
> trace_postcopy_preempt_switch_channel(channel);
> }
> @@ -2804,8 +2810,13 @@ static void ram_save_cleanup(void *opaque)
>
> static void ram_state_reset(RAMState *rs)
> {
> + int i;
> +
> + for (i = 0; i < RAM_CHANNEL_MAX; i++) {
> + rs->pss[i].last_sent_block = NULL;
> + }
> +
> rs->last_seen_block = NULL;
> - rs->last_sent_block = NULL;
> rs->last_page = 0;
> rs->last_version = ram_list.version;
> rs->xbzrle_enabled = false;
> @@ -2999,8 +3010,8 @@ void ram_postcopy_send_discard_bitmap(MigrationState *ms)
> migration_bitmap_sync(rs);
>
> /* Easiest way to make sure we don't resume in the middle of a host-page */
> + rs->pss[RAM_CHANNEL_PRECOPY].last_sent_block = NULL;
Why don't we reset the postcopy one here as well?
Dave
> rs->last_seen_block = NULL;
> - rs->last_sent_block = NULL;
> rs->last_page = 0;
>
> postcopy_each_ram_send_discard(ms);
> --
> 2.32.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
next prev parent reply other threads:[~2022-10-06 17:43 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
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 [this message]
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=Yz8Jhyd6b5DscLxr@work-vm \
--to=dgilbert@redhat.com \
--cc=ani@anisinha.ca \
--cc=berrange@redhat.com \
--cc=lsoaresp@redhat.com \
--cc=manish.mishra@nutanix.com \
--cc=peterx@redhat.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).