[Qemu-devel] semantics of qemu_peek_buffer ?

[Qemu-devel] semantics of qemu_peek_buffer ?

[Dr. David Alan Gilbert]

Hi Juan,
  What are the semantics of 'qemu_peek_buffer'?

 - is it supposed to guarantee (if there are no errors) that
   it will read 'size' bytes? (i.e. it should block)

There are currently two users of it:
   * qemu_read_buffer which spins filling it's buffer up
     with repeated calls to qemu_peek_buffer

   * vmstate_subsection_load that returns if the size read
     doesn't match what it was expecting

I can't see how both of them can be right.

The problem I'm seeing is that in my world I've got a 
qemu_peek_buffer of 8 bytes, and with a repeated virt-test
local tcp migration it's failing about 1 in 8 times;
here is some debug:

19:51:15 INFO | [qemu output] qemu_peek_buffer refill case (pre); size=8 offset=0 index=32764 pending=4 buf_index=32764 buf_size=32768 pos=23302795
19:51:15 INFO | [qemu output] qemu_fill_buffer got 1
19:51:15 INFO | [qemu output] qemu_peek_buffer refill case (post); size=8 offset=0 index=0 pending=5 buf_index=0 buf_size=5 pos=23302796
19:51:15 INFO | [qemu output] qemu_peek_buffer (size>pending); size=8 offset=0 index=0 pending=5 buf_index=0 buf_size=5 pos=23302796

i.e. I asked for 8 bytes, there were 4 in the buffer, it called fill buffer, which got one
more byte, and thus it returned me 5.

I think what vmstate_subsection_load wants (and what I want) is something
like qemu_read_buffer but which doesn't advance it's pointer, i.e. to read
a header, decide it's not for me and let the next function along use it.

vmstate_subsection_load doesn't look like it flags an error if it
doesn't read enough; I guess the effect will be just to fail to 
load a migration in some interesting way.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] semantics of qemu_peek_buffer ?

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> Hi Juan,
>   What are the semantics of 'qemu_peek_buffer'?
>
>  - is it supposed to guarantee (if there are no errors) that
>    it will read 'size' bytes? (i.e. it should block)


We are talking qemu, specifically migration.  "quarantee" is always a
too strong word O:-)

Once told that, qemu_peek_buffer() should always read the amount of
stuff it has been asked (or terun one error.

> There are currently two users of it:
>    * qemu_read_buffer which spins filling it's buffer up
>      with repeated calls to qemu_peek_buffer

<if people are using grep, function in qemu_get_buffer()>

if we ask for "size" bytes, and there are less that that size, we are in trouble.


>    * vmstate_subsection_load that returns if the size read
>      doesn't match what it was expecting

This is look-ahead of "size" chars, and has all the problems that you
can think of read-ahead of more than one char.  How is it used in
sub-sections:

- we read that the 1st char is a subsection number (but it could not be
  a subsection).
- we read the size
- we read the subsection name of that size

- we search for a subsection with that name, if it exist, we then read
  them properly.  if it don't exist, we abort the whole subsection idea,
  nad continue as if the subsection number hadn't exist in the 1st place.

> I can't see how both of them can be right.
>
> The problem I'm seeing is that in my world I've got a 
> qemu_peek_buffer of 8 bytes, and with a repeated virt-test
> local tcp migration it's failing about 1 in 8 times;
> here is some debug:
>
> 19:51:15 INFO | [qemu output] qemu_peek_buffer refill case (pre); size=8 offset=0 index=32764 pending=4 buf_index=32764 buf_size=32768 pos=23302795
> 19:51:15 INFO | [qemu output] qemu_fill_buffer got 1
> 19:51:15 INFO | [qemu output] qemu_peek_buffer refill case (post); size=8 offset=0 index=0 pending=5 buf_index=0 buf_size=5 pos=23302796
> 19:51:15 INFO | [qemu output] qemu_peek_buffer (size>pending); size=8 offset=0 index=0 pending=5 buf_index=0 buf_size=5 pos=23302796
>
> i.e. I asked for 8 bytes, there were 4 in the buffer, it called fill buffer, which got one
> more byte, and thus it returned me 5.

Uh, oh.  That shouldn't happen.  could you try to change the
   "if (pending < size)"
to
   "while (pending < size)"

remember that you need to handle errors on that loop?

BTW, what are you doing when you get that error?  It is vmstate_load or
qemu_get_buffer()?  and for what fiel?

> I think what vmstate_subsection_load wants (and what I want) is something
> like qemu_read_buffer but which doesn't advance it's pointer, i.e. to read
> a header, decide it's not for me and let the next function along use it.

Problem was to fix the case when there is less that <size> bytes into
the buffer.  i.e. we start to search for a string that is bigger than
the remaining (already read) buffer.

> vmstate_subsection_load doesn't look like it flags an error if it
> doesn't read enough; I guess the effect will be just to fail to 
> load a migration in some interesting way.

Error checking is missing there.  If we don't get enough space, we
really want to fail the subsection read.

Later, Juan.

Re: [Qemu-devel] semantics of qemu_peek_buffer ?

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> > Hi Juan,
> >   What are the semantics of 'qemu_peek_buffer'?
> >
> >  - is it supposed to guarantee (if there are no errors) that
> >    it will read 'size' bytes? (i.e. it should block)
> 
> 
> We are talking qemu, specifically migration.  "quarantee" is always a
> too strong word O:-)

It would be nice for people to expect it!

> Once told that, qemu_peek_buffer() should always read the amount of
> stuff it has been asked (or terun one error.

OK.

> > There are currently two users of it:
> >    * qemu_read_buffer which spins filling it's buffer up
> >      with repeated calls to qemu_peek_buffer
> 
> <if people are using grep, function in qemu_get_buffer()>
> 
> if we ask for "size" bytes, and there are less that that size, we are in trouble.
> 
> 
> >    * vmstate_subsection_load that returns if the size read
> >      doesn't match what it was expecting
> 
> This is look-ahead of "size" chars, and has all the problems that you
> can think of read-ahead of more than one char.  How is it used in
> sub-sections:
> 
> - we read that the 1st char is a subsection number (but it could not be
>   a subsection).
> - we read the size
> - we read the subsection name of that size
> 
> - we search for a subsection with that name, if it exist, we then read
>   them properly.  if it don't exist, we abort the whole subsection idea,
>   nad continue as if the subsection number hadn't exist in the 1st place.
> 
> > I can't see how both of them can be right.
> >
> > The problem I'm seeing is that in my world I've got a 
> > qemu_peek_buffer of 8 bytes, and with a repeated virt-test
> > local tcp migration it's failing about 1 in 8 times;
> > here is some debug:
> >
> > 19:51:15 INFO | [qemu output] qemu_peek_buffer refill case (pre); size=8 offset=0 index=32764 pending=4 buf_index=32764 buf_size=32768 pos=23302795
> > 19:51:15 INFO | [qemu output] qemu_fill_buffer got 1
> > 19:51:15 INFO | [qemu output] qemu_peek_buffer refill case (post); size=8 offset=0 index=0 pending=5 buf_index=0 buf_size=5 pos=23302796
> > 19:51:15 INFO | [qemu output] qemu_peek_buffer (size>pending); size=8 offset=0 index=0 pending=5 buf_index=0 buf_size=5 pos=23302796
> >
> > i.e. I asked for 8 bytes, there were 4 in the buffer, it called fill buffer, which got one
> > more byte, and thus it returned me 5.
> 
> Uh, oh.  That shouldn't happen.  could you try to change the
>    "if (pending < size)"
> to
>    "while (pending < size)"
> 
> remember that you need to handle errors on that loop?

Yes, I think that will work - I think really the loop that is in qemu_get_buffer()
needs to move up into qemu_peek_buffer() (with qemu_fill_buffer returning the
number of bytes it has read).

> BTW, what are you doing when you get that error?  It is vmstate_load or
> qemu_get_buffer()?  and for what fiel?

In my migration visitor world, in the compatibility code for the current format,
I'm using qemu_peek_buffer to peek the 8 byte header on ram pages to figure
out which type of page encoding we're using, so this means I'm calling qemu_peek_buffer
once per page (with 8 bytes peek'd each time), so it's calling it a lot.

> > I think what vmstate_subsection_load wants (and what I want) is something
> > like qemu_read_buffer but which doesn't advance it's pointer, i.e. to read
> > a header, decide it's not for me and let the next function along use it.
> 
> Problem was to fix the case when there is less that <size> bytes into
> the buffer.  i.e. we start to search for a string that is bigger than
> the remaining (already read) buffer.
> 
> > vmstate_subsection_load doesn't look like it flags an error if it
> > doesn't read enough; I guess the effect will be just to fail to 
> > load a migration in some interesting way.
> 
> Error checking is missing there.  If we don't get enough space, we
> really want to fail the subsection read.

Yes, I think it's possible this code could hit the problem I've seen,
but it's much less likely because there are a lot less subsections
than there are pages.
I think the kernel can return only a few bytes, possibly because when
qemu_fill_buffer fills the buffer it can read a weird number of bytes
depending how much buffer space is left; so the kernel could have a few
left for the next read.

Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK