From: Jennifer Herbert <Jennifer.Herbert@citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
PaulDurrant <paul.durrant@citrix.com>,
Wei Liu <wei.liu2@citrix.com>,
Ian Jackson <ian.jackson@eu.citrix.com>,
Xen-devel <xen-devel@lists.xen.org>
Subject: Re: [PATCH v2] dm_op: Add xendevicemodel_modified_memory_bulk.
Date: Thu, 23 Mar 2017 14:54:01 +0000 [thread overview]
Message-ID: <58D3E189.40408@citrix.com> (raw)
In-Reply-To: <58D396F10200007800146A01@prv-mh.provo.novell.com>
On 23/03/17 08:35, Jan Beulich wrote:
>>>> On 22.03.17 at 20:55, <Jennifer.Herbert@citrix.com> wrote:
>>>> --- a/xen/arch/x86/hvm/dm.c
>>>> +++ b/xen/arch/x86/hvm/dm.c
>>>> @@ -119,56 +119,89 @@ static int set_isa_irq_level(struct domain *d, uint8_t isa_irq,
>>>> }
>>>>
>>>> static int modified_memory(struct domain *d,
>>>> - struct xen_dm_op_modified_memory *data)
>>>> + struct xen_dm_op_modified_memory *header,
>>>> + xen_dm_op_buf_t* buf)
>>>> {
>>>> - xen_pfn_t last_pfn = data->first_pfn + data->nr - 1;
>>>> - unsigned int iter = 0;
>>>> + /* Process maximum of 256 pfns before checking for continuation */
>>>> + const unsigned int cont_check_interval = 0xff;
>>> Iirc the question was asked on v1 already: 256 (as the comment
>>> says) or 255 (as the value enforces)?
>> aw shucks! I had it as 255, and Paul said "255 or 256?" and I changed
>> it to 256, without thinking, assuming an off by one error, and that he
>> was right. But, with a seconds thought, it should be 255, and Paul was
>> actually referring to a comment later in the code, which was 256.
> Yet batches of 256 would seem a little more "usual".
Ok, I can set cont_check_interval = 0x100; and leave the comment as 256.
>>>> --- a/xen/include/public/hvm/dm_op.h
>>>> +++ b/xen/include/public/hvm/dm_op.h
>>>> @@ -237,13 +237,24 @@ struct xen_dm_op_set_pci_link_route {
>>>> * XEN_DMOP_modified_memory: Notify that a set of pages were modified by
>>>> * an emulator.
>>>> *
>>>> - * NOTE: In the event of a continuation, the @first_pfn is set to the
>>>> - * value of the pfn of the remaining set of pages and @nr reduced
>>>> - * to the size of the remaining set.
>>>> + * DMOP buf 1 contains an array of xen_dm_op_modified_memory_extent with
>>>> + * @nr_extent entries. The value of @nr_extent will be reduced on
>>>> + * continuation.
>>>> + *
>>>> + * @pfns_processed is used for continuation, and not intended to be usefull
>>>> + * to the caller. It gives the number of pfns already processed (and can
>>>> + * be skipped) in the last extent. This should always be set to zero.
>>>> */
>>>> #define XEN_DMOP_modified_memory 11
>>>>
>>>> struct xen_dm_op_modified_memory {
>>>> + /* IN - number of extents. */
>>>> + uint32_t nr_extents;
>>>> + /* IN - should be set to 0, and is updated on continuation. */
>>>> + uint32_t pfns_processed;
>>> I'd prefer if the field (if to be kept) wasn't named after its current
>>> purpose, nor should we state anything beyond it needing to be
>>> zero when first invoking the call. These are implementation details
>>> which callers should not rely on.
>> Assuming we keep it, how about calling it "reserved", with a comment in
>> dm.c explaining what its actually for?
> Elsewhere we use "opaque", but "reserved" is probably fine too.
> However, we may want to name it as an OUT value, for the
> error-on-extent indication mentioned above (of course another
> option would be to make nr_extent IN/OUT). As an OUT, we're
> free to use it for whatever intermediate value, just so long as
> upon final return to the caller it has the intended value. (It's
> debatable whether it should be IN/OUT, due to the need for it
> to be set to zero.)
>
> Jan
Having an indication of which extent failed seem a sensible idea. We'd
need that
parameter to be initially set to something that can represent none of
the extents,
such that if there is an error before we get to precessing the extents,
this is clear.
If we used nr_extents - the caller can check its the same value they
passed in.
If the value is the same, then the error occurred before processing an
extent.
If we used error_on_extent (previously known as pfns_processed), we
could not
use zero, as it might be the first extent to fail. In this case, we'd
need to specify
it was defaulted to 0xFFFFFFFF. Should probably define a constant -
NO_EXTENT.
Internally, we could just invert the value for using as a pfn_processed
intermediately.
However, both of these ideas suffer a problem, that if a continuation
fails before processing
the remaining extents, the returned value would be incorrect. For
reusing nr_extents, it
would appear to be the previous extent - potentially very confusing.
Using
error_on_extent would give you a totally different value, although very
likely, one that
is greater then the number of extents passed in.
Both of these schemes can be solved by only allowing 31 bit number of
extents and pfns,
and having the fields signed, to make it more obvious.
For both schemes, you could return -extent_nr, where only a negative
value indicates
and extent number. This would allow you to have the initial value of 0
for error_on_extent.
Alternatively, for the error_on_extent scheme, the pfns_processed could
be stored as a negative,
which would mean that if a negative value got returned it was was no
specific extent.
The value would need initialised by the caller as -1.
An alternative to using signed values would be to use a combination of
the two values.
If nr_extents == 0, then error_on_extent hold the extent number. It
allows full 32 bit values
but is kinda odd.
I'm happy to do any of the 4 schemes - they all have pros and cons.
What do you think?
Cheers,
-jenny
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2017-03-23 14:54 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-21 13:59 [PATCH v2] dm_op: Add xendevicemodel_modified_memory_bulk Jennifer Herbert
2017-03-22 10:33 ` Jan Beulich
2017-03-22 19:55 ` Jennifer Herbert
2017-03-23 8:35 ` Jan Beulich
2017-03-23 14:54 ` Jennifer Herbert [this message]
2017-03-23 15:58 ` Jan Beulich
2017-03-23 18:44 ` Jennifer Herbert
2017-03-24 7:21 ` Jan Beulich
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=58D3E189.40408@citrix.com \
--to=jennifer.herbert@citrix.com \
--cc=JBeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=ian.jackson@eu.citrix.com \
--cc=paul.durrant@citrix.com \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xen.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.