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. Thanks Atsushi SAKAI Daniel Veillard <veillard@redhat.com> wrote:

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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

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. Thanks Atsushi SAKAI Daniel Veillard <veillard@redhat.com> wrote:

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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

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; Thanks Atsushi SAKAI Daniel Veillard <veillard@redhat.com> wrote:

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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

Hi, Daniel Yes, Credit Scheduler just sets 2 parameter only. Please see following code. http://xen.begi.net/xen-unstable/S/1538.html#L701 Thanks Atsushi SAKAI Daniel Veillard <veillard@redhat.com> wrote:

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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

On Wed, Jun 06, 2007 at 06:49:33PM +0900, Atsushi SAKAI wrote:

Hi, Daniel

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

good !

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

Okay, applied in my tree, I will commit later, I'm currently checking the new log patch. I think there is 3 things remaining for the scheduler API: - test on 3.0.3/RHEL-5.0 , I guess it should work too there then adjust those 3 tests against the version - support for the other scheduler SEDF - documentation for all the parameters and what they mean but at least the core functionality is in, thanks, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/