Change in vdsm[master]: Replace calling setSchedulerParameters() with filling its XM...

Mark Wu has uploaded a new change for review.

Change subject: Replace calling setSchedulerParameters() with filling its XML description.
......................................................................

Replace calling setSchedulerParameters() with filling its XML description.

This patch changes to fill cgroup cpu.share into its XML description to set
Vm's niceness instead of calling setSchedulerParameters(). It can save us a
libvirt call on VM statup.

Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Signed-off-by: Mark Wu 
---
M vdsm/libvirtvm.py
1 file changed, 10 insertions(+), 10 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/90/6290/1
--
To view, visit http://gerrit.ovirt.org/6290
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu

Mark Wu has posted comments on this change.

Change subject: Replace calling setSchedulerParameters() with filling its XML description.
......................................................................

Patch Set 1: Verified

--
To view, visit http://gerrit.ovirt.org/6290
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu 
Gerrit-Reviewer: Mark Wu

Doron Fediuck has posted comments on this change.

Change subject: Replace calling setSchedulerParameters() with filling its XML description.
......................................................................

Patch Set 1: (2 inline comments)

Mark,
it terms of libvirt functionality, does this patch change the functionality?

Also, please see inline- I'd like to make sure we do not loose a default setup for nice.

....................................................
Commit Message
Line 11: libvirt call on VM statup.
(sorry for nit-picking)

s/statup/startup/ ?

I think VM start is simpler and even better

....................................................
File vdsm/libvirtvm.py
Line 704:         if 'cpuPinning' in self.conf or 'nice' in self.conf:
Mark, please correct me if I'm wrong, but in previous implementation we always had a default 'nice' (it wasn't within a conditional block), whereas in this implementation if you do not get both pinning and nice, there will be no default nice set.

--
To view, visit http://gerrit.ovirt.org/6290
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Michal Skrivanek

Laszlo Hornyak has posted comments on this change.

Change subject: Replace calling setSchedulerParameters() with filling its XML description.
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/libvirtvm.py
Line 709:             for cpuPin in cpuPinning.keys():
What if there is 'nice' in self.conf but no 'cpuPinning' in self.conf? is it possible?

--
To view, visit http://gerrit.ovirt.org/6290
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Laszlo Hornyak 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Michal Skrivanek

Mark Wu has posted comments on this change.

Change subject: Replace calling setSchedulerParameters() with filling its XML description.
......................................................................

Patch Set 1: (3 inline comments)

....................................................
Commit Message
Line 11: libvirt call on VM statup.
Done

....................................................
File vdsm/libvirtvm.py
Line 704:         if 'cpuPinning' in self.conf or 'nice' in self.conf:
Yes,  in original implementation, a default value 0 was assigned if it's missing in conf.
That means the default cpu share is 1020,  which is calculated from (20 - 0) * 51. 
Actually,  the default cpu share of each process on system is 1024. For example, you could see 
 /sys/fs/cgroup/cpu/system/libvirtd.service/libvirt/cpu.shares.  So in the new implementation,  the cpu share of all vms without 'nice' in conf  are 1024.  That almost has the same effect to original implementation.  So I think that's fine.

Line 709:             for cpuPin in cpuPinning.keys():
It's possible. But this case is already covered. If no 'cpuPinning',  cpuPinning is assigned to {},  so it will execute the for loop

--
To view, visit http://gerrit.ovirt.org/6290
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Laszlo Hornyak 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Michal Skrivanek

Doron Fediuck has posted comments on this change.

Change subject: Replace calling setSchedulerParameters() with filling its XML description.
......................................................................

Patch Set 2: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/6290
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Laszlo Hornyak 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: oVirt Jenkins CI Server

Laszlo Hornyak has posted comments on this change.

Change subject: Replace calling setSchedulerParameters() with filling its XML description.
......................................................................

Patch Set 2: Verified; Looks good to me, but someone else must approve

(2 inline comments)

sorry for the long delay to review this patch.

It looks like working ok. some comments inline.

....................................................
File vdsm/libvirtvm.py
Line 712:                 vcpupin.setAttribute('cpuset', cpuPinning[cpuPin])
Line 713:                 cputune.appendChild(vcpupin)
Line 714:             # Set Vm's niceness via setting cgroup cpu.shares
Line 715:             nice = int(self.conf.get('nice', '0'))
Line 716:             nice = max(min(nice, 19), -19)
19 and -19 would read better as maxNice and minNice constants.
Line 717:             m = self.doc.createElement('shares')
Line 718:             m.appendChild(self.doc.createTextNode(str((20 - nice) * 51)))
Line 719:             cputune.appendChild(m)
Line 720:             self.dom.appendChild(cputune)

Line 714:             # Set Vm's niceness via setting cgroup cpu.shares
Line 715:             nice = int(self.conf.get('nice', '0'))
Line 716:             nice = max(min(nice, 19), -19)
Line 717:             m = self.doc.createElement('shares')
Line 718:             m.appendChild(self.doc.createTextNode(str((20 - nice) * 51)))
I do not completely understand this part, what is 51? why 51? Maybe another talking constant name would help.
Line 719:             cputune.appendChild(m)
Line 720:             self.dom.appendChild(cputune)
Line 721: 
Line 722:         # This hack is for backward compatibility as the libvirt does not allow

--
To view, visit http://gerrit.ovirt.org/6290
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Laszlo Hornyak 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Michal Skrivanek

Laszlo Hornyak has posted comments on this change.

Change subject: Replace calling setSchedulerParameters() with filling its XML description.
......................................................................

Patch Set 2:

oh btw there was a pep8 error after merge that had to be fixed

--
To view, visit http://gerrit.ovirt.org/6290
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Laszlo Hornyak 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Michal Skrivanek

[Ryan Harper]

Ryan Harper has posted comments on this change.

Change subject: Replace calling setSchedulerParameters() with filling its XML description.
......................................................................

Patch Set 3: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 501
Line 502
Line 503
Line 504
Line 505
`man 2 getpriority' says min niceness (most favored for scheduling) is -20.

Line 504:     MAX_NICENESS = 19
Line 505:     MIN_NICENESS = -19
Line 506:     DEFAULT_VM_PRIO = 20
Line 507:     DEFAULT_CGROUP_CPUSHARE = 1024
Line 508:     CPUSHARE_SCALE_FACTOR = DEFAULT_CGROUP_CPUSHARE / DEFAULT_VM_PRIO
integer math loses some share value:
> > > 1024 / 20

51
> > > 1024.0 / 20.0

51.2

This results in the default nice=0 getting 1020 cpu share value, rather than 1024.

Without fixing the min niceness, if nice=-19, then we get a cpu share of 1989, rather than 2048.

nice=19 results in a share value of 51/51.2 ... though it's not clear to me what cpu share of 0 would mean.

I don't know that these issues would have any meaningful effect on scheduler.
Line 509: 
Line 510:     def __init__(self, conf, log):
Line 511:         """
Line 512:         Create the skeleton of a libvirt domain xml

--
To view, visit http://gerrit.ovirt.org/6290
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Laszlo Hornyak 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Ryan Harper

Mark Wu has posted comments on this change.

Change subject: Replace calling setSchedulerParameters() with filling its XML description.
......................................................................

Patch Set 3: (2 inline comments)

....................................................
File vdsm/libvirtvm.py
Line 501
Line 502
Line 503
Line 504
Line 505
This niceness is only used to represent vm's priority in ovirt internal.  Is it necessary to make it comply with linux scheduler's niceness.

Line 504:     MAX_NICENESS = 19
Line 505:     MIN_NICENESS = -19
Line 506:     DEFAULT_VM_PRIO = 20
Line 507:     DEFAULT_CGROUP_CPUSHARE = 1024
Line 508:     CPUSHARE_SCALE_FACTOR = DEFAULT_CGROUP_CPUSHARE / DEFAULT_VM_PRIO
The cpu share value is relative.  It doesn't matter what the absolute value is. The cpu share of all vms are calculated according to the same rule, so I think it should be fine.
Line 509: 
Line 510:     def __init__(self, conf, log):
Line 511:         """
Line 512:         Create the skeleton of a libvirt domain xml

--
To view, visit http://gerrit.ovirt.org/6290
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Laszlo Hornyak 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Ryan Harper

[Ryan Harper]

Ryan Harper has posted comments on this change.

Change subject: Replace calling setSchedulerParameters() with filling its XML description.
......................................................................

Patch Set 3: No score

I support filling out the xml parameters instead of calling the setScheduleParameters, but I'd prefer to avoid the nice-level mapping.  Let's just use raw cpushares values.

I see we've encoded the 'nice' level in the runningVM stats. Maybe switching nice for cgroup cpushares is a different patch.

--
To view, visit http://gerrit.ovirt.org/6290
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Laszlo Hornyak 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Ryan Harper

Mark Wu has abandoned this change.

Change subject: Replace calling setSchedulerParameters() with filling its XML description.
......................................................................

Patch Set 3: Abandoned

--
To view, visit http://gerrit.ovirt.org/6290
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: I211e191022f5a18fa7d97d5a8fb42e10729ddd06
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu 
Gerrit-Reviewer: Doron Fediuck 
Gerrit-Reviewer: Gal Hammer 
Gerrit-Reviewer: Laszlo Hornyak 
Gerrit-Reviewer: Mark Wu 
Gerrit-Reviewer: Michal Skrivanek 
Gerrit-Reviewer: Ryan Harper