Re: [Xen-devel] [PATCH v6 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.03.16 at 18:53, <dario.faggioli@xxxxxxxxxx> wrote:
> On Mon, 2016-03-07 at 09:40 -0700, Jan Beulich wrote:
>> > > > On 07.03.16 at 17:28, <lichong659@xxxxxxxxx> wrote:
>> > On Mon, Mar 7, 2016 at 6:59 AM, Jan Beulich <JBeulich@xxxxxxxx>
>> > wrote:
>> > > 
>> > > > @@ -1163,6 +1173,96 @@ rt_dom_cntl(
>> > > > 
>> > > > +    case XEN_DOMCTL_SCHEDOP_getvcpuinfo:
>> > > > +        if ( guest_handle_is_null(op->u.v.vcpus) )
>> > > > +        {
>> > > > +            rc = -EINVAL;
>> > > Perhaps rather -EFAULT? But then again - what is this check good
>> > > for
>> > > (considering that it doesn't cover other obviously bad handle
>> > > values)?
>> > Dario suggested this in the last post, because vcpus is a handle
>> > and
>> > needs to be validated.
>>
>> Well, as said - the handle being non-null doesn't make it a valid
>> handle. Any validation can be left to copy_{to,from}_guest*()
>> unless you mean to give a null handle some special meaning.
>> 
> IIRC, I was looking at how XEN_SYSCTL_pcitopoinfo is handled, for
> reference, and that has some guest_handle_is_null()==>EINVAL sainity
> checking (in xen/common/sysctl.c), which, when I thought about it, made
> sense to me.
> 
> My reasoning was, sort of:
>  1. if the handle is NULL, no point getting into the somewhat 
>     complicated logic of the while,
>  2. more accurate error reporting: as being passed a NULL handler 
>     looked something we could identify and call invalid, rather than 
>     waiting for the copy to fault.

I think the XEN_SYSCTL_pcitopoinfo was misguided in this respect,
cloning non applicable logic here which returns the number of needed
(array) elements in such a case for a few other operations.

>> > > > +            {
>> > > > +                rc = -EINVAL;
>> > > > +                break;
>> > > > +            }
>> > > > +
>> > > > +            spin_lock_irqsave(&prv->lock, flags);
>> > > > +            svc = rt_vcpu(d->vcpu[local_sched.vcpuid]);
>> > > > +            local_sched.s.rtds.budget = svc->budget /
>> > > > MICROSECS(1);
>> > > > +            local_sched.s.rtds.period = svc->period /
>> > > > MICROSECS(1);
>> > > > +            spin_unlock_irqrestore(&prv->lock, flags);
>> > > > +
>> > > > +            if ( __copy_to_guest_offset(op->u.v.vcpus, index,
>> > > > +                    &local_sched, 1) )
>> > > > +            {
>> > > > +                rc = -EFAULT;
>> > > > +                break;
>> > > > +            }
>> > > > +            if ( (++index > 0x3f) && hypercall_preempt_check()
>> > > > )
>> > > > +                break;
>> > > So how is the caller going to be able to reliably read all vCPU-
>> > > s'
>> > > information for a guest with more than 64 vCPU-s?
>> > In libxc, we re-issue hypercall if the current one is preempted.
>> And with the current code - how does libxc know? (And anyway,
>> this should only be a last resort, if the hypervisor can't by itself
>> arrange for a continuation. If done this way, having a code
>> comment referring to the required caller behavior would seem to
>> be an absolute must.)
>> 
> I definitely agree on commenting.
> 
> About the structure of the code, as said above, I do like
> how XEN_SYSCTL_pcitopoinfo ended up being handled, I think it is a
> great fit for this specific case and, comparing at both this and
> previous version, I do think this one is (bugs apart) looking better.
> 
> I'm sure I said this --long ago-- when discussing v4 (and maybe even
> previous versions), as well as more recently, when reviewing v5, and
> that's why Chong (finally! :-D) did it.
> 
> So, with the comment in place (and with bugs fixed :-)), are you (Jan)
> ok with this being done this way?

Well, this _might_ be acceptable for "get" (since the caller
abandoning the sequence of calls prematurely is no problem),
but for "set" it looks less suitable, as similar abandoning would
leave the guest in some inconsistent / unintended state. The
issue with XEN_SYSCTL_pcitopoinfo was, iirc, the lack of a
good way of encoding the continuation information, and while
that would seem applicable here too I'm not sure now whether
doing it the way it was done was the best choice. Clearly
stating (in the public interface header) that certain normally
input-only fields are volatile would allow the continuation to
be handled without tool stack assistance afaict.

>> > > > +        }
>> > > > +
>> > > > +        if ( !rc && (op->u.v.nr_vcpus != index) )
>> > > > +            op->u.v.nr_vcpus = index;
>> > > I don't think the right side of the && is really necessary /
>> > > useful.
>> > The right side is to check whether the vcpus array is fully
>> > processed.
>> > When it is true and no error occurs (rc == 0), we
>> > update op->u.v.nr_vcpus, which is returned to libxc, and helps xc
>> > function figuring out how many un-processed vcpus should
>> > be taken care of in the next hypercall.
>> Just consider what the contents of op->u.v.nr_vcpus is after
>> this piece of code was executed, once with the full conditional,
>> and another time with the right side of the && omitted.
>> 
> BTW, Chong, I'm not sure this has to do with what Jan is saying, but
> looking again at XEN_SYSCTL_pcitopoinfo, it looks to me you're missing
> copying nr_vcpus back up to the guest (which is actually what makes
> libxc knows whether all vcpus have been processed or now).

Indeed that is why the conditional makes sense there, but not here.
And the copying back is already being taken care of by the caller of
sched_adjust().

Jan

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