Welcome! Log In Create A New Profile Recent Messages

Advanced

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

Posted by Liu.yi 
Liu.yi
[Xen-devel] credit scheduler svc->flags access race?
December 31, 2011 06:59PM
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
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: [xen.1045712.n5.nabble.com]
Sent from the Xen - Dev mailing list archive at Nabble.com.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
[lists.xensource.com]
Keir Fraser
Re: [Xen-devel] credit scheduler svc->flags access race?
January 03, 2012 11:36PM
On 31/12/2011 02:55, "Liu.yi" <liu.yi24@zte.com.cn> 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:

Cc'ing George Dunlap. This is probably a good bug fix.

-- Keir

> --- 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
> 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:
> [xen.1045712.n5.nabble.com]
> 504p5111504.html
> Sent from the Xen - Dev mailing list archive at Nabble.com.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> [lists.xensource.com]



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
[lists.xensource.com]
George Dunlap
Re: [Xen-devel] credit scheduler svc->flags access race?
January 05, 2012 03:06PM
On Fri, Dec 30, 2011 at 9:55 PM, Liu.yi <liu.yi24@zte.com.cn> 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:

[wiki.xen.org]

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: [xen.1045712.n5.nabble.com]
> Sent from the Xen - Dev mailing list archive at Nabble.com.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> [lists.xensource.com]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
[lists.xensource.com]
Hi, Keir and George, thanks for your concerns.

Access race to svc->flags seems inevitably when csched_acct calls
vcpu_pause_nosync:
vm excutes PAUSE instruction.
hypercalls call csched_vcpu_yield.

I had think about taking csched_private->lock in csched_vcpu_yield, but
csched_acct takes this lock for a long time so that csched_vcpu_yield may
been blocked for long time too, so I chooes set_bit and clear_bit.

Atomic operations using LOCK prefix (like set_bit and clear_bit) will block
all the physical cpus which excute memory access. Does this fact implies
that spin locks are more efficient when their's granularity is small?

Sorry for my poor english.

Liuyi

--
View this message in context: [xen.1045712.n5.nabble.com]
Sent from the Xen - Dev mailing list archive at Nabble.com.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
[lists.xensource.com]
Jan Beulich
Re: [Xen-devel] credit scheduler svc->flags access race?
January 06, 2012 12:01AM
>>> On 05.01.12 at 03:36, "Liu.yi" <liu.yi24@zte.com.cn> wrote:
> Atomic operations using LOCK prefix (like set_bit and clear_bit) will block
> all the physical cpus which excute memory access.

... to that same cache line.

> Does this fact implies
> that spin locks are more efficient when their's granularity is small?

No - after all, acquiring a spin lock unavoidably implies using a LOCKed
instruction.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
[lists.xensource.com]
Understood, thanks.

--
View this message in context: [xen.1045712.n5.nabble.com]
Sent from the Xen - Dev mailing list archive at Nabble.com.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
[lists.xensource.com]
Sorry, you do not have permission to post/reply in this forum.