Re: [PATCH] migration: RDMA registrations interval optimization

[Juan Quintela]

Zhiwei Jiang <elish.jiang@ucloud.cn> wrote:
> RDMA migration very hard to complete when VM run mysql
> benchmark on 1G host hugepage.I think the time between
> ram_control_before_iterate(f, RAM_CONTROL_ROUND) and
> after_iterate is too large when 1G host pagesize,so 1M
> buffer size match with mlx driver that will be good.
> after this patch,it will work as normal on my situation.
>
> Signed-off-by: Zhiwei Jiang <elish.jiang@ucloud.cn>

Hi

Nice catch.

Could you split the migrate_use_rdma() part (which is obvious).
I can't believe that wedon't have that function. And the rest.

See my comments there.

> diff --git a/migration/migration.c b/migration/migration.c
> index 041b8451a6..934916b161 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -457,6 +457,8 @@ void migrate_add_address(SocketAddress *address)
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p = NULL;
> +    MigrationState *s = migrate_get_current();
> +    s->enabled_rdma_migration = false;

This shouldn't be needed (famous last words).  If it is needed, we are
masking other problem that we need to fix, like not initialzing the
migration state properly.
> +    /*
> +     * Enable RDMA migration
> +     */
> +    bool enabled_rdma_migration;


I think that ""rdma_enabled" is enough, what do you think?

I think that the comment is not needed.

> diff --git a/migration/ram.c b/migration/ram.c
> index 7a43bfd7af..dc0c0e2565 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2043,7 +2043,11 @@ static int ram_save_host_page(RAMState *rs, 
> PageSearchStatus *pss,
>          qemu_ram_pagesize(pss->block) >> TARGET_PAGE_BITS;
>      unsigned long hostpage_boundary =
>          QEMU_ALIGN_UP(pss->page + 1, pagesize_bits);
> +    /* Set RDMA boundary default 256*4K=1M that driver delivery more 
> effective*/
> +    unsigned long rdma_boundary =
> +        QEMU_ALIGN_UP(pss->page + 1, 256);
>      unsigned long start_page = pss->page;
> +    bool use_rdma = migrate_use_rdma();
>      int res;

To not have to do the comparison everytime, what about:

       unsigned long boundary = migrate_use_rdma() ? rdma_boundary : 
hostpage_boundary;

>      if (ramblock_is_ignored(pss->block)) {
> @@ -2069,7 +2073,7 @@ static int ram_save_host_page(RAMState *rs, 
> PageSearchStatus *pss,
>              }
>          }
>          pss->page = migration_bitmap_find_dirty(rs, pss->block, pss->page);
> -    } while ((pss->page < hostpage_boundary) &&
> +    } while ((pss->page < (use_rdma ? rdma_boundary : hostpage_boundary)) &&
>               offset_in_ramblock(pss->block,
>                                  ((ram_addr_t)pss->page) <<
> TARGET_PAGE_BITS));

    } while ((pss->page < boundary) &&
              offset_in_ramblock(pss->block,
                                 ((ram_addr_t)pss->page) << TARGET_PAGE_BITS));

We remove one comparison in every round.

What do you think?

Later, Juan.

PD. Nice acchievement being able to migrate with a relative high load
    using 1GB pages.  Could you share your setup and how fast it goes.
    Thanks.