RE: [PATCH] migration: Count new_dirty instead of real_dirty

[Zhoujian (jay)]

Hi Keqian,

> -----Original Message-----
> From: zhukeqian
> Sent: Monday, June 15, 2020 11:19 AM
> To: qemu-devel@nongnu.org; qemu-arm@nongnu.org; Paolo Bonzini
> <pbonzini@redhat.com>; Zhoujian (jay) <jianjay.zhou@huawei.com>
> Cc: Juan Quintela <quintela@redhat.com>; Chao Fan <fanc.fnst@cn.fujitsu.com>;
> Wanghaibin (D) <wanghaibin.wang@huawei.com>
> Subject: Re: [PATCH] migration: Count new_dirty instead of real_dirty
> 
> Hi Paolo and Jian Zhou,
> 
> Do you have any suggestion on this patch?
> 
> Thanks,
> Keqian
> 
> On 2020/6/1 12:02, Keqian Zhu wrote:
> > DIRTY_LOG_INITIALLY_ALL_SET feature is on the queue. This fixs the

s/fixs/fixes

> > dirty rate calculation for this feature. After introducing this
> > feature, real_dirty_pages is equal to total memory size at begining.
> > This causing wrong dirty rate and false positive throttling.

I think it should be tested whether DIRTY_LOG_INITIALLY_ALL_SET is enabled
in ram_init_bitmaps(maybe?) in order to be compatible with the old path.

Thanks,
Jay Zhou

> >
> > BTW, real dirty rate is not suitable and not very accurate.
> >
> > 1. For not suitable: We mainly concern on the relationship between
> >    dirty rate and network bandwidth. Net increasement of dirty pages
> >    makes more sense.
> > 2. For not very accurate: With manual dirty log clear, some dirty pages
> >    will be cleared during each peroid, our "real dirty rate" is less
> >    than real "real dirty rate".



> >
> > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
> > ---
> >  include/exec/ram_addr.h | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h index
> > 5e59a3d8d7..af9677e291 100644
> > --- a/include/exec/ram_addr.h
> > +++ b/include/exec/ram_addr.h
> > @@ -443,7 +443,7 @@ static inline
> >  uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
> >                                                 ram_addr_t start,
> >                                                 ram_addr_t length,
> > -                                               uint64_t
> *real_dirty_pages)
> > +                                               uint64_t
> > + *accu_dirty_pages)
> >  {
> >      ram_addr_t addr;
> >      unsigned long word = BIT_WORD((start + rb->offset) >>
> > TARGET_PAGE_BITS); @@ -469,7 +469,6 @@ uint64_t
> cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
> >              if (src[idx][offset]) {
> >                  unsigned long bits = atomic_xchg(&src[idx][offset], 0);
> >                  unsigned long new_dirty;
> > -                *real_dirty_pages += ctpopl(bits);
> >                  new_dirty = ~dest[k];
> >                  dest[k] |= bits;
> >                  new_dirty &= bits;
> > @@ -502,7 +501,6 @@ uint64_t
> cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
> >                          start + addr + offset,
> >                          TARGET_PAGE_SIZE,
> >                          DIRTY_MEMORY_MIGRATION)) {
> > -                *real_dirty_pages += 1;
> >                  long k = (start + addr) >> TARGET_PAGE_BITS;
> >                  if (!test_and_set_bit(k, dest)) {
> >                      num_dirty++;
> > @@ -511,6 +509,7 @@ uint64_t
> cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
> >          }
> >      }
> >
> > +    *accu_dirty_pages += num_dirty;
> >      return num_dirty;
> >  }
> >  #endif
> >