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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.02.16 at 12:20, <dario.faggioli@xxxxxxxxxx> wrote:
> 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.

I'm of two minds here: I can see your viewpoint, but considering
this is called "temporarily pin a vcpu" the condition is supposed to
be going away again soon.

>> 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.

Yes indeed.

>> > > @@ -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...

In addition to Jürgen's reply: My main concern here is that on
a bug system this message would get printed for almost every
vCPU in the system, which could end up being a lot of noise.

And there's a similar message on the resume side I think -
perhaps that one should be silenced too.

Jan

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