1. Patchew
  2. QEMU
  3. View series

[PATCH] migration: Count new_dirty instead of real_dirty

Keqian Zhu posted 1 patch 3 years, 11 months ago
Test docker-mingw@fedora passed
Test checkpatch passed
Test asan passed
Test docker-quick@centos7 passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200601040250.38324-1-zhukeqian1@huawei.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>
There is a newer version of this series
include/exec/ram_addr.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
[PATCH] migration: Count new_dirty instead of real_dirty
Posted by Keqian Zhu 3 years, 11 months ago 
DIRTY_LOG_INITIALLY_ALL_SET feature is on the queue. This fixs the
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.

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
-- 
2.19.1
Re: [PATCH] migration: Count new_dirty instead of real_dirty
Posted by zhukeqian 3 years, 10 months ago 
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
> 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.
> 
> 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
>
RE: [PATCH] migration: Count new_dirty instead of real_dirty
Posted by Zhoujian (jay) 3 years, 10 months ago 
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
> >
Re: [PATCH] migration: Count new_dirty instead of real_dirty
Posted by zhukeqian 3 years, 10 months ago 
Hi Jay Zhou,

On 2020/6/15 19:50, Zhoujian (jay) wrote:
> 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
Thanks.
> 
>>> 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.
Yeah, you are right ;-)

But after I tested old path yesterday, I found that the num_dirty_pages_period
becomes total ram size after log sync in ram_init_bitmaps. The reason is that
bitmap of ramblock is initialized to be all set, so old path counts them as dirty
by mistake.

This bug causes false positive throttling at the end of first ram save iteration,
even without our DIRTY_LOG_INITIALLY_ALL_SET feature.
> 
> 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".
> 
I should correct these commit messages for reason above :-)
> 
> 
>>>
>>> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com>
[...]

Thanks,
Keqian

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.