Re: [PATCH v16 0/7] support dirty restraint on vCPU

[Dr. David Alan Gilbert]

* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * huangy81@chinatelecom.cn (huangy81@chinatelecom.cn) wrote:
> > From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> Queued via my migration/hmp/etc tree

Hi,
  Unfortunately I've had to unqueue this - it breaks the
qmp-cmd-test:

# starting QEMU: exec ./x86_64-softmmu/qemu-system-x86_64 -qtest 
unix:/tmp/qtest-142136.sock -qtest-log /dev/fd/2 -chardev 
socket,path=/tmp/qtest-142136.qmp,id=char0 -mon chardev=char0,mode=control 
-display none -nodefaults -machine none -accel qtest
[I 1646239093.713627] OPENED
[R +0.000190] endianness
[S +0.000196] OK little
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 6}, "package": 
"v6.2.0-1867-g817703d65a"}, "capabilities": ["oob"]}}{"execute": 
"qmp_capabilities"}

{"return": {}}{"execute": "query-vcpu-dirty-limit"}

{"error": {"class": "GenericError", "desc": "dirty page limit not enabled"}}**
ERROR:../tests/qtest/qmp-cmd-test.c:84:test_query: assertion failed: 
(qdict_haskey(resp, "return"))
Bail out! ERROR:../tests/qtest/qmp-cmd-test.c:84:test_query: assertion failed: 
(qdict_haskey(resp, "return"))
[I +0.195433] CLOSED
Aborted (core dumped)


qmp-cmd-test tries to run every query command; so either you need to:
  a) Add it to the list of skipped command in qmp-cmd-test
  b) Make it not actually error when the limit isn't enabled.

Dave

> > 
> > v16
> > - rebase on master
> > - drop the unused typedef syntax in [PATCH v15 6/7] 
> > - add the Reviewed-by and Acked-by tags by the way 
> > 
> > v15
> > - rebase on master
> > - drop the 'init_time_ms' parameter in function vcpu_calculate_dirtyrate 
> > - drop the 'setup' field in dirtylimit_state and call dirtylimit_process
> >   directly, which makes code cleaner.
> > - code clean in dirtylimit_adjust_throttle
> > - fix miss dirtylimit_state_unlock() in dirtylimit_process and
> >   dirtylimit_query_all
> > - add some comment
> > 
> > Please review. Thanks,
> > 
> > Regards
> > Yong 
> > 
> > v14
> > - v13 sent by accident, resend patchset. 
> > 
> > v13
> > - rebase on master
> > - passing NULL to kvm_dirty_ring_reap in commit
> >   "refactor per-vcpu dirty ring reaping" to keep the logic unchanged.
> >   In other word, we still try the best to reap as much PFNs as possible
> >   if dirtylimit not in service.
> > - move the cpu list gen id changes into a separate patch.   
> > - release the lock before sleep during dirty page rate calculation.
> > - move the dirty ring size fetch logic into a separate patch.
> > - drop the DIRTYLIMIT_LINEAR_ADJUSTMENT_WATERMARK MACRO .
> > - substitute bh with function pointer when implement dirtylimit.
> > - merge the dirtylimit_start/stop into dirtylimit_change.
> > - fix "cpu-index" parameter type with "int" to keep consistency.
> > - fix some syntax error in documents.
> > 
> > Please review. Thanks,
> > 
> > Yong
> > 
> > v12
> > - rebase on master
> > - add a new commmit to refactor per-vcpu dirty ring reaping, which can 
> > resolve 
> >   the "vcpu miss the chances to sleep" problem
> > - remove the dirtylimit_thread and implemtment throttle in bottom half 
> > instead.
> > - let the dirty ring reaper thread keep sleeping when dirtylimit is in 
> > service 
> > - introduce cpu_list_generation_id to identify cpu_list changing. 
> > - keep taking the cpu_list_lock during dirty_stat_wait to prevent vcpu 
> > plug/unplug
> >   when calculating the dirty page rate
> > - move the dirtylimit global initializations out of dirtylimit_set_vcpu and 
> > do
> >   some code clean
> > - add DIRTYLIMIT_LINEAR_ADJUSTMENT_WATERMARK in case of oscillation when 
> > throttling 
> > - remove the unmatched count field in dirtylimit_state
> > - add stub to fix build on non-x86
> > - refactor the documents
> > 
> > Thanks Peter and Markus for reviewing the previous versions, please review.
> > 
> > Thanks,
> > Yong
> > 
> > v11
> > - rebase on master
> > - add a commit " refactor dirty page rate calculation"  so that dirty page 
> > rate limit
> >   can reuse the calculation logic. 
> > - handle the cpu hotplug/unplug case in the dirty page rate calculation 
> > logic.
> > - modify the qmp commands according to Markus's advice.
> > - introduce a standalone file dirtylimit.c to implement dirty page rate 
> > limit
> > - check if dirty limit in service by dirtylimit_state pointer instead of 
> > global variable
> > - introduce dirtylimit_mutex to protect dirtylimit_state
> > - do some code clean and docs
> > 
> > See the commit for more detail, thanks Markus and Peter very mush for the 
> > code
> > review and give the experienced and insightful advices, most modifications 
> > are
> > based on these advices.
> > 
> > v10:
> > - rebase on master
> > - make the following modifications on patch [1/3]:
> >   1. Make "dirtylimit-calc" thread joinable and join it after quitting.
> > 
> >   2. Add finalize function to free dirtylimit_calc_state
> > 
> >   3. Do some code clean work
> > 
> > - make the following modifications on patch [2/3]:
> >   1. Remove the original implementation of throttle according to
> >      Peter's advice.
> >      
> >   2. Introduce a negative feedback system and implement the throttle
> >      on all vcpu in one thread named "dirtylimit". 
> > 
> >   3. Simplify the algo when calculation the throttle_us_per_full:
> >      increase/decrease linearly when there exists a wide difference
> >      between quota and current dirty page rate, increase/decrease
> >      a fixed time slice when the difference is narrow. This makes
> >      throttle responds faster and reach the quota smoothly.
> > 
> >   4. Introduce a unfit_cnt in algo to make sure throttle really
> >      takes effect.
> > 
> >   5. Set the max sleep time 99 times more than "ring_full_time_us".         
> >                                                                             
> >                                                                             
> >          
> >                                                                             
> >                                                                             
> >                                                                             
> >          
> >   6. Make "dirtylimit" thread joinable and join it after quitting.          
> >                                                                             
> >                                                                             
> >          
> >                                                                             
> >                                                                             
> >                                                                             
> >          
> > - make the following modifications on patch [3/3]:                          
> >                                                                             
> >                                                                             
> >          
> >   1. Remove the unplug cpu handling logic.                                  
> >                                                                             
> >                                                                             
> >          
> >                                                                             
> >                                                                             
> >                                                                             
> >          
> >   2. "query-vcpu-dirty-limit" only return dirtylimit information of         
> >                                                                             
> >                                                                             
> >          
> >      vcpus that enable dirtylimit                                           
> >                                                                             
> >                                                                             
> >          
> >                                                                             
> >                                                                             
> >                                                                             
> >          
> >   3. Remove the "dirtylimit_setup" function                                 
> >                                                                             
> >                                                                             
> >          
> >                                                                             
> >                                                                             
> >                                                                             
> >          
> >   4. Trigger the dirtylimit and initialize the global state only            
> >                                                                             
> >                                                                             
> >          
> >      when someone enable dirtylimit, and finalize it after the last         
> >                                                                             
> >                                                                             
> >          
> >      dirtylimit be canceled.                                                
> >                                                                             
> >                                                                             
> >          
> >                                                                             
> >                                                                             
> >                                                                             
> >          
> >   5. Redefine the qmp command vcpu-dirty-limit/query-vcpu-dirty-limit:      
> >                                                                             
> >                                                                             
> >          
> >      enable/disable dirtylimit use a single command "vcpu-dirty-limit",
> >      to enable/disabled dirtylimit on specified vcpu only if "cpu-index"
> >      is specified, otherwise, all vcpu will be affected.
> > 
> >   6. Redefine the hmp command vcpu_dirty_limit/info vcpu_dirty_limit
> > 
> > - other points about the code review:
> >   1. "merge the code of calculation dirty page rate"
> >      I think maybe it's not suitable to touch the 'calc-dirty-rate',
> >      because 'calc-dirty-rate' will stop sync log after calculating 
> >      the dirtyrate and the 'dirtylimit-cal' will not untill the last
> >      dirtylimit be canceled, if we merge the GLOBAL_DIRTY_LIMIT into
> >      GLOBAL_DIRTY_DIRTYRATE, the two are interacted with each other.
> > 
> >   2. The new implementaion of throttle algo enlightened by Peter
> >      responds faster and consume less cpu resource than the older,
> >      we make a impressed progress.
> > 
> >      And there is a viewpoint may be discussed, it is that the new 
> >      throttle logic is "passive", vcpu sleeps only after dirty ring,
> >      is full, unlike the "auto-converge" which will kick vcpu instead
> >      in a fixed slice time. If the vcpu is memory-write intensive
> >      and the ring size is large, it will produce dirty memory during
> >      the dirty ring full time and the throttle works not so good, it
> >      means the throttle depends on the dirty ring size. 
> > 
> >      I actually tested the new algo in two case:
> > 
> >      case 1: dirty-ring-size: 4096, dirtyrate: 1170MB/s 
> >      result: minimum quota dirtyrate is 25MB/s or even less
> >              minimum vcpu util is 6%
> > 
> >      case 2: dirty-ring-size: 65536, dirtyrate: 1170MB/s 
> >      result: minimum quota dirtyrate is 256MB/s
> >              minimum vcpu util is 24%
> >      
> >      I post this just for discussion, i think this is not a big deal
> >      beacase if we set the dirty-ring-size to the maximum value(65536),
> >      we assume the server's bandwidth is capable of handling it.
> > 
> >   3. I hard-code the acceptable deviation value to 25MB/s, see the
> >      macro DIRTYLIMIT_TOLERANCE_RANGE. I'm struggling to decide 
> >      whether to let it configurable
> >    
> >   4. Another point is the unplug cpu handle, current algo affects the
> >      unplugged vcpu, if we set dirty limit on it, we should fork 2 
> >      thread "dirtylimit" and "dirtylimit-calc" but do nothing, once the
> >      vcpu is hot-plugged, dirty limit works, i think the logic is ok
> >      but still there can be different advice.
> > 
> > - to let developers play with it easier, i post the hmp usage example:
> >   (qemu) vcpu_dirty_limit -g on -1 500
> >   [Please use 'info vcpu_dirty_limit' to query dirty limit for virtual CPU]
> >   
> >   (qemu) info vcpu_dirty_limit 
> >   vcpu[0], limit rate 500 (MB/s), current rate 415 (MB/s)
> >   vcpu[1], limit rate 500 (MB/s), current rate 496 (MB/s)
> >   vcpu[2], limit rate 500 (MB/s), current rate 0 (MB/s)
> >   vcpu[3], limit rate 500 (MB/s), current rate 0 (MB/s)
> >   (qemu) vcpu_dirty_limit -g off
> >   [Please use 'info vcpu_dirty_limit' to query dirty limit for virtual CPU]
> >   
> >   (qemu) info vcpu_dirty_limit 
> >   Dirty page limit not enabled!
> >   
> >   (qemu) vcpu_dirty_limit on 0 300
> >   [Please use 'info vcpu_dirty_limit' to query dirty limit for virtual CPU]
> >   
> >   (qemu) vcpu_dirty_limit on 1 500
> >   [Please use 'info vcpu_dirty_limit' to query dirty limit for virtual CPU]
> >   
> >   (qemu) info vcpu_dirty_limit 
> >   vcpu[0], limit rate 300 (MB/s), current rate 342 (MB/s)
> >   vcpu[1], limit rate 500 (MB/s), current rate 485 (MB/s)
> >   
> >   (qemu) vcpu_dirty_limit off 0
> >   [Please use 'info vcpu_dirty_limit' to query dirty limit for virtual CPU]
> >   
> >   (qemu) info vcpu_dirty_limit 
> >   vcpu[1], limit rate 500 (MB/s), current rate 528 (MB/s)
> >   
> >   (qemu) vcpu_dirty_limit off 1
> >   [Please use 'info vcpu_dirty_limit' to query dirty limit for virtual CPU]
> >   
> >   (qemu) info vcpu_dirty_limit 
> >   Dirty page limit not enabled!
> > 
> > Thanks very much for the instructive algo suggestion given by Peter,
> > the comment and other code reviews made by Markus.
> > 
> > Please review, thanks!
> > 
> > v9:
> > - rebase on master
> > - fix the meson directory change, keep it untouched.
> > 
> > v8:
> > - rebase on master
> > - polish the error message and remove the "unlikely" compilation syntax
> >   according to the advice given by Markus.
> > - keep the dirty tracking enabled during "dirtylimit-calc" lifecycle
> >   so that the overhead can be reduced according to the advice given by
> >   Peter. 
> > - merge the "set/cancel" qmp commands into one named "vcpu-dirty-limit"
> >   and introduce qmp command "query-vcpu-dirty-limit" to query dirty
> >   limit information about virtual CPU, according to the advice given by
> >   Peter.
> > - check if vcpu index is valid and handle the unplug case before
> >   enabling, disabling dirty limit for virtual CPU.
> > - introduce hmp commands so developers can play with them easier, use
> >   "vcpu_dirty_limit" to enable dirty limit and "info vcpu_dirty_limit"
> >   to query.
> > 
> > The patch [2/3] has not been touched so far. Any corrections and
> > suggetions are welcome. 
> > 
> > Please review, thanks!
> > 
> > v7:
> > - rebase on master
> > - polish the comments and error message according to the
> >   advices given by Markus
> > - introduce dirtylimit_enabled function to pre-check if dirty
> >   page limit is enabled before canceling.
> > 
> > v6:
> > - rebase on master
> > - fix dirtylimit setup crash found by Markus
> > - polish the comments according to the advice given by Markus
> > - adjust the qemu qmp command tag to 7.0
> > 
> > v5:
> > - rebase on master
> > - adjust the throttle algorithm by removing the tuning in 
> >   RESTRAINT_RATIO case so that dirty page rate could reachs the quota
> >   more quickly.
> > - fix percentage update in throttle iteration.
> > 
> > v4:
> > - rebase on master
> > - modify the following points according to the advice given by Markus
> >   1. move the defination into migration.json
> >   2. polish the comments of set-dirty-limit
> >   3. do the syntax check and change dirty rate to dirty page rate
> > 
> > Thanks for the carefule reviews made by Markus.
> > 
> > Please review, thanks!
> > 
> > v3:
> > - rebase on master
> > - modify the following points according to the advice given by Markus
> >   1. remove the DirtyRateQuotaVcpu and use its field as option directly
> >   2. add comments to show details of what dirtylimit setup do
> >   3. explain how to use dirtylimit in combination with existing qmp
> >      commands "calc-dirty-rate" and "query-dirty-rate" in documentation.
> > 
> > Thanks for the carefule reviews made by Markus.
> > 
> > Please review, thanks!
> > 
> > Hyman
> > 
> > v2:
> > - rebase on master
> > - modify the following points according to the advices given by Juan
> >   1. rename dirtyrestraint to dirtylimit
> >   2. implement the full lifecyle function of dirtylimit_calc, include
> >      dirtylimit_calc and dirtylimit_calc_quit
> >   3. introduce 'quit' field in dirtylimit_calc_state to implement the
> >      dirtylimit_calc_quit
> >   4. remove the ready_cond and ready_mtx since it may not be suitable
> >   5. put the 'record_dirtypage' function code at the beggining of the
> >      file
> >   6. remove the unnecesary return;
> > - other modifications has been made after code review
> >   1. introduce 'bmap' and 'nr' field in dirtylimit_state to record the
> >      number of running thread forked by dirtylimit
> >   2. stop the dirtyrate calculation thread if all the dirtylimit thread
> >      are stopped
> >   3. do some renaming works
> >      dirtyrate calulation thread -> dirtylimit-calc
> >      dirtylimit thread -> dirtylimit-{cpu_index}
> >      function name do_dirtyrestraint -> dirtylimit_check
> >      qmp command dirty-restraint -> set-drity-limit
> >      qmp command dirty-restraint-cancel -> cancel-dirty-limit
> >      header file dirtyrestraint.h -> dirtylimit.h
> > 
> > Please review, thanks !
> > 
> > thanks for the accurate and timely advices given by Juan. we really
> > appreciate it if corrections and suggetions about this patchset are
> > proposed.
> > 
> > Best Regards !
> > 
> > Hyman
> > 
> > v1:
> > this patchset introduce a mechanism to impose dirty restraint
> > on vCPU, aiming to keep the vCPU running in a certain dirtyrate
> > given by user. dirty restraint on vCPU maybe an alternative
> > method to implement convergence logic for live migration,
> > which could improve guest memory performance during migration
> > compared with traditional method in theory.
> > 
> > For the current live migration implementation, the convergence
> > logic throttles all vCPUs of the VM, which has some side effects.
> > -'read processes' on vCPU will be unnecessarily penalized
> > - throttle increase percentage step by step, which seems
> >   struggling to find the optimal throttle percentage when
> >   dirtyrate is high.
> > - hard to predict the remaining time of migration if the
> >   throttling percentage reachs 99%
> > 
> > to a certain extent, the dirty restraint machnism can fix these
> > effects by throttling at vCPU granularity during migration.
> > 
> > the implementation is rather straightforward, we calculate
> > vCPU dirtyrate via the Dirty Ring mechanism periodically
> > as the commit 0e21bf246 "implement dirty-ring dirtyrate calculation"
> > does, for vCPU that be specified to impose dirty restraint,
> > we throttle it periodically as the auto-converge does, once after
> > throttling, we compare the quota dirtyrate with current dirtyrate,
> > if current dirtyrate is not under the quota, increase the throttling
> > percentage until current dirtyrate is under the quota.
> > 
> > this patchset is the basis of implmenting a new auto-converge method
> > for live migration, we introduce two qmp commands for impose/cancel
> > the dirty restraint on specified vCPU, so it also can be an independent
> > api to supply the upper app such as libvirt, which can use it to
> > implement the convergence logic during live migration, supplemented
> > with the qmp 'calc-dirty-rate' command or whatever.
> > 
> > we post this patchset for RFC and any corrections and suggetions about
> > the implementation, api, throttleing algorithm or whatever are very
> > appreciated!
> > 
> > Please review, thanks !
> > 
> > Best Regards !
> > 
> > Hyman Huang (7):
> >   accel/kvm/kvm-all: Refactor per-vcpu dirty ring reaping
> >   cpus: Introduce cpu_list_generation_id
> >   migration/dirtyrate: Refactor dirty page rate calculation
> >   softmmu/dirtylimit: Implement vCPU dirtyrate calculation periodically
> >   accel/kvm/kvm-all: Introduce kvm_dirty_ring_size function
> >   softmmu/dirtylimit: Implement virtual CPU throttle
> >   softmmu/dirtylimit: Implement dirty page rate limit
> > 
> >  accel/kvm/kvm-all.c         |  45 +++-
> >  accel/stubs/kvm-stub.c      |   5 +
> >  cpus-common.c               |   8 +
> >  hmp-commands-info.hx        |  13 +
> >  hmp-commands.hx             |  32 +++
> >  include/exec/cpu-common.h   |   1 +
> >  include/exec/memory.h       |   5 +-
> >  include/hw/core/cpu.h       |   6 +
> >  include/monitor/hmp.h       |   3 +
> >  include/sysemu/dirtylimit.h |  37 +++
> >  include/sysemu/dirtyrate.h  |  28 +++
> >  include/sysemu/kvm.h        |   2 +
> >  migration/dirtyrate.c       | 227 +++++++++++------
> >  migration/dirtyrate.h       |   7 +-
> >  qapi/migration.json         |  80 ++++++
> >  softmmu/dirtylimit.c        | 602 
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  softmmu/meson.build         |   1 +
> >  softmmu/trace-events        |   7 +
> >  18 files changed, 1010 insertions(+), 99 deletions(-)
> >  create mode 100644 include/sysemu/dirtylimit.h
> >  create mode 100644 include/sysemu/dirtyrate.h
> >  create mode 100644 softmmu/dirtylimit.c
> > 
> > -- 
> > 1.8.3.1
> > 
> > 
> -- 
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK