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 08.03.16 at 11:34, <dario.faggioli@xxxxxxxxxx> wrote:
> On Tue, 2016-03-08 at 02:10 -0700, Jan Beulich wrote:
>> > > > On 07.03.16 at 18:53, <dario.faggioli@xxxxxxxxxx> wrote:
>> > On Mon, 2016-03-07 at 09:40 -0700, Jan Beulich wrote:
>> > > 
>> > 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.
>> 
> Sorry, I'm not sure I am getting: are you saying that, for _these_
> domctls, we should consider the handle being NULL as a way of the
> caller to ask for the size of the array?

No, I've tried to point out that _when_ such behavior is intended,
the special casing of a null handle is warranted. But not (normally)
in other cases.

>> > 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.
>>
> Are you referring to the fact that, with this interface, the caller has
> the chance to leave intentionally, or that it may happen that not all
> vcpus are updated because of some bug (still in the caller)?
> 
> Well, if it's intentional, or even if the caller is buggy in the sense
> that the code is written in a way that it misses updating some vcpus
> (and if the interface and the behavior is well documented, as you
> request), then one gets what he "wants" (and, in the latter case, it
> wouldn't be too hard to debug and figure out the issue, I think).

Intentional or buggy doesn't matter much - if we can avoid making
ourselves dependent upon user mode code behaving well, I think
we should.

> If it's for bugs (still in the caller) like copy_from_guest_offset()
> faulting because the array is too small, that can happen if using
> continuation too, can't it? And it would still leave the guest in
> similar inconsistent or unintended state, IMO...

True, albeit that's what error indications are for.

> One last point. Of course, since we are talking about bugs, the final
> status is not the one desired, but it's not inconsistent in the sense
> that the guest can't continue running, or crashes, or anything like
> that. It's something like:
>  - you wants all the 32 vcpus of guest A to have these new parameters
>  - due to a bug, you're (for instance) passing me an array with only 
>    16 vcpus parameters
>  - result: onlyt 16 vcpus will have the new parameters.

That was my understanding, yes.

>>  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. 
>>
> As far as I can remember and see, it was being done by means of an
> additional dedicated parameter in the handle (called ti->first_dev in
> that case). Then at some point, you said:
> 
> http://lists.xenproject.org/archives/html/xen-devel/2015-03/msg02623.html 
> "Considering this is a tools only interface, enforcing a not too high
>  limit on num_devs would seem better than this not really clean
>  continuation mechanism. The (tool stack) caller(s) can be made
>  iterate."
> 
> With which I did agree (and I still do :-)), as well as I agree on the
> fact that we basically are in the same situation here.
> 
> Chong tried doing things with continuations for a few rounds, including
> v5, which is here:
> http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg00817.html 
> 
> and he also used an additional field (vcpu_index).
> 
> So, all this being said, my preference stays for the way the code looks
> like in this version (with all the due commenting added). Of course,
> it's your preference that really matters here, me not being the
> maintainer of this code. :-)
> 
> So, how do you prefer Chong to continue doing this?

Well, modeling this after the other sysctl makes it something I
can't reasonably reject. Hence what I've been saying so far were
merely suggestions, as - other than you - I'm not convinced
anymore the model used there was a good one. The more that
here - afaict - no extra fields would be needed: The continuation
encoding would simply consist of op->u.v.nr_vcpus getting
suitably decreased and op->u.v.vcpus suitably bumped.

>> 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.
>>
> Which (sorry, I'm not getting again) I guess is something
> different/more than what was done in v5 (the relevant hunks of which
> I'm pasting at the bottom of this email, for convenience)?

The hunks you had included didn't cover the public interface
header changes. My point here is: Generally we demand that
fields in public interface structures not documented as OUT
would not get modified by a hypercall. Since I'm suggesting to
use both fields to encode continuation information, that rule
would get violated, which therefore needs spelling out in a
comment in the header.

Jan

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