Re: [Xen-devel] credit scheduler svc->flags access race?

[George Dunlap <dunlapg@xxxxxxxxx>]

On Fri, Dec 30, 2011 at 9:55 PM, Liu.yi <liu.yi24@xxxxxxxxxx> wrote:
> hi,all
> In my HP DL380G7 server, csched_vcpu_yield and csched_acct may access
> svc->flags at the same time, when this happens vms stop running because
> csched_vcpu_yield overwrites svc->flags which csched_acct set to
> CSCHED_FLAG_VCPU_PARKED (My vm's schedule cap values is not 0).
> Vms run fine if I modified schedule_credit.c as follows:
>
> --- xen/common/sched_credit.c   2010-12-10 10:19:45.000000000 +0800
> +++ ../../xen-4.1.0/xen/common/sched_credit.c   2010-12-31

Liu,

Thanks for the patch.  Unfortunately it doesn't apply for me.  It
needs to be able to apply using either "patch -p1 < patch_file" or "hg
qimport patch_file".  There are instructions on how to make such a
patch here:

http://wiki.xen.org/wiki/SubmittingXenPatches

Just from looking at it: You're right, the svc->flags variable is
modified while holding the vcpu scheduling lock in the case of
vcpu_yield, but the scheduler private lock in the case of
csched_acct().  It looks like your solution makes the update atomic --
let me see if that's the best thing.  I think the updates shouldn't be
too frequent, so it might be easier than trying to sort out locking.

I'll take a look at changing the locking and get back with you.

 -George

> 10:47:39.000000000 +0800
> @@ -135,7 +135,7 @@ struct csched_vcpu {
>     struct vcpu *vcpu;
>     atomic_t credit;
>     s_time_t start_time;   /* When we were scheduled (used for credit) */
> -    uint16_t flags;
> +    uint32_t flags;
>     int16_t pri;
>  #ifdef CSCHED_STATS
>     struct {
> @@ -787,7 +787,7 @@ csched_vcpu_yield(const struct scheduler
>     if ( !sched_credit_default_yield )
>     {
>         /* Let the scheduler know that this vcpu is trying to yield */
> -        sv->flags |= CSCHED_FLAG_VCPU_YIELD;
> +        set_bit(1, &sv->flags);
>     }
>  }
>
> @@ -1086,7 +1086,7 @@ csched_acct(void* dummy)
>                 {
>                     CSCHED_STAT_CRANK(vcpu_park);
>                     vcpu_pause_nosync(svc->vcpu);
> -                    svc->flags |= CSCHED_FLAG_VCPU_PARKED;
> +                    set_bit(0, &svc->flags);
>                 }
>
>                 /* Lower bound on credits */
> @@ -1111,7 +1111,7 @@ csched_acct(void* dummy)
>                      */
>                     CSCHED_STAT_CRANK(vcpu_unpark);
>                     vcpu_unpause(svc->vcpu);
> -                    svc->flags &= ~CSCHED_FLAG_VCPU_PARKED;
> +                    clear_bit(0, &svc->flags);
>                 }
>
>                 /* Upper bound on credits means VCPU stops earning */
> @@ -1337,7 +1337,7 @@ csched_schedule(
>      * Clear YIELD flag before scheduling out
>      */
>     if ( scurr->flags & CSCHED_FLAG_VCPU_YIELD )
> -        scurr->flags &= ~(CSCHED_FLAG_VCPU_YIELD);
> +        clear_bit(1, &scurr->flags);
>
> Are these modification correct? Another interesting thing is that vms run
> fine on other server even if these vms's credit cap value is not 0. I don't
> know why.
> My xen version is 4.1.0, dom0 kernel version is 2.6.32.41 from jerry's git a
> few months before.
>
> Thanks
>
> liuyi
>
>
> --
> View this message in context: 
> http://xen.1045712.n5.nabble.com/credit-scheduler-svc-flags-access-race-tp5111504p5111504.html
> Sent from the Xen - Dev mailing list archive at Nabble.com.
>
> _______________________________________________
> 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