On Mon, Jun 04, 2007 at 05:17:56PM +0900, Atsushi SAKAI wrote: > Hi, > > Is there any plan to apply scheduler controller patch? > > https://www.redhat.com/archives/libvir-list/2007-May/msg00291.html > https://www.redhat.com/archives/libvir-list/2007-May/msg00295.html In theory, yes. I'm not enthusiastic about the new APIs but considering that there is no unity in the various existing schedulers, we have to make an API which stay open like this and will be able to evolve to future needs. I read the patch quickly, and I need to do a more thorough review before applying it. I would feel better too if the case XEN_SCHEDULER_SEDF wasn't just made of TODOs "/* TODO: Implement for Xen/SEDF */" in most places, I'm afraid that since it's the old scheduler you have little interest in implementing those, and it's likely nobody else will so applying the patch means adding TODOs to the code nearly forever. So in practice I nedd to make a second pass on it, and if you could provide a separate patch cleaning up the Xen/SEDF in the meantime that would be perfect :-) thanks, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

On Mon, Jun 04, 2007 at 06:28:26PM +0900, Atsushi SAKAI wrote: > Hi, Daniel > > I plan to post Xen/SEDF patch. > Anyway I hope current patch will apply first, > Since coding frame of this command is fixed previously. I'm going though your code right now. I find something suspicious: in xenHypervisorGetSchedulerParameters you check /* * Support only dom_interface_version >=5 * (Xen3.1.0 or later) */ if (dom_interface_version < 5) { virXenError(VIR_ERR_NO_XEN, "xenHypervisorGetSchedulerParameters unsupported in dom interface < 5", 0); return -1; } And I don't see why ! The two next hypercalls, the system one to get the scheduler ID and the domain one to get the scheduler parameters both exist on the xen-3.03 available for example in RHEL-5.0. So why exit with an error there ? Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

On Tue, Jun 05, 2007 at 11:23:43AM +0900, Atsushi SAKAI wrote: > Hi, Daniel > > I confirmed to work Xen3.1.0 only. > And not run on Xen3.0.4.(I guess domctl hypercall problem.) > And not tested yet for Xen3.0.3. > > This is the reason I limit to work this command. Okay, I will double check on a 3.0.3 box. I also surprized in xenHypervisorSetSchedulerParameters(), in the XEN_SCHEDULER_CREDIT, you loop on the params to extract the values for weight and cap, but you never error: - if you can't interpret one of the parameters - if no parameters is actually found. The memset at the beginning set to 0 those 2 values. I wonder what the result of calling the hypercall with (0, 0) means (or if one of the values is 0), I'm afraid it could actually hang the domain, without any error being raise if there is a missing parameter. I could see 2 ways to fix this: - either prepopulate the weight and cap values by an earlier hypercall XEN_V2_OP_SCHEDULER - or fail if while going through the loop we don't get the 2 values. The second option seems nicer. The current error handling there is not okay, especially since there is no indication in the documentation on what the parameters needed might be (i.e. basically the only way to get this to work for someone else is by trial and no error is ever raised, so that really need to be fixed). Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

On Tue, Jun 05, 2007 at 07:42:31PM +0900, Atsushi SAKAI wrote: > Hi, Daniel > > Thank you for reviewing. > > About xenHypervisorSetSchedulerParameters() in case of credit scheduler. > I just set not changed parameter at first. > I guess no problem on this. > Any problem? > > + /* credit scheduler parameters > + * following values do not change the parameters > + */ > + op_dom.u.getschedinfo.u.credit.weight = 0; > + op_dom.u.getschedinfo.u.credit.cap = (uint16_t)~0U; yes I have seen that, but I wonder what happens when only one parameter is not 0, does the hypercall set only that parameter ? That does no prevent pushing that first version but that something which should be double checked to avoid catastrophic behaviour on edge cases. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

Hi, Is there any plan to apply scheduler controller patch? https://www.redhat.com/archives/libvir-list/2007-May/msg00291.html https://www.redhat.com/archives/libvir-list/2007-May/msg00295.html

Hi, Daniel Thank you for committing scheduler API patch. And Thanks for review the code. It works fine and code looks good to me.

And I found nice-to-have fix on comments.:-)