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

[Halil Pasic]

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.

> 
> 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.

>> -    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.

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.

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?

Cheers,
Halil

>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
[..]

signature.asc
Description: OpenPGP digital signature