Change in vdsm[master]: vdsm: implement NUMA aware KSM policy

dudi at redhat.com dudi at redhat.com
Tue Jun 2 07:26:14 UTC 2015


Dudi Maroshi has posted comments on this change.

Change subject: vdsm: implement NUMA aware KSM policy
......................................................................


Patch Set 9:

(7 comments)

https://gerrit.ovirt.org/#/c/40129/9/vdsm/mom.d/03-ksm.policy
File vdsm/mom.d/03-ksm.policy:

Line 23: (defvar ksm_free_percent 0.20)
Line 24: 
Line 25: # KSM service can unmerged shared pages (ksm_run = 2)
Line 26: # When this happens the KSM is affectivley non responsive
Line 27: (defvar ksm_umerging_pages 0)
> There is probably a typo here. But I believe you do not need the variable a
Done.

A typo for sure.

Yet we do need a global/external variable that persist across  function invocations.
Line 28: 
Line 29: ### Helper functions
Line 30: (def change_npages (delta)
Line 31: {


Line 34:     (if (< newval ksm_npages_min) (set newval ksm_npages_min) 0)
Line 35:     (Host.Control "ksm_pages_to_scan" newval)
Line 36: })
Line 37: 
Line 38: (def enable_ksmMergeAcrossNodes()
> please use words separated by _ in names.. and this should probably be call
Done

Will rename the function to better reflect its responsibility.
Line 39: {
Line 40:     (debug "entry: enable_ksmMergeAcrossNodes")
Line 41:     (debug Host.ksm_run "=ksm_run")
Line 42:     (debug Host.ksm_merge_across_nodes "=ksm_merge_across_nodes")


Line 36: })
Line 37: 
Line 38: (def enable_ksmMergeAcrossNodes()
Line 39: {
Line 40:     (debug "entry: enable_ksmMergeAcrossNodes")
> Please reduce the number of logging lines. (You can merge them to a single 
The debug lines are my method of debugging the state machine.

The state machine status is progressing through invocations. Therefore we need a progress feedback. Also we need feedback about the logic path the program takes. Need advise as to override debug function with finer control switch (turn on/off per flag).

The debug lines are also used as documentation.

Need advise to improve debugging technique.
Line 41:     (debug Host.ksm_run "=ksm_run")
Line 42:     (debug Host.ksm_merge_across_nodes "=ksm_merge_across_nodes")
Line 43:     (debug ksmMergeAcrossNodes "=ksmMergeAcrossNodes")
Line 44:     (if (!= Host.ksm_merge_across_nodes ksmMergeAcrossNodes){


Line 42:     (debug Host.ksm_merge_across_nodes "=ksm_merge_across_nodes")
Line 43:     (debug ksmMergeAcrossNodes "=ksmMergeAcrossNodes")
Line 44:     (if (!= Host.ksm_merge_across_nodes ksmMergeAcrossNodes){
Line 45:         (debug "ksm_merge_across_nodes policy change")
Line 46:         (if (== Host.ksm_run 0){
> Please add comments explaining the intended behaviour (in detail!) so we do
Done
Line 47:             (debug "ksm_run == 0 : Update ksm_merge_across_nodes")
Line 48:             (Host.Control "ksm_merge_across_nodes" ksmMergeAcrossNodes)
Line 49:             (debug "ksm_merge_across_nodes configured")}
Line 50:           6)


Line 42:     (debug Host.ksm_merge_across_nodes "=ksm_merge_across_nodes")
Line 43:     (debug ksmMergeAcrossNodes "=ksmMergeAcrossNodes")
Line 44:     (if (!= Host.ksm_merge_across_nodes ksmMergeAcrossNodes){
Line 45:         (debug "ksm_merge_across_nodes policy change")
Line 46:         (if (== Host.ksm_run 0){
> Shouldn't we unmerge when the across node settings changes to 0 even though
Yes need to unmerge KSM when transition ksmMergeAcrossNode=0 to ksmMergeAcrossNode=1 as well. Following the official documentation https://www.kernel.org/doc/Documentation/vm/ksm.txt
Line 47:             (debug "ksm_run == 0 : Update ksm_merge_across_nodes")
Line 48:             (Host.Control "ksm_merge_across_nodes" ksmMergeAcrossNodes)
Line 49:             (debug "ksm_merge_across_nodes configured")}
Line 50:           6)


Line 69:             (debug "ksm_merge_across_nodes configured")}
Line 70:           9)
Line 71:      } 10)
Line 72:     (debug "exit: enable_ksmMergeAcrossNodes")
Line 73: })
> Please use the function as a function.. return the expected ksm_run mode he
Done, need some syntax instructions.
Line 74: ### Main Script
Line 75: # Methodology: Since running KSM does incur some overhead, try to run it only
Line 76: # when necessary.  If the amount of committed KSM shareable memory is high or if
Line 77: # free memory is low, enable KSM to try to increase free memory.  Large memory


Line 80: 
Line 81: (defvar ksm_pressure_threshold (* Host.mem_available ksm_free_percent))
Line 82: (defvar ksm_committed Host.ksm_shareable)
Line 83: (enable_ksmMergeAcrossNodes)
Line 84: (if (== ksm_umerging_pages 0)
> Why didn't you add this as another condition to the nested if and used the 
As stated there is separation of concerns, applying_KSM_policy and activating_KSM. Both share operational kernel flags. applying_KSM_policy invocation is rare, may take few MoM cycles to complete. While  activating_KSM may override applying_KSM_policy run mode.  Therefore we might wish to make them exclusive to each other (no kernel flags overriding, simpler to understand).
Line 85:     (if (or (and (< (+ ksm_pressure_threshold ksm_committed) Host.mem_available)
Line 86:                  (> (Host.StatAvg "mem_free") ksm_pressure_threshold)) 
Line 87:              (not ksmEnabled))
Line 88:         (Host.Control "ksm_run" 0)


-- 
To view, visit https://gerrit.ovirt.org/40129
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec607f9ef3284c1448bfc2831d125fc4d81b28d2
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Dudi Maroshi <dudi at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Dudi Maroshi <dudi at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Sivák <msivak at redhat.com>
Gerrit-Reviewer: Max Kovgan <mvk at redhat.com>
Gerrit-Reviewer: Roy Golan <rgolan at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list