Re: [PATCH] migration/rdma: add x-rdma-chunk-shift parameter

[Markus Armbruster <armbru@redhat.com>]

I just realized you neglected to cc: migration maintainers.  I'm fixing
that for you.  You can use scripts/get_maintainer *patch to find
maintainers you might want to cc:.

"Zhang, GuoQing (Sam)" <guoqzhan@amd.com> writes:

> Hi Markus,
>
> Thank you for your feedback. Please see my replies inline.
>
> Regards
> Sam
>
>
> On 2026/3/19 16:48, Markus Armbruster wrote:

[...]

>> Samuel Zhang<guoqing.zhang@amd.com> writes:
>>
>>> The default 1MB RDMA chunk size causes slow live migration because
>>> each chunk triggers a write_flush (ibv_post_send). For 8GB RAM,
>>> 1MB chunks produce ~15000 flushes vs ~3700 with 1GB chunks.
>>>
>>> Add x-rdma-chunk-shift parameter to configure the RDMA chunk size
>>> (2^N bytes) for faster migration.
>>> Usage: -global migration.x-rdma-chunk-shift=30

Does monitor command migrate-set-parameters work?  Testing...  it's
accepted:

    { "execute": "migrate-set-parameters", "arguments": { "x-rdma-chunk-shift": 30 } }
    {"return": {}}

Didn't test it actually works.

However, query-migrate-parameters does not show the new parameter.
Needs fixing.  See migrate_mark_all_params_present().

Also missing: update to hmp_migrate_set_parameter() and
hmp_info_migrate_parameters().

Friends don't let friends use -global unless there is no other way.

>>> Performance with RDMA live migration of 8GB RAM VM:
>>>
>>> | x-rdma-chunk-shift | chunk size | time (s) | throughput (Mbps) |
>>> |--------------------|------------|----------|-------------------|
>>> | 20 (default)       | 1 MB       | 37.915   | 1,007             |
>>> | 25                 | 32 MB      | 17.880   | 2,260             |
>>> | 30                 | 1 GB       |  4.368   | 17,529            |
>>>
>>> Signed-off-by: Samuel Zhang<guoqing.zhang@amd.com>
>>> ---
>>>  migration/options.c | 13 +++++++++++++
>>>  migration/options.h |  1 +
>>>  migration/rdma.c    | 37 ++++++++++++++++++++++---------------
>>>  qapi/migration.json |  9 ++++++++-
>>>  4 files changed, 44 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/migration/options.c b/migration/options.c
>>> index f33b297929..1503ae35a2 100644
>>> --- a/migration/options.c
>>> +++ b/migration/options.c
>>> @@ -90,6 +90,7 @@ const PropertyInfo qdev_prop_StrOrNull;
>>>
>>>  #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD     1000    /* milliseconds */
>>>  #define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT            1       /* MB/s */
>>> +#define DEFAULT_MIGRATE_X_RDMA_CHUNK_SHIFT          20      /* 1MB */
>>>
>>>  const Property migration_properties[] = {
>>>      DEFINE_PROP_BOOL("store-global-state", MigrationState,
>>> @@ -183,6 +184,9 @@ const Property migration_properties[] = {
>>>      DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState,
>>>                         parameters.zero_page_detection,
>>>                         ZERO_PAGE_DETECTION_MULTIFD),
>>> +    DEFINE_PROP_UINT8("x-rdma-chunk-shift", MigrationState,
>>> +                      parameters.x_rdma_chunk_shift,
>>> +                      DEFAULT_MIGRATE_X_RDMA_CHUNK_SHIFT),
>>>
>>>      /* Migration capabilities */
>>>      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
>>> @@ -993,6 +997,15 @@ ZeroPageDetection migrate_zero_page_detection(void)
>>>      return s->parameters.zero_page_detection;
>>>  }
>>>
>>> +uint8_t migrate_rdma_chunk_shift(void)
>>> +{
>>> +    MigrationState *s = migrate_get_current();
>>> +    uint8_t chunk_shift = s->parameters.x_rdma_chunk_shift;
>>> +
>>> +    assert(20 <= chunk_shift && chunk_shift <= 30);
>>
>> Where is this ensured?
>
> It is ensured just here.

So, invalid configuration will be accepted, the VM starts, and when you
try to migrate it, it crashes.  Potential data loss.

Crashing on bad input is always wrong.

Always, always, always test with bad input, too.

I caught in in review this time, but it could've slipped through easily.
Dangerous.

> The chunk_shift is provided by user when starting qemu process.
>
> `-global migration.x-rdma-chunk-shift=30`
>
> Valid range is [20, 30], if user provided an invalid value, assert will fail and qemu will exit with error logs.
>
> I don't know any better places to validate the value. Any suggestions on this? Thank you!

It's perfectly fine not to know things!  Just ask then.

@x-rdma-chunk-shift is a member of MigrationParameters.  The other
members are validated migrate_params_check().  Try doing the same for
@x-rdma-chunk-shift.

>>> +    return chunk_shift;
>>> +}
>>> +
>>>  /* parameters helpers */
>>>
>>>  AnnounceParameters *migrate_announce_params(void)
>>> diff --git a/migration/options.h b/migration/options.h
>>> index b502871097..3f214465a3 100644
>>> --- a/migration/options.h
>>> +++ b/migration/options.h
>>> @@ -87,6 +87,7 @@ const char *migrate_tls_creds(void);
>>>  const char *migrate_tls_hostname(void);
>>>  uint64_t migrate_xbzrle_cache_size(void);
>>>  ZeroPageDetection migrate_zero_page_detection(void);
>>> +uint8_t migrate_rdma_chunk_shift(void);
>>>
>>>  /* parameters helpers */
>>>
>>> diff --git a/migration/rdma.c b/migration/rdma.c
>>> index 55ab85650a..d914a7cd3b 100644
>>> --- a/migration/rdma.c
>>> +++ b/migration/rdma.c
>>> @@ -44,11 +44,18 @@
>>>
>>>  #define RDMA_RESOLVE_TIMEOUT_MS 10000
>>>
>>> -/* Do not merge data if larger than this. */
>>> -#define RDMA_MERGE_MAX (2 * 1024 * 1024)
>>> -#define RDMA_SIGNALED_SEND_MAX (RDMA_MERGE_MAX / 4096)
>>> +#define RDMA_SIGNALED_SEND_MAX 512
>>> +
>>> +static inline uint64_t rdma_chunk_size(void)
>>> +{
>>> +    return 1UL << migrate_rdma_chunk_shift();
>>> +}
>>>
>>> -#define RDMA_REG_CHUNK_SHIFT 20 /* 1 MB */
>>> +/* Do not merge data if larger than this. */
>>> +static inline uint64_t rdma_merge_max(void)
>>> +{
>>> +    return rdma_chunk_size() * 2;
>>> +}
>>>
>>>  /*
>>>   * This is only for non-live state being migrated.
>>> @@ -527,21 +534,21 @@ static int qemu_rdma_exchange_send(RDMAContext *rdma, RDMAControlHeader *head,
>>>  static inline uint64_t ram_chunk_index(const uint8_t *start,
>>>                                         const uint8_t *host)
>>>  {
>>> -    return ((uintptr_t) host - (uintptr_t) start) >> RDMA_REG_CHUNK_SHIFT;
>>> +    return ((uintptr_t) host - (uintptr_t) start) >> migrate_rdma_chunk_shift();
>>>  }
>>>
>>>  static inline uint8_t *ram_chunk_start(const RDMALocalBlock *rdma_ram_block,
>>>                                         uint64_t i)
>>>  {
>>>      return (uint8_t *)(uintptr_t)(rdma_ram_block->local_host_addr +
>>> -                                  (i << RDMA_REG_CHUNK_SHIFT));
>>> +                                  (i << migrate_rdma_chunk_shift()));
>>>  }
>>>
>>>  static inline uint8_t *ram_chunk_end(const RDMALocalBlock *rdma_ram_block,
>>>                                       uint64_t i)
>>>  {
>>>      uint8_t *result = ram_chunk_start(rdma_ram_block, i) +
>>> -                                         (1UL << RDMA_REG_CHUNK_SHIFT);
>>> +                                         rdma_chunk_size();
>>>
>>>      if (result > (rdma_ram_block->local_host_addr + rdma_ram_block->length)) {
>>>          result = rdma_ram_block->local_host_addr + rdma_ram_block->length;
>>> @@ -1841,6 +1848,7 @@ static int qemu_rdma_write_one(RDMAContext *rdma,
>>>      struct ibv_send_wr *bad_wr;
>>>      int reg_result_idx, ret, count = 0;
>>>      uint64_t chunk, chunks;
>>> +    uint64_t chunk_size = rdma_chunk_size();
>>>      uint8_t *chunk_start, *chunk_end;
>>>      RDMALocalBlock *block = &(rdma->local_ram_blocks.block[current_index]);
>>>      RDMARegister reg;
>>> @@ -1861,22 +1869,21 @@ retry:
>>>      chunk_start = ram_chunk_start(block, chunk);
>>>
>>>      if (block->is_ram_block) {
>>> -        chunks = length / (1UL << RDMA_REG_CHUNK_SHIFT);
>>> +        chunks = length / chunk_size;
>>>
>>> -        if (chunks && ((length % (1UL << RDMA_REG_CHUNK_SHIFT)) == 0)) {
>>> +        if (chunks && ((length % chunk_size) == 0)) {
>>>              chunks--;
>>>          }
>>>      } else {
>>> -        chunks = block->length / (1UL << RDMA_REG_CHUNK_SHIFT);
>>> +        chunks = block->length / chunk_size;
>>>
>>> -        if (chunks && ((block->length % (1UL << RDMA_REG_CHUNK_SHIFT)) == 0)) {
>>> +        if (chunks && ((block->length % chunk_size) == 0)) {
>>>              chunks--;
>>>          }
>>>      }
>>>
>>>      trace_qemu_rdma_write_one_top(chunks + 1,
>>> -                                  (chunks + 1) *
>>> -                                  (1UL << RDMA_REG_CHUNK_SHIFT) / 1024 / 1024);
>>> +                                  (chunks + 1) * chunk_size / 1024 / 1024);
>>>
>>>      chunk_end = ram_chunk_end(block, chunk + chunks);
>>>
>>> @@ -2176,7 +2183,7 @@ static int qemu_rdma_write(RDMAContext *rdma,
>>>      rdma->current_length += len;
>>>
>>>      /* flush it if buffer is too large */
>>> -    if (rdma->current_length >= RDMA_MERGE_MAX) {
>>> +    if (rdma->current_length >= rdma_merge_max()) {
>>>          return qemu_rdma_write_flush(rdma, errp);
>>>      }
>>>
>>> @@ -3522,7 +3529,7 @@ int rdma_registration_handle(QEMUFile *f)
>>>                  } else {
>>>                      chunk = reg->key.chunk;
>>>                      host_addr = block->local_host_addr +
>>> -                        (reg->key.chunk * (1UL << RDMA_REG_CHUNK_SHIFT));
>>> +                        (reg->key.chunk * rdma_chunk_size());
>>>                      /* Check for particularly bad chunk value */
>>>                      if (host_addr < (void *)block->local_host_addr) {
>>>                          error_report("rdma: bad chunk for block %s"
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index 7134d4ce47..0521bf3d69 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -1007,9 +1007,14 @@
>>>  #     is @cpr-exec.  The first list element is the program's filename,
>>>  #     the remainder its arguments.  (Since 10.2)
>>>  #
>>> +# @x-rdma-chunk-shift: RDMA memory registration chunk shift.
>>> +#     The chunk size is 2^N bytes where N is the value.
>>
>> The value of what?  Oh, the value of @x-rdma-chunk-shift.
>>
>> Acceptable range?  I doubt 0 or 255 work :)
>
> The valid range is [20, 30]. 20 is the default/original value. 30 is the largest value working on my servers with Mellanox RDMA NIC.

The range needs to be documented here.

>> Would this be easier to document if we make it a byte count
>> @x-rdma-chunk-size, must be a power of two?
>
> Switch to byte count will be easier to document, but the user may find it a bit harder to use.
>
> `-global migration.x-rdma-chunk-size=1073741824`
>
> The valid range is [1048576, 1073741824] and it should be power of 2.
>
>
> Do you prefer `x-rdma-chunk-size`? If yes, I will make the switch in v2 patch.

Have a look at migration parameter @max-bandwidth.  It supports size
prefixes like 1M.

>>> +#     Defaults to 20 (1 MiB).  Only takes effect for RDMA migration.
>>> +#     (Since 10.2)
>>
>> 11.0 right now, but realistically 11.1.
>
> OK. I will update it in v2 patch.
>
>
>>> +#
>>>  # Features:
>>>  #
>>> -# @unstable: Members @x-checkpoint-delay and
>>> +# @unstable: Members @x-rdma-chunk-shift, @x-checkpoint-delay and
>>>  #     @x-vcpu-dirty-limit-period are experimental.
>>
>> Keep the list of members sorted:
>>
>>     # @unstable: Members @x-checkpoint-delay, @x-rdma-chunk-shift, and
>>     #     @x-vcpu-dirty-limit-period are experimental.
>
>
> OK. I will update it in v2 patch.
>
>
>>
>>>  #
>>>  # Since: 2.4
>>> @@ -1045,6 +1050,8 @@
>>>              '*vcpu-dirty-limit': 'uint64',
>>>              '*mode': 'MigMode',
>>>              '*zero-page-detection': 'ZeroPageDetection',
>>> +            '*x-rdma-chunk-shift': { 'type': 'uint8',
>>> +                                     'features': [ 'unstable' ] },
>>>              '*direct-io': 'bool',
>>>              '*cpr-exec-command': [ 'str' ]} }