Re: [Qemu-devel] [PATCH] vnc: shared buffer: skip some optimizations.

[Gerd Hoffmann <kraxel@redhat.com>]

Stefano Stabellini wrote:
> Gerd Hoffmann wrote:
> 
>> Sending screen updates to the client will be done using the server
>> surface + dirty map only, so the guest updating the screen in parallel
>> can't cause trouble here.
> 
> Could you please explain an example of trouble caused by sich situations?

Sure.

> Given A, B and C three different points in time with A < B < C,
> currently what we are doing is:
> 
> A) getting the guest dirty map
> 
> B) filtering the guest dirty map using memcmp with the server surface
> (called old_data in the current code), however the dirty map could be
> inconsistent with the framebuffer because was set in A
> 
> C) sending updates to the client from the guest surface based on the
> dirty map, however the dirty map is inconsistent because it was set in A
> and filtered in B

The dirty bitmap inconsistency isn't a problem at all.  The next update
round will fix that.

The trouble spot is here:  The guest might have updated the screen
between (b) and (c).  This will cause the vnc clients view of the screen
content become inconsistent with old_data.  On the next update round (b)
will stop working correctly due to that.

What my patch does in terms of the old code is make step (c) use
old_data instead of the guest-visible framebuffer.  I didn't like
stacking hacks on hacks like this though, so I decided to explicitly
name the two surfaces.

You still could get away with just one dirty bitmap for both surfaces.
Having two bitmaps IMHO makes the code more readable though.  It also
allows killing the memset(old_data,42,size) hack for forced client
updates (see framebuffer_update_request()).  An -- in preparation for
the future -- it makes the vnc code a bit more thread-friendly.  Guest
screen updating (aka vnc_update()) can run in parallel with encoding
updates for the guest (i.e. step (c)).

> If your goal is to send the most updated version of the framebuffer to
> the guest ASAP, the current code is better at it.

But *that* is exactly where the bug is ;)

cheers,
  Gerd