Re: [PATCH v7 11/12] multifd: Zero pages transmission

[Juan Quintela]

Leonardo Brás <leobras@redhat.com> wrote:
> On Tue, 2022-08-02 at 08:39 +0200, Juan Quintela wrote:
>> This implements the zero page dection and handling.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>

Hi

on further investigation, I see why it can't be a problem.


>>  static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
>> @@ -358,6 +365,18 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams 
>> *p, Error **errp)
>>          p->normal[i] = offset;
>>      }
>>  
>> +    for (i = 0; i < p->zero_num; i++) {
>> +        uint64_t offset = be64_to_cpu(packet->offset[p->normal_num + i]);
>> +
>> +        if (offset > (block->used_length - p->page_size)) {

We are checked that we are inside the RAM block.  You can't have a
bigger that 32bit offset when you have 32bits of RAM.


>> @@ -688,8 +710,16 @@ static void *multifd_send_thread(void *opaque)
>>              }
>>  
>>              for (int i = 0; i < p->pages->num; i++) {
>> -                p->normal[p->normal_num] = p->pages->offset[i];
>> -                p->normal_num++;
>> +                uint64_t offset = p->pages->offset[i];

We are reading the offset here.
p->pages->offset is ram_addr_t, so no prolbem here.

>> +                if (use_zero_page &&
>> +                    buffer_is_zero(rb->host + offset, p->page_size)) {
>> +                    p->zero[p->zero_num] = offset;
>
> Same here.

This and next case are exactly the same, we are doing:

ram_addr_t offset1;
u64 foo = offset1;
ram_addr_t offest2 = foo;

So, it should be right.  Everything is unsigned here.

>> +                    p->zero_num++;
>> +                    ram_release_page(rb->idstr, offset);
>> +                } else {
>> +                    p->normal[p->normal_num] = offset;
>
> Same here? (p->normal[i] can also be u32)

Thanks, Juan.