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

Submitted by Dan McGee on Feb. 13, 2012, 7:48 p.m.

Details

Message ID 1329162521-12278-1-git-send-email-dpmcgee@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Dan McGee Feb. 13, 2012, 7:48 p.m.
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@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(-)

Patch hide | download patch | download mbox

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);
+    /* 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,

Comments

While the best thing would have been to pass the first image already to 
the client using those 256K (and calculate the bandwidth based on the 
first data passed to the client and continue to do so, as the protocol 
continues), the next best thing would probably be pass *some* image to 
the client - some JPEG compressed logo or something.
This is:
(1) already quite compressed.
(2) useful in some way.
(3) probably 'cheaper' than creating some random data.

I think QEMU has this feature (off by default) to load a bootsplash 
image as part of the BIOS. I wonder if it can be somehow re-used.

Y.


On 02/13/2012 09:48 PM, 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@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);
> +    /* 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,
On Mon, Feb 13, 2012 at 3:11 PM, Yaniv Kaul <ykaul@redhat.com> wrote:
> While the best thing would have been to pass the first image already to the
> client using those 256K (and calculate the bandwidth based on the first data
> passed to the client and continue to do so, as the protocol continues), the
> next best thing would probably be pass *some* image to the client - some
> JPEG compressed logo or something.
> This is:
> (1) already quite compressed.
> (2) useful in some way.
> (3) probably 'cheaper' than creating some random data.
>
> I think QEMU has this feature (off by default) to load a bootsplash image as
> part of the BIOS. I wonder if it can be somehow re-used.

This sounds cool, but also seems overkill for at least this initial
improvement to the ping functionality, which is completely broken
right now if compression is involved, as far as I can tell. I'd rather
get this in first, then let someone else improve it if they find the
need for something better.

As far as (1) and (3) are concerned, would it be an awful idea to just
compile this data in? Take something like
http://gulf.br.tripod.com/wallpaper/tux.jpg (around 256KB), convert it
to a C unsigned char array, and use that as the data packets.

For (2), making it useful, that seems like a much bigger proposition
that I was hoping not to get involved with here.

-Dan
On 02/14/2012 01:54 AM, Dan McGee wrote:
> On Mon, Feb 13, 2012 at 3:11 PM, Yaniv Kaul<ykaul@redhat.com>  wrote:
>> While the best thing would have been to pass the first image already to the
>> client using those 256K (and calculate the bandwidth based on the first data
>> passed to the client and continue to do so, as the protocol continues), the
>> next best thing would probably be pass *some* image to the client - some
>> JPEG compressed logo or something.
>> This is:
>> (1) already quite compressed.
>> (2) useful in some way.
>> (3) probably 'cheaper' than creating some random data.
>>
>> I think QEMU has this feature (off by default) to load a bootsplash image as
>> part of the BIOS. I wonder if it can be somehow re-used.
> This sounds cool, but also seems overkill for at least this initial
> improvement to the ping functionality, which is completely broken
> right now if compression is involved, as far as I can tell. I'd rather
> get this in first, then let someone else improve it if they find the
> need for something better.

Yes, agreed.

>
> As far as (1) and (3) are concerned, would it be an awful idea to just
> compile this data in? Take something like
> http://gulf.br.tripod.com/wallpaper/tux.jpg (around 256KB), convert it
> to a C unsigned char array, and use that as the data packets.

I think it makes sense to look at what QEMU is doing with the bootsplash 
first.
Y.

>
> For (2), making it useful, that seems like a much bigger proposition
> that I was hoping not to get involved with here.
>
> -Dan
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@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? Note that you should then
keep the spice_marshaller_add_ref. spice_marshaller_add doesn't free for
you (I checked).

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.

> +    /* 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
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On 02/14/2012 10:40 AM, Alon Levy wrote:

<SNIP>

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

VPNs usually use DEFLATE (which is close to gzip).
Both SSL/TLS and IPsec do, at least.
Y.

> But it's probably a good indication of the compressability.
>
>> +    /* 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
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Yaniv Kaul píše v Po 13. 02. 2012 v 23:11 +0200:
> While the best thing would have been to pass the first image already to 
> the client using those 256K (and calculate the bandwidth based on the 
> first data passed to the client and continue to do so, as the protocol 
> continues),

upstream & downstream bugs requesting it:
http://bugs.freedesktop.org/show_bug.cgi?id=43756
https://bugzilla.redhat.com/show_bug.cgi?id=787197

David

>  the next best thing would probably be pass *some* image to 
> the client - some JPEG compressed logo or something.
> This is:
> (1) already quite compressed.
> (2) useful in some way.
> (3) probably 'cheaper' than creating some random data.
> 
> I think QEMU has this feature (off by default) to load a bootsplash 
> image as part of the BIOS. I wonder if it can be somehow re-used.
> 
> Y.
> 
> 
> On 02/13/2012 09:48 PM, 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@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);
> > +    /* 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,
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
On Tue, Feb 14, 2012 at 2:40 AM, Alon Levy <alevy@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@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
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@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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel