| Message ID | 20200601040250.38324-1-zhukeqian1@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Count new_dirty instead of real_dirty | expand |
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 >
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 > >
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
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
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(-)