Re: [Xen-devel] [RFC/RFT][PATCH 0 of 3] rework locking in sched_adjust

[George Dunlap <George.Dunlap@xxxxxxxxxxxxx>]

On Wed, Nov 23, 2011 at 2:55 PM, Dario Faggioli <raistlin@xxxxxxxx> wrote:
> Hi everyone,
>
> This series changes how locks are dealt with while adjusting domains'
> scheduling parameters.
>
> I've done and am still doing tests for credit and credit2, and it's
> surviving to all I threw at it up to now. Unfortunately, I can't test
> the sedf part yet, since it is not working on my test boxes due to other
> issues (which I'm also trying to track down). I'm sending the series out
> anyway, so that at least you can have a look at it, and maybe give a
> spin on the sedf part, if you don't have anything more interesting to
> do. ;-P
>
> Juergen series on xl scheduling support attached to this mail, in the
> form of a single patch, for testing convenience.

Hey Dario,  sorry for the long delay in responding.

Overall the patch series looks good to me.  The only issue I see is
that it looks like you introduce a regression between patch 2 and 3 --
that is, if you apply patches 1 and 2, but not 3, then the lock you
grab in sched_sedf.c:sedf_adjust() won't protect against concurrent
accesses from other vcpus; nor will it correctly handle updates to the
per-vcpu EDOM_INFO() variables.

Regressions in mid-patch series are bad because it can mess up bisection.

I think a better way of breaking it down would be:
* Add scheduler global locks, and have them called in the pluggable
scheduler *_adjust() function.  This is redundant, but shouldn't be
harmful.
* Atomically remove both the pause and the lock in schedule.c, and add
the scheduling locks around the per-vcpu EDOM_INFO variables in
sched_sedf.c, all in one patch.  This makes the patch bigger, but it
avoids a regression.  The two things are related anwyay: the reason
you now need scheduling locks around EDOM_INFO variables is because
you're getting rid of the pausing and the lock in schedule.c.

Thoughts?

 -George

>
> --
>
> xen/common/sched_credit.c  |   10 +++++++---
> xen/common/sched_credit2.c |   21 +++++++++++----------
> xen/common/sched_sedf.c    |  131 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------
> xen/common/schedule.c      |   34 ++--------------------------------
> 4 files changed, 130 insertions(+), 66 deletions(-)
>
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -------------------------------------------------------------------
> Dario Faggioli, http://retis.sssup.it/people/faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
> PhD Candidate, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa (Italy)
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
>

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