Peter Xu <pet...@redhat.com> writes:
> On Mon, Oct 23, 2023 at 05:35:55PM -0300, Fabiano Rosas wrote:
>> From: Nikolay Borisov <nbori...@suse.com>
>>
>> Add the necessary code to parse the format changes for the 'fixed-ram'
>> capability.
>>
>> One of the more notable changes in behavior is that in the 'fixed-ram'
>> case ram pages are restored in one go rather than constantly looping
>> through the migration stream.
>>
>> Signed-off-by: Nikolay Borisov <nbori...@suse.com>
>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>> ---
>> (farosas) reused more of the common code by making the fixed-ram
>> function take only one ramblock and calling it from inside
>> parse_ramblock.
>> ---
>> migration/ram.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 93 insertions(+)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 152a03604f..cea6971ab2 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -3032,6 +3032,32 @@ static void fixed_ram_insert_header(QEMUFile *file,
>> RAMBlock *block)
>> qemu_put_buffer(file, (uint8_t *) header, header_size);
>> }
>>
>> +static int fixed_ram_read_header(QEMUFile *file, struct FixedRamHeader
>> *header)
>> +{
>> + size_t ret, header_size = sizeof(struct FixedRamHeader);
>> +
>> + ret = qemu_get_buffer(file, (uint8_t *)header, header_size);
>> + if (ret != header_size) {
>> + return -1;
>> + }
>> +
>> + /* migration stream is big-endian */
>> + be32_to_cpus(&header->version);
>> +
>> + if (header->version > FIXED_RAM_HDR_VERSION) {
>> + error_report("Migration fixed-ram capability version mismatch
>> (expected %d, got %d)",
>> + FIXED_RAM_HDR_VERSION, header->version);
>
> I know it doesn't matter a lot for now, but it'll be good to start using
> Error** in new codes?
This whole series was written before the many discussions we had about
error handling. Thanks for pointing that out, I'll revise and change
where appropriate.
>> + return -1;
>> + }
>> +
>> + be64_to_cpus(&header->page_size);
>> + be64_to_cpus(&header->bitmap_offset);
>> + be64_to_cpus(&header->pages_offset);
>> +
>> +
>> + return 0;
>> +}
>> +
>> /*
>> * Each of ram_save_setup, ram_save_iterate and ram_save_complete has
>> * long-running RCU critical section. When rcu-reclaims in the code
>> @@ -3932,6 +3958,68 @@ void colo_flush_ram_cache(void)
>> trace_colo_flush_ram_cache_end();
>> }
>>
>> +static void read_ramblock_fixed_ram(QEMUFile *f, RAMBlock *block,
>> + long num_pages, unsigned long *bitmap)
>> +{
>> + unsigned long set_bit_idx, clear_bit_idx;
>> + unsigned long len;
>> + ram_addr_t offset;
>> + void *host;
>> + size_t read, completed, read_len;
>> +
>> + for (set_bit_idx = find_first_bit(bitmap, num_pages);
>> + set_bit_idx < num_pages;
>> + set_bit_idx = find_next_bit(bitmap, num_pages, clear_bit_idx + 1))
>> {
>> +
>> + clear_bit_idx = find_next_zero_bit(bitmap, num_pages, set_bit_idx +
>> 1);
>> +
>> + len = TARGET_PAGE_SIZE * (clear_bit_idx - set_bit_idx);
>> + offset = set_bit_idx << TARGET_PAGE_BITS;
>> +
>> + for (read = 0, completed = 0; completed < len; offset += read) {
>> + host = host_from_ram_block_offset(block, offset);
>> + read_len = MIN(len, TARGET_PAGE_SIZE);
>
> Why MIN()? I didn't read qemu_get_buffer_at() yet, but shouldn't len
> always be multiple of target page size or zero?
>
Hmm, this is not my code. The original code had MIN(len, BUFSIZE) with
BUFSIZE being defined at 4M. I think the idea might have been to cap the
amount of pages read.
So it seems I have made a mistake here and this could be reading more
pages at a time.
