[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf |
Date: |
Mon, 17 Oct 2016 20:06:01 +0100 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
* Halil Pasic (address@hidden) wrote:
>
>
> On 10/17/2016 05:36 AM, David Gibson wrote:
> > On Tue, Oct 11, 2016 at 06:18:32PM +0100, Dr. David Alan Gilbert (git)
> > wrote:
> >> From: "Dr. David Alan Gilbert" <address@hidden>
> >>
> >> Convert the sbuf structure to a VMStateDescription.
> >> Note this uses the VMSTATE_WITH_TMP mechanism to calculate
> >> and reload the offsets based on the pointers.
> >>
> >> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
>
> Hi Dave!
>
> I had a brief look, which means I intend to have a deeper
> one too, but for now you will have to live with this.
Thanks.
> >
> > Reviewed-by: David Gibson <address@hidden>
> >
> >> ---
> >> slirp/sbuf.h | 4 +-
> >> slirp/slirp.c | 116
> >> ++++++++++++++++++++++++++++++++++++++--------------------
> >> 2 files changed, 78 insertions(+), 42 deletions(-)
> >>
> >> diff --git a/slirp/sbuf.h b/slirp/sbuf.h
> >> index efcec39..a722ecb 100644
> >> --- a/slirp/sbuf.h
> >> +++ b/slirp/sbuf.h
> >> @@ -12,8 +12,8 @@
> >> #define sbspace(sb) ((sb)->sb_datalen - (sb)->sb_cc)
> >>
> >> struct sbuf {
> >> - u_int sb_cc; /* actual chars in buffer */
> >> - u_int sb_datalen; /* Length of data */
> >> + uint32_t sb_cc; /* actual chars in buffer */
> >> + uint32_t sb_datalen; /* Length of data */
> >> char *sb_wptr; /* write pointer. points to where the next
> >> * bytes should be written in the sbuf */
> >> char *sb_rptr; /* read pointer. points to where the next
> >> diff --git a/slirp/slirp.c b/slirp/slirp.c
> >> index 6276315..2f7802e 100644
> >> --- a/slirp/slirp.c
> >> +++ b/slirp/slirp.c
> >> @@ -1185,19 +1185,72 @@ static const VMStateDescription vmstate_slirp_tcp
> >> = {
> >> }
> >> };
> >>
> >> -static void slirp_sbuf_save(QEMUFile *f, struct sbuf *sbuf)
> >> +/* The sbuf has a pair of pointers that are migrated as offsets;
> >> + * we calculate the offsets and restore the pointers using
> >> + * pre_save/post_load on a tmp structure.
> >> + */
> >> +struct sbuf_tmp {
> >> + struct sbuf *parent;
> >> + uint32_t roff, woff;
> >> +};
> >> +
> >> +static void sbuf_tmp_pre_save(void *opaque)
> >> +{
> >> + struct sbuf_tmp *tmp = opaque;
> >> + tmp->woff = tmp->parent->sb_wptr - tmp->parent->sb_data;
> >> + tmp->roff = tmp->parent->sb_rptr - tmp->parent->sb_data;
> >> +}
> >> +
> >> +static int sbuf_tmp_post_load(void *opaque, int version)
> >> {
>
> What makes me think about the properties of this approach,
> is, that each time we use a parent pointer to read we have
> a data dependency. This seems to me much more complicated
> that the current massaging function approach were we say
> "OK now everything below me is there, now let us transform".
> Of course the proposed approach is more powerful.
Yes it is, but we have to apply a transform to the data
so that means we somehow need to get to both a temporary
piece of storage and the parent data.
> >> - uint32_t off;
> >> -
> >> - qemu_put_be32(f, sbuf->sb_cc);
> >> - qemu_put_be32(f, sbuf->sb_datalen);
> >> - off = (uint32_t)(sbuf->sb_wptr - sbuf->sb_data);
> >> - qemu_put_sbe32(f, off);
> >> - off = (uint32_t)(sbuf->sb_rptr - sbuf->sb_data);
> >> - qemu_put_sbe32(f, off);
> >> - qemu_put_buffer(f, (unsigned char*)sbuf->sb_data, sbuf->sb_datalen);
> >> + struct sbuf_tmp *tmp = opaque;
> >> + uint32_t requested_len = tmp->parent->sb_datalen;
>
> Ok, data parent->sb_datalen was previously loaded at #1
>
> >> +
> >> + /* Allocate the buffer space used by the field after the tmp */
> >> + sbreserve(tmp->parent, tmp->parent->sb_datalen);
> #2
> >> +
> >> + if (tmp->parent->sb_datalen != requested_len) {
> >> + return -ENOMEM;
> >> + }
> >> + if (tmp->woff >= requested_len ||
> >> + tmp->roff >= requested_len) {
> >> + error_report("invalid sbuf offsets r/w=%u/%u len=%u",
> >> + tmp->roff, tmp->woff, requested_len);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + tmp->parent->sb_wptr = tmp->parent->sb_data + tmp->woff;
> >> + tmp->parent->sb_rptr = tmp->parent->sb_data + tmp->roff;
>
> Ok, parent->sb_data is assigned and the backing memory allocated
> at #2
>
> >> +
> >> + return 0;
> >> }
> >>
> >> +
> >> +static const VMStateDescription vmstate_slirp_sbuf_tmp = {
> >> + .name = "slirp-sbuf-tmp",
> >> + .post_load = sbuf_tmp_post_load,
> >> + .pre_save = sbuf_tmp_pre_save,
> >> + .version_id = 0,
> >> + .fields = (VMStateField[]) {
> >> + VMSTATE_UINT32(woff, struct sbuf_tmp),
> >> + VMSTATE_UINT32(roff, struct sbuf_tmp),
> >> + VMSTATE_END_OF_LIST()
> >> + }
> >> +};
> >> +
> >> +static const VMStateDescription vmstate_slirp_sbuf = {
> >> + .name = "slirp-sbuf",
> >> + .version_id = 0,
> >> + .fields = (VMStateField[]) {
> >> + VMSTATE_UINT32(sb_cc, struct sbuf),
> >> + VMSTATE_UINT32(sb_datalen, struct sbuf),
>
> #1
>
> >> + VMSTATE_WITH_TMP(struct sbuf, struct sbuf_tmp,
> >> vmstate_slirp_sbuf_tmp),
> >> + VMSTATE_VBUFFER_UINT32(sb_data, struct sbuf, 0, NULL, 0,
> >> sb_datalen),
>
> OK, memory was allocated at #2
> It is a bit confusing though (for a novice like me) that we have a non ALLOC
> VBUFFER
> whose pointer is NULL after post_load.
I don't think this pointer can be NULL; the sbreserve at #2 causes it to be
allocated.
But yes, it's a shame I can't use VMS_ALLOC here, but the sbreserve is not
a trivial allocation function.
> Now if I imagine the original stream were written in the following sequence:
> vbuffer_length (sb_datalen), vbuffer_data (sb_data), offsets (sb_wptr,
> sb_rptr)
> which seems completely valid to me then the context would not be sufficient
> to compute sb_wptr and sb_rptr because the lifetime of vbuffer_data and
> the tmp do not overlap.
If that was the case you could still do it pretty easily.
You'd have to add the sb_datalen and sb_data fields to the temporary
and then move the VMSTATE_VBUFFER_UINT32 into the tmp so it would operate
on the copied fields.
> I aware it's a trade-off between how long the temporary data lives and
> how complicated the dependencies get. Or am I getting something wrong?
No, I think that's right. The other option I thought of was a macro
to allocate a temporary and then another to free it and then someway
to tell macros in between that they should operate on the temporary
rather than the main pointer; but then you'd have to be VERY careful
to not allow yourself to access a temporary that's been freed.
This structure means you can't make that mistake.
Dave
>
> Cheers,
> Halil
>
> >> + VMSTATE_END_OF_LIST()
> >> + }
> >> +};
> [..]
>
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK
- Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP, (continued)
- Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP, Dr. David Alan Gilbert, 2016/10/17
- Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP, Jianjun Duan, 2016/10/17
- Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP, Dr. David Alan Gilbert, 2016/10/17
- Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP, Jianjun Duan, 2016/10/17
- Re: [Qemu-devel] [very-WIP 1/7] migration: Add VMSTATE_WITH_TMP, Dr. David Alan Gilbert, 2016/10/18
[Qemu-devel] [very-WIP 2/7] tests/migration: Add test for VMSTATE_WITH_TMP, Dr. David Alan Gilbert (git), 2016/10/11
[Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf, Dr. David Alan Gilbert (git), 2016/10/11
[Qemu-devel] [very-WIP 4/4] virtio/migration: Migrate virtio-net to VMState, Dr. David Alan Gilbert (git), 2016/10/11