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

[Dario Faggioli <dfaggioli@xxxxxxxx>]

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@xxxxxxx>" then? Cc-ing Lars...

> Signed-off-by: Eslam Elnikety <elnikety@xxxxxxxxxx>
> Reviewed-by: Leonard Foerster <foersleo@xxxxxxxxx>
> Reviewed-by: Petre Eftime <epetre@xxxxxxxxxx>
>
About the patch itself:

Acked-by: Dario Faggioli <dfaggioli@xxxxxxxx>

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: signature.asc
Description: This is a digitally signed message part

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