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

[Dan McGee]

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.

> 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