Re: [PATCH v3 2/2] xen: credit1: avoid boosting vCPUs being "just" migrated

[George Dunlap <george.dunlap@citrix.com>]

On 12/02/16 16:29, Dario Faggioli wrote:
> Moving a vCPU to a different pCPU means offlining it and
> then waking it up, on the new pCPU. Credit1 grants BOOST
> priority to vCPUs that wakes up, with the aim of improving
> I/O latency. The net effect of this all is that vCPUs get
> boosted when migrating, which shouldn't happen.
> 
> For instance, this causes scheduling anomalies and,
> potentially, performance problems, as reported here:
>   http://lists.xen.org/archives/html/xen-devel/2015-10/msg02851.html
> 
> This patch fixes this by noting down (by means of a flag)
> the fact that the vCPU is about to undergo a migration.
> This way we can tell, later, during a wakeup, whether the
> vCPU is migrating or unblocking, and decide whether or
> not to apply the boosting.
> 
> Note that it is important that atomic-safe bit operations
> are used when manipulating vCPUs' flags. Take the chance
> and add a comment about this.
> 
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>
> ---
> Changes from v2:
>  * test_and_clear() is necessary when accessing svc->flags;
>  * added a comment about such need at the top, where the flags
>    are defined.
> 
> Changes from v1:
>  * rewritten, following suggestion got during review: there
>    are no wakeup flags any longer, and all is done in sched_credit.c
>    by setting a flag in csched_cpu_pick() and testing (and
>    cleating) it in csched_vcpu_wake().
> ---
>  xen/common/sched_credit.c |   30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> index 5708701..597a784 100644
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -63,9 +63,14 @@
>  
>  /*
>   * Flags
> + *
> + * Note that svc->flags (where these flags live) is protected by an
> + * inconsistent set of locks. Therefore atomic-safe bit operations must
> + * be used for accessing it.
>   */
>  #define CSCHED_FLAG_VCPU_PARKED    0x0  /* VCPU over capped credits */
>  #define CSCHED_FLAG_VCPU_YIELD     0x1  /* VCPU yielding */
> +#define CSCHED_FLAG_VCPU_MIGRATING 0x2  /* VCPU may have moved to a new pcpu */
>  
>  
>  /*
> @@ -787,6 +792,16 @@ _csched_cpu_pick(const struct scheduler *ops, struct vcpu *vc, bool_t commit)
>  static int
>  csched_cpu_pick(const struct scheduler *ops, struct vcpu *vc)
>  {
> +    struct csched_vcpu *svc = CSCHED_VCPU(vc);
> +
> +    /*
> +     * We have been called by vcpu_migrate() (in schedule.c), as part
> +     * of the process of seeing if vc can be migrated to another pcpu.
> +     * We make a note about this in svc->flags so that later, in
> +     * csched_vcpu_wake() (still called from vcpu_migrate()) we won't
> +     * get boosted, which we don't deserve as we are "only" migrating.
> +     */
> +    set_bit(CSCHED_FLAG_VCPU_MIGRATING, &svc->flags);
>      return _csched_cpu_pick(ops, vc, 1);
>  }
>  
> @@ -1022,11 +1037,18 @@ csched_vcpu_wake(const struct scheduler *ops, struct vcpu *vc)
>       * more CPU resource intensive VCPUs without impacting overall 
>       * system fairness.
>       *
> -     * The one exception is for VCPUs of capped domains unpausing
> -     * after earning credits they had overspent. We don't boost
> -     * those.
> +     * There are two cases, when we don't want to boost:
> +     *  - VCPUs that are waking up after a migration, rather than
> +     *    after having block;
> +     *  - VCPUs of capped domains unpausing after earning credits
> +     *    they had overspent.
> +     *
> +     * Note that checking whether we are "only" migrating must be
> +     * done up front, as we do not want the clearing of the bit we
> +     * set in csched_cpu_pick() to be short-circuited away.
>       */
> -    if ( svc->pri == CSCHED_PRI_TS_UNDER &&
> +    if ( !test_and_clear_bit(CSCHED_FLAG_VCPU_MIGRATING, &svc->flags)  &&
> +         svc->pri == CSCHED_PRI_TS_UNDER &&
>           !test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )

Sorry to be late reviewing this.

So we always want to clear the 'migrating' flag, regardless of whether
we do anything with boosting.  Would that logic be clearer if we cleared
it as a separate step, storing the result in a local variable?  E.g.:

bool migrating;

...

/* Always clear migrating flag if it's set */
migrating = test_and_clear_bit(...)

if ( !migrating && ...) {
}

Then we wouldn't need the last paragraph in the comment.

That said, this is v3, so if you'd rather just get this in as it is,
then you can have my Acked-by as well.

 -George