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

qemu-devel

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From:	Hyman
Subject:	Re: [PATCH v16 0/7] support dirty restraint on vCPU
Date:	Thu, 3 Mar 2022 01:18:21 +0800
User-agent:	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.4.1



在 2022/3/3 0:53, 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

query-vcpu-dirty-limit sucess only if dirty ring feature enabled. So iprefer to add this command to the list of kipped command. I'll fix itnext version and run the qtests before i post the patchset.


Thinks
Yong

   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" function4. 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]