Re: [Xen-devel] [PATCH v1 3/5] xen: sched: null: deassign offline vcpus from pcpus

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 8/25/18 1:21 AM, Dario Faggioli wrote:
> If a pCPU has been/is being offlined, we don't want it to be neither
> assigned to any pCPU, nor in the wait list.
> 
> Therefore, when we detect that a vcpu is going offline, remove it from
> both places.

Hmm, this commit message wasn't very informative.

It looks like what you really mean to do is:

> Signed-off-by: Dario Faggioli <dfaggioli@xxxxxxxx>
> ---
> Cc: George Dunlap <george.dunlap@xxxxxxxxxxxxx>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Roger Pau Monne <roger.pau@xxxxxxxxxx>
> ---
>  xen/common/sched_null.c |   43 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/common/sched_null.c b/xen/common/sched_null.c
> index 1426124525..6259f4643e 100644
> --- a/xen/common/sched_null.c
> +++ b/xen/common/sched_null.c
> @@ -361,7 +361,8 @@ static void vcpu_assign(struct null_private *prv, struct 
> vcpu *v,
>      }
>  }
>  
> -static void vcpu_deassign(struct null_private *prv, struct vcpu *v)
> +/* Returns true if a cpu was tickled */
> +static bool vcpu_deassign(struct null_private *prv, struct vcpu *v)
>  {
>      unsigned int bs;
>      unsigned int cpu = v->processor;
> @@ -406,11 +407,13 @@ static void vcpu_deassign(struct null_private *prv, 
> struct vcpu *v)
>                  vcpu_assign(prv, wvc->vcpu, cpu);
>                  cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
>                  spin_unlock(&prv->waitq_lock);
> -                return;
> +                return true;
>              }
>          }
>      }
>      spin_unlock(&prv->waitq_lock);
> +
> +    return false;
>  }
>  
>  /* Change the scheduler of cpu to us (null). */
> @@ -518,6 +521,14 @@ static void null_vcpu_remove(const struct scheduler 
> *ops, struct vcpu *v)
>  
>      lock = vcpu_schedule_lock_irq(v);
>  
> +    /* If offline, the vcpu shouldn't be assigned, nor in the waitqueue */
> +    if ( unlikely(!is_vcpu_online(v)) )
> +    {
> +        ASSERT(per_cpu(npc, v->processor).vcpu != v);
> +        ASSERT(list_empty(&nvc->waitq_elem));
> +        goto out;
> +    }

* Handle the case of an offline vcpu being removed (ASSERTing that it's
neither on a processor nor on the waitqueue)

But wait, isn't this fixing a important regression in patch 2?  If after
patch 2 but before patch 3, a VM is created with offline vcpus, and then
destroyed, won't the offline vcpus reach here neither on the waitlist
nor on a vcpu?

Offlining/onlining vcpus is one thing; but creating and destroying
guests is something different.

>      /* If v is in waitqueue, just get it out of there and bail */
>      if ( unlikely(!list_empty(&nvc->waitq_elem)) )
>      {
> @@ -567,11 +578,31 @@ static void null_vcpu_wake(const struct scheduler *ops, 
> struct vcpu *v)
>  
>  static void null_vcpu_sleep(const struct scheduler *ops, struct vcpu *v)
>  {
> +    struct null_private *prv = null_priv(ops);
> +    unsigned int cpu = v->processor;
> +    bool tickled = false;
> +
>      ASSERT(!is_idle_vcpu(v));
>  
> +    /* We need to special case the handling of the vcpu being offlined */
> +    if ( unlikely(!is_vcpu_online(v)) )
> +    {
> +        struct null_vcpu *nvc = null_vcpu(v);
> +
> +        printk("YYY d%dv%d going down?\n", v->domain->domain_id, v->vcpu_id);
> +        if ( unlikely(!list_empty(&nvc->waitq_elem)) )
> +        {
> +            spin_lock(&prv->waitq_lock);
> +            list_del_init(&nvc->waitq_elem);
> +            spin_unlock(&prv->waitq_lock);
> +        }
> +        else if ( per_cpu(npc, cpu).vcpu == v )
> +            tickled = vcpu_deassign(prv, v);
> +    }

* Handle the unexpected(?) case of a vcpu being put to sleep as(?) it's
offlined

If it's not unexpected, then why the printk?

And if it is unexpected, what is the expected path for a cpu going
offline to be de-assigned from a vcpu (which is what the title seems to
imply this patch is about)?

> +
>      /* If v is not assigned to a pCPU, or is not running, no need to bother 
> */
> -    if ( curr_on_cpu(v->processor) == v )
> -        cpu_raise_softirq(v->processor, SCHEDULE_SOFTIRQ);
> +    if ( likely(!tickled && curr_on_cpu(cpu) == v) )
> +        cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
>  
>      SCHED_STAT_CRANK(vcpu_sleep);
>  }
> @@ -615,12 +646,12 @@ static void null_vcpu_migrate(const struct scheduler 
> *ops, struct vcpu *v,
>       *
>       * In the latter, there is just nothing to do.
>       */
> -    if ( likely(list_empty(&nvc->waitq_elem)) )
> +    if ( likely(per_cpu(npc, v->processor).vcpu == v) )
>      {
>          vcpu_deassign(prv, v);
>          SCHED_STAT_CRANK(migrate_running);
>      }
> -    else
> +    else if ( !list_empty(&nvc->waitq_elem) )
>          SCHED_STAT_CRANK(migrate_on_runq);

* Teach null_vcpu_migrate() that !on_waitqueue != on_vcpu.

It looks like the comment just above this hunk is now out of date:

"v is either assigned to a pCPU, or in the waitqueue."

 -George

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