[Spice-devel] [PATCH] Use non-zero data for initial ping

[Alon Levy]

On Tue, Feb 14, 2012 at 09:01:47AM -0600, Dan McGee wrote:
> On Tue, Feb 14, 2012 at 2:40 AM, Alon Levy <alevy at redhat.com> wrote:
> > On Mon, Feb 13, 2012 at 01:48:41PM -0600, Dan McGee wrote:
> >> This prevents compression over things such as VPN links misleading us on
> >> available bandwidth. The page used before was 4K worth of zeroes, which
> >> isn't a very realistic test at all.
> >>
> >> We now use a generated sequence of bytes that vary in value and offset
> >> between consecutive values, causing compression programs to have a much
> >> harder time reducing our ping packets to nothing.
> >>
> >> Here are some comparisons of before/after using a small standalone
> >> program that generates bytes in the same fashion:
> >>
> >> Before:
> >>     4096 zerodata
> >>     43   zerodata.bz2
> >>     38   zerodata.gz
> >>     92   zerodata.xz
> >>
> >> After:
> >>     4096 seqdata
> >>     1213 seqdata.bz2
> >>     4119 seqdata.gz
> >>     3208 seqdata.xz
> >>
> >> Signed-off-by: Dan McGee <dpmcgee at gmail.com>
> >> ---
> >>
> >> This was a TODO item on the following page:
> >> http://spice-space.org/page/Features/Bandwidth_monitoring
> >>
> >> The standalone test program is below if anyone wants to try it out; simply pass
> >> '4096' or similar as the first argument and you will get 4096 "random" bytes on
> >> stdout that you can play with.
> >>
> >> #include <stdio.h>
> >> #include <stdlib.h>
> >>
> >> int main(int argc, char *argv[])
> >> {
> >>       int i, total;
> >>
> >>       total = atoi(argv[1]);
> >>
> >>       for(i = 0; i < total; i++) {
> >>               div_t result = div(i, 256);
> >>               if(result.quot % 2 == 0) {
> >>                       fputc((result.quot + 1) * result.rem, stdout);
> >>               } else {
> >>                       fputc(512 - (result.quot * result.rem), stdout);
> >>               }
> >>       }
> >>       return 0;
> >> }
> >>
> >>
> >>  server/main_channel.c |   24 ++++++++++++++++--------
> >>  1 files changed, 16 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/server/main_channel.c b/server/main_channel.c
> >> index f7e1ab0..2ce20c0 100644
> >> --- a/server/main_channel.c
> >> +++ b/server/main_channel.c
> >> @@ -22,6 +22,7 @@
> >>  #include <inttypes.h>
> >>  #include <stdint.h>
> >>  #include <stdio.h>
> >> +#include <stdlib.h>
> >>  #include <unistd.h>
> >>  #include <sys/socket.h>
> >>  #include <netinet/in.h>
> >> @@ -45,8 +46,6 @@
> >>  #include "red_channel.h"
> >>  #include "generated_marshallers.h"
> >>
> >> -#define ZERO_BUF_SIZE 4096
> >> -
> >>  #define REDS_MAX_SEND_IOVEC 100
> >>
> >>  #define NET_TEST_WARMUP_BYTES 0
> >> @@ -54,8 +53,6 @@
> >>
> >>  #define PING_INTERVAL (1000 * 10)
> >>
> >> -static uint8_t zero_page[ZERO_BUF_SIZE] = {0};
> >> -
> >>  typedef struct RedsOutItem RedsOutItem;
> >>  struct RedsOutItem {
> >>      PipeItem base;
> >> @@ -335,17 +332,28 @@ static void main_channel_marshall_ping(SpiceMarshaller *m, int size, int ping_id
> >>  {
> >>      struct timespec time_space;
> >>      SpiceMsgPing ping;
> >> +    uint8_t *ping_data;
> >> +    int i;
> >>
> >>      ping.id = ping_id;
> >>      clock_gettime(CLOCK_MONOTONIC, &time_space);
> >>      ping.timestamp = time_space.tv_sec * 1000000LL + time_space.tv_nsec / 1000LL;
> >>      spice_marshall_msg_ping(m, &ping);
> >>
> >> -    while (size > 0) {
> >> -        int now = MIN(ZERO_BUF_SIZE, size);
> >> -        size -= now;
> >> -        spice_marshaller_add_ref(m, zero_page, now);
> >> +    ping_data = spice_malloc(size);
> >
> > Are you freeing this anywhere? Why not leave the static above, just
> > rename it since it's no longer a zero_page?
> Because that is a 4K, static sized page, and repeating that to get to
> the normal 250K ping could result in misleading results again, if the
> compression algorithm window is long enough that it sees the repeating
> pattern. The crude algorithm I proposed is effective at ensuring the
> bytes don't repeat on a too-short window.
Right. So I guess allocate 250k and release it after ping test is done.

> 
> > Note that you should then
> > keep the spice_marshaller_add_ref. spice_marshaller_add doesn't free for
> > you (I checked).
> Hmm, I do see this now; for some reason I was under that impression,
> so this is my bad. Will fix (if this still holds true after looking at
> some of the other concerns raised).
> 
> > Other then that looks good, and like you say probably better - although
> > I'm not convinced that by checking several common desktop compression
> > programs you are checking whatever compression scheme vpn's are using.
> > But it's probably a good indication of the compressability.
> As Yaniv Kaul indicated, deflate is usually used which is similar
> enough to gzip that I thought this was a applicable test. At the very
> least, it showed the downfall of the old behavior with compression
> involved (any compression algorithm worth its salt reduced the zero
> page to a just a handful of bytes).
> 
> >> +    /* this produces ascending and descending byte runs which vary in offset
> >> +     * every 512 bytes, preventing prevents compression from being able to
> >> +     * influence the resulting size of the ping data too much */
> >> +    for(i = 0; i < size; i++) {
> >> +        div_t result = div(i, 256);
> >> +        if(result.quot % 2 == 0) {
> >> +            ping_data[i] = (result.quot + 1) * result.rem;
> >> +        } else {
> >> +            ping_data[i] = 512 - (result.quot * result.rem);
> >> +        }
> >>      }
> >> +
> >> +    spice_marshaller_add(m, ping_data, size);
> >>  }
> >>
> >>  void main_channel_push_mouse_mode(MainChannel *main_chan, int current_mode,
> >> --
> >> 1.7.9
> 
> I'll look for a way to get at the bootsplash image and get back to everyone.
> 
> -Dan
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel