All of lore.kernel.org
 help / color / mirror / Atom feed
From: Juan Quintela <quintela@redhat.com>
To: "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com>
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 1/1] Count used RAMBlock pages for migration_dirty_pages
Date: Fri, 21 Mar 2014 14:11:54 +0100	[thread overview]
Message-ID: <8761n7itlx.fsf@elfo.mitica> (raw)
In-Reply-To: <1395399490-13295-1-git-send-email-dgilbert@redhat.com> (David Alan Gilbert's message of "Fri, 21 Mar 2014 10:58:10 +0000")

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> This is a fix for a bug* triggered by a migration after hot unplugging
> a few virtio-net NICs, that caused migration never to converge, because
> 'migration_dirty_pages' is incorrectly initialised.

Good catch.

> 'migration_dirty_pages' is used as a tally of the number of outstanding
> dirty pages, to give the migration code an idea of how much more data
> will need to be transferred, and thus whether it can end the iterative
> phase.
>
> It was initialised to the total size of the RAMBlock address space,
> however hotunplug can leave this space sparse, and hence
> migration_dirty_pages ended up too large.
>
> Note that the code tries to be careful when counting to deal with
> RAMBlocks that share the same end/start page - I don't know
> if this is actually possible and it does complicate the code,
> but since there was other code that dealt with unaligned RAMBlocks
> it seemed possible.

Couldn't we just check at block addition that it dont' overlap?

What code do you mean?

My understanding is that the "normal" way of creating new RAMBlocks is
with qemu_ram_alloc_from_ptr(), and my reading is that block never
overlap.  (Important words of the sentence: "my reading").

>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> (* https://bugzilla.redhat.com/show_bug.cgi?id=1074913 )
> ---
>  arch_init.c | 41 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index f18f42e..ef0e98d 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -727,11 +727,8 @@ static void reset_ram_globals(void)
>  static int ram_save_setup(QEMUFile *f, void *opaque)
>  {
>      RAMBlock *block;
> -    int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> +    int64_t ram_bitmap_pages;
>  
> -    migration_bitmap = bitmap_new(ram_pages);
> -    bitmap_set(migration_bitmap, 0, ram_pages);
> -    migration_dirty_pages = ram_pages;
>      mig_throttle_on = false;
>      dirty_rate_high_cnt = 0;
>  
> @@ -770,6 +767,42 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      bytes_transferred = 0;
>      reset_ram_globals();
>  
> +    ram_bitmap_pages = last_ram_offset() >> TARGET_PAGE_BITS;
> +    migration_bitmap = bitmap_new(ram_bitmap_pages);
> +    bitmap_set(migration_bitmap, 0, ram_bitmap_pages);
> +    /*
> +     * Count the total number of pages used by ram blocks. We clear the dirty
> +     * bit for the start/end of each ramblock as we go so that we don't double
> +     * count ramblocks that have overlapping pages - at entry the whole dirty
> +     * bitmap is set.
> +     */
> +    migration_dirty_pages = 0;
> +    QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> +        uint64_t block_pages = 0;
> +        ram_addr_t saddr, eaddr;
> +
> +        saddr = block->mr->ram_addr;
> +        eaddr = saddr + block->length - 1;

If my assumtpion is true:  block->lenght-1 / TARGET_PAGE_SIZE (rounded
up) should be enough, no?

Reason for this is that migration bitmap handling is already slow, and
we are adding a whole two passes here?

Later, Juan.

  reply	other threads:[~2014-03-21 13:12 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-03-21 10:58 [Qemu-devel] [PATCH 1/1] Count used RAMBlock pages for migration_dirty_pages Dr. David Alan Gilbert (git)
2014-03-21 13:11 ` Juan Quintela [this message]
2014-03-21 13:22   ` Dr. David Alan Gilbert
2014-03-21 15:15     ` Paolo Bonzini
2014-03-21 15:45       ` Dr. David Alan Gilbert
2014-03-21 15:49         ` Paolo Bonzini
2014-03-21 16:08           ` Juan Quintela
2014-03-21 13:44 ` 陈梁
2014-03-21 14:41   ` 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=8761n7itlx.fsf@elfo.mitica \
    --to=quintela@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /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.