Re: [Xen-devel] [PATCH] scheduler rate controller

["Lv, Hui" <hui.lv@xxxxxxxxx>]

Hi George,

Thank you very much for your feedbacks.
I have finished the measurement work based on the delay method. From the 
comparable results, 1ms-delay can do as well as SRC patch to gain significant 
performance boost without obvious drawbacks.

1. Basically, the "delay method" can achieve nearly the same benefits as my 
previous SRC patch, 11% overall performance boost for SPECvirt than original 
credit scheduler.
2. I have tried 1ms delay and 10ms delay, there is no big difference between 
these two configurations. (1ms is enough to achieve a good performance)
3. I have compared different load level response time/latency (low, high, 
peak), "delay method" didn't bring very much Qos increase.
4. 1ms delay can reduce 30% context switch at peak performance, where produces 
the benefits.

You can find the raw data from the attached excel file.
The attached credit_1.diff patch works stable at my side.


> Looks fine overall.  One issue with the patch is that it will not only fail to
> preempt for a higher-priority vcpu, it will also fail to preempt for tasklets.
> Tasklet work must be done immediately.  Perhaps we can add
> "!tasklet_work_scheduled" to the list of conditions for

Yes, added "!tasklet_work_scheduled". I have done the experiments to compare 
with/without this, there is not big difference. 
> 
> Why did you change "MICROSECS" to "MILLISECS" when calculating timeslice?
> In this case, it will set the timeslice to a full second!
> Not what we want...
Sorry , it's my typo, I have changed

> From a software maintenance perspective, I'm not a fan of early returns from
> functions.  I think it's too easy not to notice that there's a different 
> return
> path.  In this case, I think I'd prefer adding a label, and using "goto out;"
> instead.

Followed this code style.

> If you were using mercurial queues, you could put this after the last one, 
> and it
> would be easier to see the proposed "adaptive" part of the code. :-)
> 
> Hypervisors are very complicated; it's best to keep things as absolutely 
> simple
> as possible.  This kind of mechanism is exactly the sort of thing that makes 
> it
> very hard to predict what will happen.  I think unless you can show that it 
> adds
> a significant benefit, it's better just to use the min timeslice.

In fact, I have tested the results with 1ms delay and 10ms delay, there is no 
significant performance improvement. It means, even though we select adaptive 
method, there is also no significant performance boost with comparison to 1ms.
1ms is good enough insofar. 

> Regarding this particular code, a couple of items, just for feedback:
> * All of the ratelimiting code and data structures should be in the pluggable
> scheduler, not in common code.
Agreed.


> an adaptive solution, you'd need to make a per-cpu variable with the per-cpu
> rate scheduling; and then set it to the global variable (which is the user
> configuration).

It seems no need to make it adaptive now :)

> I've taken your two patches, given them the proper formating, and made them
> into a patch series (the first adding the ratelimiting, the second adding the
> adaptive bit); they are attached.  You should be able to easily pull them 
> into a
> mercurial patchqueue using "hg qimport /path/to/patches/*.diff".  In the
> future, I will not look at any patches which do not apply using either "patch 
> -p1"
> or "hg qimport."

Thanks for the coach to submit patch in a right way.
> 
>  -George

Attachment: credit_1.diff
Description: credit_1.diff

Attachment: scheduler comparison.xlsx
Description: scheduler comparison.xlsx

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel