Re: [PATCH v2 2/2] credit: Don't steal vcpus which have yielded

[Juergen Gross <jgross@xxxxxxxx>]

On 21.09.23 14:23, George Dunlap wrote:
> On large systems with many vcpus yielding due to spinlock priority
> inversion, it's not uncommon for a vcpu to yield its timeslice, only
> to be immediately stolen by another pcpu looking for higher-priority
> work.
>
> To prevent this:
>
> * Keep the YIELD flag until a vcpu is removed from a runqueue
>
> * When looking for work to steal, skip vcpus which have yielded
>
> NB that this does mean that sometimes a VM is inserted into an empty
> runqueue; handle that case.
>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxx>
> ---
> Changes since v1:
> - Moved a comment tweak to the right patch
>
> CC: Dario Faggioli <dfaggioli@xxxxxxxx>
> ---
>   xen/common/sched/credit.c | 25 ++++++++++++++-----------
>   1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/xen/common/sched/credit.c b/xen/common/sched/credit.c
> index 5c06f596d2..38a6f6fa6d 100644
> --- a/xen/common/sched/credit.c
> +++ b/xen/common/sched/credit.c
> @@ -298,14 +298,10 @@ __runq_insert(struct csched_unit *svc)
>        * runnable unit if we can.  The next runq_sort will bring it forward
>        * within 30ms if the queue too long. */
>       if ( test_bit(CSCHED_FLAG_UNIT_YIELD, &svc->flags)
> -         && __runq_elem(iter)->pri > CSCHED_PRI_IDLE )
> -    {
> +         && __runq_elem(iter)->pri > CSCHED_PRI_IDLE
> +         && iter->next != runq)
Style

>           iter=iter->next;
>   
> -        /* Some sanity checks */
> -        BUG_ON(iter == runq);
> -    }
> -
>       list_add_tail(&svc->runq_elem, iter);
>   }
>   
> @@ -321,6 +317,11 @@ __runq_remove(struct csched_unit *svc)
>   {
>       BUG_ON( !__unit_on_runq(svc) );
>       list_del_init(&svc->runq_elem);
> +
> +    /*
> +     * Clear YIELD flag when scheduling back in
> +     */
> +    clear_bit(CSCHED_FLAG_UNIT_YIELD, &svc->flags);
>   }
>   
>   static inline void
> @@ -1637,6 +1638,13 @@ csched_runq_steal(int peer_cpu, int cpu, int pri, int 
> balance_step)
>           if ( speer->pri <= pri )
>               break;
>   
> +        /*
> +         * Don't steal a UNIT which has yielded; it's waiting for a
> +         * reason
> +         */
> +        if (test_bit(CSCHED_FLAG_UNIT_YIELD, &speer->flags))
Style

> +            continue;
> +
>           /* Is this UNIT runnable on our PCPU? */
>           unit = speer->unit;
>           BUG_ON( is_idle_unit(unit) );
> @@ -1954,11 +1962,6 @@ static void cf_check csched_schedule(
>           dec_nr_runnable(sched_cpu);
>       }
>   
> -    /*
> -     * Clear YIELD flag before scheduling out
> -     */
> -    clear_bit(CSCHED_FLAG_UNIT_YIELD, &scurr->flags);
> -
>       do {
>           snext = __runq_elem(runq->next);
>   
With the style issues fixed:

Reviewed-by: Juergen Gross <jgross@xxxxxxxx>


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature