Re: [Qemu-devel] [PATCH 11/51] ram: Move dup_pages into RAMState

[Juan Quintela]

Peter Xu <address@hidden> wrote:
> On Tue, Mar 28, 2017 at 08:43:37PM +0200, Juan Quintela wrote:
>> Peter Xu <address@hidden> wrote:
>> > On Thu, Mar 23, 2017 at 09:45:04PM +0100, Juan Quintela wrote:
>> >> Once there rename it to its actual meaning, zero_pages.
>> >> 
>> >> Signed-off-by: Juan Quintela <address@hidden>
>> >> Reviewed-by: Dr. David Alan Gilbert <address@hidden>
>> >
>> > Reviewed-by: Peter Xu <address@hidden>
>> >
>> > Will post a question below though (not directly related to this patch
>> > but context-wide)...
>> >>  {
>> >>      int pages = -1;
>> >>  
>> >>      if (is_zero_range(p, TARGET_PAGE_SIZE)) {
>> >> -        acct_info.dup_pages++;
>> >> +        rs->zero_pages++;
>> >>          *bytes_transferred += save_page_header(f, block,
>> >>                                                 offset | 
>> >> RAM_SAVE_FLAG_COMPRESS);
>> >>          qemu_put_byte(f, 0);
>> >> @@ -822,11 +826,11 @@ static int ram_save_page(RAMState *rs, 
>> >> MigrationState *ms, QEMUFile *f,
>> >>              if (bytes_xmit > 0) {
>> >>                  acct_info.norm_pages++;
>> >>              } else if (bytes_xmit == 0) {
>> >> -                acct_info.dup_pages++;
>> >> +                rs->zero_pages++;
>> >
>> > This code path looks suspicous... since iiuc currently it should only
>> > be triggered by RDMA case, and I believe here qemu_rdma_save_page()
>> > should have met something wrong (so that it didn't return with
>> > RAM_SAVE_CONTROL_DELAYED). Then is it correct we do increase zero page
>> > counting unconditionally here? (hmm, the default bytes_xmit is zero as
>> > well...)
>> 
>> My head hurts at this point.
>
> Sorry about that! :(

Hahaha, it was a ""figure of speak" O:-)

>> ok.  bytse_xmit can only be zero if we called qemu_rdma_save_page() with
>> size=0 or there has been an RDMA error.  We ver call the function with
>> size = 0.  And if there is one error, we are in very bady shape already.
>> 
>> > Another thing is that I see when RDMA is enabled we are updating
>> > accounting info with acct_update_position(), while we updated it here
>> > as well. Is this an issue of duplicated accounting?
>> 
>> I think stats and rdma are not right.  I have to check more that.
>
> Sorry to have led the discussion too far away from the topic. I guess
> it'll be perfectly okay to just mark this as TODO item, and we can
> just move on with current series (and I believe you have further
> patches after this big one :).

Yeap.

> Out of curiosity - to what extent are people using migration with
> RDMA? Should that be "very rare"? Thanks,

I don't really have numbers.  Some customers find it very important, but
I don't have a good idea of how to put it.

Later, Juan.