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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Juan Quintela (quintela@redhat.com) wrote:
> Peter Xu <peterx@redhat.com> 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 <quintela@redhat.com>
> >> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> >
> > 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.
> 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.

It should be vaguely right; the rdma code calls back into acct_update_position
to update them; but I agree it looks odd;  that line almost looks like it's the error
case - so why is it incrementing dup_pages?

Dave

> Thanks, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK