From: "Jan Beulich" <JBeulich@suse.com>
To: Dario Faggioli <dario.faggioli@citrix.com>,
Chong Li <lichong659@gmail.com>
Cc: Chong Li <chong.li@wustl.edu>, Sisu Xi <xisisu@gmail.com>,
George Dunlap <george.dunlap@eu.citrix.com>,
xen-devel <xen-devel@lists.xen.org>,
Meng Xu <mengxu@cis.upenn.edu>,
Dagaen Golomb <dgolomb@seas.upenn.edu>
Subject: Re: [PATCH v6 for Xen 4.7 1/4] xen: enable per-VCPU parameter settings for RTDS scheduler
Date: Tue, 08 Mar 2016 04:47:20 -0700 [thread overview]
Message-ID: <56DEC9D802000078000DA5AF@prv-mh.provo.novell.com> (raw)
In-Reply-To: <1457433240.3102.161.camel@citrix.com>
>>> On 08.03.16 at 11:34, <dario.faggioli@citrix.com> wrote:
> On Tue, 2016-03-08 at 02:10 -0700, Jan Beulich wrote:
>> > > > On 07.03.16 at 18:53, <dario.faggioli@citrix.com> 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@lists.xen.org
http://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-03-08 11:47 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-06 17:55 [PATCH v6 for Xen 4.7 0/4] Enable per-VCPU parameter settings for RTDS scheduler Chong Li
2016-03-06 17:55 ` [PATCH v6 for Xen 4.7 1/4] xen: enable " Chong Li
2016-03-07 12:59 ` Jan Beulich
2016-03-07 16:28 ` Chong Li
2016-03-07 16:40 ` Jan Beulich
2016-03-07 17:53 ` Dario Faggioli
2016-03-07 22:16 ` Chong Li
2016-03-08 9:10 ` Jan Beulich
2016-03-08 10:34 ` Dario Faggioli
2016-03-08 11:47 ` Jan Beulich [this message]
2016-03-08 19:09 ` Wei Liu
2016-03-09 16:10 ` Dario Faggioli
2016-03-09 16:38 ` Jan Beulich
2016-03-13 17:05 ` Chong Li
2016-03-14 8:37 ` Jan Beulich
2016-03-14 9:10 ` Dario Faggioli
2016-03-14 9:15 ` Jan Beulich
2016-03-14 10:05 ` Dario Faggioli
2016-03-15 16:22 ` Chong Li
2016-03-15 16:41 ` Dario Faggioli
2016-03-15 17:22 ` Chong Li
2016-03-16 3:14 ` Meng Xu
2016-03-16 3:32 ` Chong Li
2016-03-16 3:43 ` Meng Xu
2016-03-16 8:23 ` Dario Faggioli
2016-03-16 14:37 ` Meng Xu
2016-03-16 14:46 ` Chong Li
2016-03-16 14:53 ` Dario Faggioli
2016-03-16 14:46 ` Chong Li
2016-03-16 14:54 ` Dario Faggioli
2016-03-16 10:48 ` Jan Beulich
2016-03-10 22:35 ` Chong Li
2016-03-10 22:50 ` Wei Liu
2016-03-14 9:07 ` Dario Faggioli
2016-03-06 17:55 ` [PATCH v6 for Xen 4.7 2/4] libxc: " Chong Li
2016-03-08 19:09 ` Wei Liu
2016-03-08 19:32 ` Chong Li
2016-03-08 19:36 ` Wei Liu
2016-03-06 17:55 ` [PATCH v6 for Xen 4.7 3/4] libxl: " Chong Li
2016-03-08 19:12 ` Wei Liu
2016-03-09 0:38 ` Chong Li
2016-03-09 14:01 ` Wei Liu
2016-03-09 17:28 ` Dario Faggioli
2016-03-09 21:57 ` Chong Li
2016-03-09 17:09 ` Dario Faggioli
2016-03-09 17:28 ` Dario Faggioli
2016-03-06 17:55 ` [PATCH v6 for Xen 4.7 4/4] xl: " Chong Li
2016-03-08 19:12 ` Wei Liu
2016-03-08 21:24 ` Chong Li
2016-03-09 14:01 ` Wei Liu
2016-03-09 14:09 ` Wei Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56DEC9D802000078000DA5AF@prv-mh.provo.novell.com \
--to=jbeulich@suse.com \
--cc=chong.li@wustl.edu \
--cc=dario.faggioli@citrix.com \
--cc=dgolomb@seas.upenn.edu \
--cc=george.dunlap@eu.citrix.com \
--cc=lichong659@gmail.com \
--cc=mengxu@cis.upenn.edu \
--cc=xen-devel@lists.xen.org \
--cc=xisisu@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).