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 <x...@yyy.zzz>" then? Cc-ing Lars...
> Signed-off-by: Eslam Elnikety <elnik...@amazon.com>
> Reviewed-by: Leonard Foerster <foers...@amazon.de>
> Reviewed-by: Petre Eftime <epe...@amazon.com>
>
About the patch itself:
Acked-by: Dario Faggioli <dfaggi...@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)
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
