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

On Wed, Nov 23, 2011 at 2:55 PM, Dario Faggioli <raistlin@linux.it>
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@lists.xensource.com
> http://lists.xensource.com/xen-devel
>

On Tue, Dec 6, 2011 at 1:24 PM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> Hey Dario,  sorry for the long delay in responding.
>
Hi, and no problem at all :-)
> Regressions in mid-patch series are bad because it can mess up bisection.
>
I strongly agree. I didn''t think much about that for this series since
the mechanism
I was trying to fix was (especially for sedf) already broken in
various ways but,
indeed, patches could be better split.
> 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.
>
Ok, I''ll go for this.
> 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.
>
Yeah, what I was trying to do was leaving a clear footstep about why
pausing was going and, yes, also making patches smaller, but again, I
see your point and will follow your advice. :-)

Thanks and Regards,
Dario

-- 
<<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)