Re: [Xen-devel] [PATCH] xen: add hypercall option to temporarily pin a vcpu

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2016-02-26 at 12:14 +0100, Juergen Gross wrote:
> On 26/02/16 11:39, Jan Beulich wrote:
> > 
> > > @@ -670,7 +676,13 @@ int cpu_disable_scheduler(unsigned int cpu)
> > >              if ( cpumask_empty(&online_affinity) &&
> > >                   cpumask_test_cpu(cpu, v->cpu_hard_affinity) )
> > >              {
> > > -                printk(XENLOG_DEBUG "Breaking affinity for
> > > %pv\n", v);
> > > +                if ( v->affinity_broken )
> > > +                {
> > > +                    /* The vcpu is temporarily pinned, can't
> > > move it. */
> > > +                    vcpu_schedule_unlock_irqrestore(lock, flags,
> > > v);
> > > +                    ret = -EBUSY;
> > > +                    continue;
> > > +                }
> > So far the function can only return 0 or -EAGAIN. By using
> > "continue"
> > here you will make it impossible for the caller to reliably
> > determine
> > whether possibly both things failed. Despite -EBUSY being a logical
> > choice here, I think you'd better use -EAGAIN here too. And it
> > needs
> > to be determined whether continuing the loop in this as well as the
> > pre-existing cases is actually the right thing to do.
> EBUSY vs. EAGAIN: by returning EAGAIN I would signal to Xen tools
> that
> the hypervisor is currently not able to do the desired operation
> (especially removing a cpu from a cpupool), but the situation will
> change automatically via scheduling. EBUSY will stop retries in Xen
> tools and this is want I want here: I can't be sure the situation
> will change soon.
> 
I agree with this.

> Regarding continuation of the loop: I think you are right in the
> EBUSY case: I should break out of the loop. I should not do so in the
> EAGAIN case as I want to remove as many vcpus from the physical cpu
> as
> possible without returning to the Xen tools in between.
> 
And with this too.

And I think that, if we indeed break out of the loop on EBUSY, that
will also make it possible to figure out properly what actually went
wrong, so it should be fine from that point of view as well.

> > > @@ -679,6 +691,8 @@ int cpu_disable_scheduler(unsigned int cpu)
> > >                      v->affinity_broken = 1;
> > >                  }
> > >  
> > > +                printk(XENLOG_DEBUG "Breaking affinity for
> > > %pv\n", v);
> > Wouldn't it be even better to make this the "else" to the
> > preceding if(), since in the suspend case this is otherwise going
> > to be printed for every vCPU not currently running on pCPU0?
> Yes, I'll change it.
> 
On this, can (either of) you elaborate a bit more? I don't think I'm
following...

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel