Re: [Qemu-devel] [very-WIP 3/4] slirp: VMStatify sbuf

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Halil Pasic (pasic@linux.vnet.ibm.com) 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" <dgilbert@redhat.com>
> >>
> >> 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 <dgilbert@redhat.com>
> 
> 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 <david@gibson.dropbear.id.au>
> > 
> >> ---
> >>  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 / dgilbert@redhat.com / Manchester, UK