[PATCH] sched/credit: avoid priority boost for capped domains when unpark

[PATCH] sched/credit: avoid priority boost for capped domains when unpark

[Eslam Elnikety]

When unpausing a capped domain, the scheduler currently clears the
CSCHED_FLAG_VCPU_PARKED flag before vcpu_wake(). This, in turn, causes the
vcpu_wake to set CSCHED_PRI_TS_BOOST, resulting in an unfair credit boost. The
comment around the changed lines already states that clearing the flag should
happen AFTER the unpause. This bug was introduced in commit be650750945
"credit1: Use atomic bit operations for the flags structure".

Original patch author credit: Xi Xiong.

Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
Reviewed-by: Leonard Foerster <foersleo@amazon.de>
Reviewed-by: Petre Eftime <epetre@amazon.com>
---
 xen/common/sched_credit.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 3abe20def8..8eb1aba12a 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1538,7 +1538,7 @@ csched_acct(void* dummy)
                 svc->pri = CSCHED_PRI_TS_UNDER;
 
                 /* Unpark any capped domains whose credits go positive */
-                if ( test_and_clear_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )
+                if ( test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )
                 {
                     /*
                      * It's important to unset the flag AFTER the unpause()
@@ -1547,6 +1547,8 @@ csched_acct(void* dummy)
                      */
                     SCHED_STAT_CRANK(vcpu_unpark);
                     vcpu_unpause(svc->vcpu);
+                    /* Now clear the PARKED flag */
+                    clear_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags);
                 }
 
                 /* Upper bound on credits means VCPU stops earning */
-- 
2.15.3.AMZN


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH] sched/credit: avoid priority boost for capped domains when unpark

[Eslam Elnikety]

When unpausing a capped domain, the scheduler currently clears the
CSCHED_FLAG_VCPU_PARKED flag before vcpu_wake(). This, in turn, causes the
vcpu_wake to set CSCHED_PRI_TS_BOOST, resulting in an unfair credit boost. The
comment around the changed lines already states that clearing the flag should
happen AFTER the unpause. This bug was introduced in commit be650750945
"credit1: Use atomic bit operations for the flags structure".

Original patch author credit: Xi Xiong.

Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
Reviewed-by: Leonard Foerster <foersleo@amazon.de>
Reviewed-by: Petre Eftime <epetre@amazon.com>
---
 xen/common/sched_credit.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 3abe20def8..8eb1aba12a 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1538,7 +1538,7 @@ csched_acct(void* dummy)
                 svc->pri = CSCHED_PRI_TS_UNDER;
 
                 /* Unpark any capped domains whose credits go positive */
-                if ( test_and_clear_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )
+                if ( test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )
                 {
                     /*
                      * It's important to unset the flag AFTER the unpause()
@@ -1547,6 +1547,8 @@ csched_acct(void* dummy)
                      */
                     SCHED_STAT_CRANK(vcpu_unpark);
                     vcpu_unpause(svc->vcpu);
+                    /* Now clear the PARKED flag */
+                    clear_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags);
                 }
 
                 /* Upper bound on credits means VCPU stops earning */
-- 
2.15.3.AMZN


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] sched/credit: avoid priority boost for capped domains when unpark

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2399 bytes --]

On Fri, 2019-05-03 at 15:38 +0000, Eslam Elnikety wrote:
> When unpausing a capped domain, the scheduler currently clears the
> CSCHED_FLAG_VCPU_PARKED flag before vcpu_wake(). This, in turn,
> causes the
> vcpu_wake to set CSCHED_PRI_TS_BOOST, resulting in an unfair credit
> boost. The
> comment around the changed lines already states that clearing the
> flag should
> happen AFTER the unpause. This bug was introduced in commit
> be650750945
> "credit1: Use atomic bit operations for the flags structure".
> 
> Original patch author credit: Xi Xiong.
> 
Mmm... I'm not an expert of these things, but doesn't this means we
need a "Signed-off-by: Xi Xiong <xxx@yyy.zzz>" then? Cc-ing Lars...

> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
> Reviewed-by: Leonard Foerster <foersleo@amazon.de>
> Reviewed-by: Petre Eftime <epetre@amazon.com>
>
About the patch itself:

Acked-by: Dario Faggioli <dfaggioli@suse.com>

With just one suggestion...

> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1538,7 +1538,7 @@ csched_acct(void* dummy)
>                  svc->pri = CSCHED_PRI_TS_UNDER;
>  
>                  /* Unpark any capped domains whose credits go
> positive */
> -                if ( test_and_clear_bit(CSCHED_FLAG_VCPU_PARKED,
> &svc->flags) )
> +                if ( test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags)
> )
>                  {
>                      /*
>                       * It's important to unset the flag AFTER the
> unpause()
> @@ -1547,6 +1547,8 @@ csched_acct(void* dummy)
>                       */
>                      SCHED_STAT_CRANK(vcpu_unpark);
>                      vcpu_unpause(svc->vcpu);
> +                    /* Now clear the PARKED flag */
> +                    clear_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags);
>
I don't think adding the comment here is necessary. The one which is
already present, explains things well enough, and this one does not add
much.

Acked-by stands anyway, but I'd prefer it to be removed (which I think
could be done when committing the patch?).

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] sched/credit: avoid priority boost for capped domains when unpark

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2399 bytes --]

On Fri, 2019-05-03 at 15:38 +0000, Eslam Elnikety wrote:
> When unpausing a capped domain, the scheduler currently clears the
> CSCHED_FLAG_VCPU_PARKED flag before vcpu_wake(). This, in turn,
> causes the
> vcpu_wake to set CSCHED_PRI_TS_BOOST, resulting in an unfair credit
> boost. The
> comment around the changed lines already states that clearing the
> flag should
> happen AFTER the unpause. This bug was introduced in commit
> be650750945
> "credit1: Use atomic bit operations for the flags structure".
> 
> Original patch author credit: Xi Xiong.
> 
Mmm... I'm not an expert of these things, but doesn't this means we
need a "Signed-off-by: Xi Xiong <xxx@yyy.zzz>" then? Cc-ing Lars...

> Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
> Reviewed-by: Leonard Foerster <foersleo@amazon.de>
> Reviewed-by: Petre Eftime <epetre@amazon.com>
>
About the patch itself:

Acked-by: Dario Faggioli <dfaggioli@suse.com>

With just one suggestion...

> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -1538,7 +1538,7 @@ csched_acct(void* dummy)
>                  svc->pri = CSCHED_PRI_TS_UNDER;
>  
>                  /* Unpark any capped domains whose credits go
> positive */
> -                if ( test_and_clear_bit(CSCHED_FLAG_VCPU_PARKED,
> &svc->flags) )
> +                if ( test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags)
> )
>                  {
>                      /*
>                       * It's important to unset the flag AFTER the
> unpause()
> @@ -1547,6 +1547,8 @@ csched_acct(void* dummy)
>                       */
>                      SCHED_STAT_CRANK(vcpu_unpark);
>                      vcpu_unpause(svc->vcpu);
> +                    /* Now clear the PARKED flag */
> +                    clear_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags);
>
I don't think adding the comment here is necessary. The one which is
already present, explains things well enough, and this one does not add
much.

Acked-by stands anyway, but I'd prefer it to be removed (which I think
could be done when committing the patch?).

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] sched/credit: avoid priority boost for capped domains when unpark

[Lars Kurth]

> On 3 May 2019, at 10:48, Dario Faggioli <dfaggioli@suse.com> wrote:
> 
> On Fri, 2019-05-03 at 15:38 +0000, Eslam Elnikety wrote:
>> When unpausing a capped domain, the scheduler currently clears the
>> CSCHED_FLAG_VCPU_PARKED flag before vcpu_wake(). This, in turn,
>> causes the
>> vcpu_wake to set CSCHED_PRI_TS_BOOST, resulting in an unfair credit
>> boost. The
>> comment around the changed lines already states that clearing the
>> flag should
>> happen AFTER the unpause. This bug was introduced in commit
>> be650750945
>> "credit1: Use atomic bit operations for the flags structure".
>> 
>> Original patch author credit: Xi Xiong.
>> 
> Mmm... I'm not an expert of these things, but doesn't this means we
> need a "Signed-off-by: Xi Xiong <xxx@yyy.zzz>" then? Cc-ing Lars...

As far as I can tell from a quick search on xen-devel@ Xi Xiong is or 
was an Amazon employee so a signed-off-by is not strictly necessary
but you may want to say something like.

Original patch author credit: Xi Xiong of Amazon

Lars

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] sched/credit: avoid priority boost for capped domains when unpark

[Lars Kurth]

> On 3 May 2019, at 10:48, Dario Faggioli <dfaggioli@suse.com> wrote:
> 
> On Fri, 2019-05-03 at 15:38 +0000, Eslam Elnikety wrote:
>> When unpausing a capped domain, the scheduler currently clears the
>> CSCHED_FLAG_VCPU_PARKED flag before vcpu_wake(). This, in turn,
>> causes the
>> vcpu_wake to set CSCHED_PRI_TS_BOOST, resulting in an unfair credit
>> boost. The
>> comment around the changed lines already states that clearing the
>> flag should
>> happen AFTER the unpause. This bug was introduced in commit
>> be650750945
>> "credit1: Use atomic bit operations for the flags structure".
>> 
>> Original patch author credit: Xi Xiong.
>> 
> Mmm... I'm not an expert of these things, but doesn't this means we
> need a "Signed-off-by: Xi Xiong <xxx@yyy.zzz>" then? Cc-ing Lars...

As far as I can tell from a quick search on xen-devel@ Xi Xiong is or 
was an Amazon employee so a signed-off-by is not strictly necessary
but you may want to say something like.

Original patch author credit: Xi Xiong of Amazon

Lars

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] sched/credit: avoid priority boost for capped domains when unpark

[Elnikety, Eslam]

[-- Attachment #1.1: Type: text/plain, Size: 1354 bytes --]


On 3. May 2019, at 20:56, Lars Kurth <lars.kurth.xen@gmail.com<mailto:lars.kurth.xen@gmail.com>> wrote:



On 3 May 2019, at 10:48, Dario Faggioli <dfaggioli@suse.com<mailto:dfaggioli@suse.com>> wrote:

On Fri, 2019-05-03 at 15:38 +0000, Eslam Elnikety wrote:
When unpausing a capped domain, the scheduler currently clears the
CSCHED_FLAG_VCPU_PARKED flag before vcpu_wake(). This, in turn,
causes the
vcpu_wake to set CSCHED_PRI_TS_BOOST, resulting in an unfair credit
boost. The
comment around the changed lines already states that clearing the
flag should
happen AFTER the unpause. This bug was introduced in commit
be650750945
"credit1: Use atomic bit operations for the flags structure".

Original patch author credit: Xi Xiong.

Mmm... I'm not an expert of these things, but doesn't this means we
need a "Signed-off-by: Xi Xiong <xxx@yyy.zzz<mailto:xxx@yyy.zzz>>" then? Cc-ing Lars...

As far as I can tell from a quick search on xen-devel@ Xi Xiong is or
was an Amazon employee so a signed-off-by is not strictly necessary
but you may want to say something like.

Original patch author credit: Xi Xiong of Amazon

Lars

Thanks for the prompt responses, Lars and Dario.

Indeed. Xi was with Amazon. I will adjust the commit message accordingly. (I will also omit the additional comment as pointed out by Dario).

[-- Attachment #1.2: Type: text/html, Size: 6898 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] sched/credit: avoid priority boost for capped domains when unpark

[Elnikety, Eslam]

[-- Attachment #1.1: Type: text/plain, Size: 1354 bytes --]


On 3. May 2019, at 20:56, Lars Kurth <lars.kurth.xen@gmail.com<mailto:lars.kurth.xen@gmail.com>> wrote:



On 3 May 2019, at 10:48, Dario Faggioli <dfaggioli@suse.com<mailto:dfaggioli@suse.com>> wrote:

On Fri, 2019-05-03 at 15:38 +0000, Eslam Elnikety wrote:
When unpausing a capped domain, the scheduler currently clears the
CSCHED_FLAG_VCPU_PARKED flag before vcpu_wake(). This, in turn,
causes the
vcpu_wake to set CSCHED_PRI_TS_BOOST, resulting in an unfair credit
boost. The
comment around the changed lines already states that clearing the
flag should
happen AFTER the unpause. This bug was introduced in commit
be650750945
"credit1: Use atomic bit operations for the flags structure".

Original patch author credit: Xi Xiong.

Mmm... I'm not an expert of these things, but doesn't this means we
need a "Signed-off-by: Xi Xiong <xxx@yyy.zzz<mailto:xxx@yyy.zzz>>" then? Cc-ing Lars...

As far as I can tell from a quick search on xen-devel@ Xi Xiong is or
was an Amazon employee so a signed-off-by is not strictly necessary
but you may want to say something like.

Original patch author credit: Xi Xiong of Amazon

Lars

Thanks for the prompt responses, Lars and Dario.

Indeed. Xi was with Amazon. I will adjust the commit message accordingly. (I will also omit the additional comment as pointed out by Dario).

[-- Attachment #1.2: Type: text/html, Size: 6898 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel