[Qemu-devel] [PATCH 0/4] VNC server memory savings

[Qemu-devel] [PATCH 0/4] VNC server memory savings

[Peter Lieven]

While debugging increased memory footprint of Qemu (since qemu-kvm-1.2.0)
I found some culprits in the VNC code. This series includes some
first optimizations.

Peter Lieven (4):
  vnc: make the Buffer capacity increase in powers of two
  vnc: allow the Buffer to shrink again
  vnc-jobs: move buffer_reset to vnc_async_encoding_end
  vnc: destroy server surface if no client is connected

 ui/vnc-jobs.c |  3 +--
 ui/vnc.c      | 36 +++++++++++++++++++++++++++++++++---
 2 files changed, 34 insertions(+), 5 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/4] vnc: make the Buffer capacity increase in powers of two

[Peter Lieven]

This makes sure the number of reallocs is in O(log N).

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index caf82f5..8cfd2d8 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -653,10 +653,13 @@ void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
     vnc_write_s32(vs, encoding);
 }
 
+#define BUFFER_MIN_INIT_SIZE 4096
+
 void buffer_reserve(Buffer *buffer, size_t len)
 {
     if ((buffer->capacity - buffer->offset) < len) {
-        buffer->capacity += (len + 1024);
+        buffer->capacity = pow2ceil(buffer->capacity + len);
+        buffer->capacity = MAX(buffer->capacity, BUFFER_MIN_INIT_SIZE);
         buffer->buffer = g_realloc(buffer->buffer, buffer->capacity);
     }
 }
-- 
1.9.1

[Qemu-devel] [PATCH 2/4] vnc: allow the Buffer to shrink again

[Peter Lieven]

currently the Buffer can only grow. This increases Qemu memory footprint
dramatically since normally the biggest VNC updates are at connection time.
But also after a VNC session has terminated there is one persistent buffer
in queue->buffer which I have seen to increase to over 100MB and it is never
getting smaller again.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 8cfd2d8..061e337 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -654,6 +654,7 @@ void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
 }
 
 #define BUFFER_MIN_INIT_SIZE 4096
+#define BUFFER_MIN_SHRINK_SIZE 65536
 
 void buffer_reserve(Buffer *buffer, size_t len)
 {
@@ -674,9 +675,19 @@ uint8_t *buffer_end(Buffer *buffer)
     return buffer->buffer + buffer->offset;
 }
 
+static void buffer_shrink(Buffer *buffer) {
+    size_t new = MAX(pow2ceil(buffer->offset) * 2,
+                     BUFFER_MIN_SHRINK_SIZE);
+    if (new < buffer->capacity) {
+        buffer->buffer = g_realloc(buffer->buffer, new);
+        buffer->capacity = new;
+    }
+}
+
 void buffer_reset(Buffer *buffer)
 {
-        buffer->offset = 0;
+     buffer->offset = 0;
+     buffer_shrink(buffer);
 }
 
 void buffer_free(Buffer *buffer)
@@ -698,6 +709,7 @@ void buffer_advance(Buffer *buf, size_t len)
     memmove(buf->buffer, buf->buffer + len,
             (buf->offset - len));
     buf->offset -= len;
+    buffer_shrink(buf);
 }
 
 static void vnc_desktop_resize(VncState *vs)
-- 
1.9.1

[Qemu-devel] [PATCH 3/4] vnc-jobs: move buffer_reset to vnc_async_encoding_end

[Peter Lieven]

now that we are able to shrink the buffer it makes sense
to move the buffer_reset to vnc_async_encoding_end to
actually shrink the buffer when it is no longer used.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc-jobs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index 22c9abc..201d9b7 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -195,8 +195,6 @@ static void vnc_async_encoding_start(VncState *orig, VncState *local)
     local->zrle = orig->zrle;
     local->output =  queue->buffer;
     local->csock = -1; /* Don't do any network work on this thread */
-
-    buffer_reset(&local->output);
 }
 
 static void vnc_async_encoding_end(VncState *orig, VncState *local)
@@ -208,6 +206,7 @@ static void vnc_async_encoding_end(VncState *orig, VncState *local)
     orig->lossy_rect = local->lossy_rect;
 
     queue->buffer = local->output;
+    buffer_reset(&queue->buffer);
 }
 
 static int vnc_worker_thread_loop(VncJobQueue *queue)
-- 
1.9.1

[Qemu-devel] [PATCH 4/4] vnc: destroy server surface if no client is connected

[Peter Lieven]

if no client is connected there is no need to keep the server
surface. Throw it away and replace it with a dummy surface to
save memory.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 ui/vnc.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 061e337..57f0e54 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -776,6 +776,11 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
 
     vnc_abort_display_jobs(vd);
 
+    /* if no client is connected use a dummy surface */
+    if (QTAILQ_EMPTY(&vd->clients)) {
+        surface = qemu_create_displaysurface(0, 0);
+    }
+
     /* server surface */
     qemu_pixman_image_unref(vd->server);
     vd->ds = surface;
@@ -1263,6 +1268,9 @@ void vnc_disconnect_finish(VncState *vs)
 
     if (vs->initialized) {
         QTAILQ_REMOVE(&vs->vd->clients, vs, next);
+        if (QTAILQ_EMPTY(&vs->vd->clients)) {
+            vnc_dpy_switch(&vs->vd->dcl, NULL);
+        }
         qemu_remove_mouse_mode_change_notifier(&vs->mouse_mode_notifier);
     }
 
@@ -3083,6 +3091,7 @@ void vnc_init_state(VncState *vs)
 {
     vs->initialized = true;
     VncDisplay *vd = vs->vd;
+    bool first_client;
 
     vs->last_x = -1;
     vs->last_y = -1;
@@ -3095,9 +3104,15 @@ void vnc_init_state(VncState *vs)
     qemu_mutex_init(&vs->output_mutex);
     vs->bh = qemu_bh_new(vnc_jobs_bh, vs);
 
+    first_client = QTAILQ_EMPTY(&vd->clients);
     QTAILQ_INSERT_TAIL(&vd->clients, vs, next);
 
-    graphic_hw_update(vd->dcl.con);
+    if (first_client) {
+        /* set/restore the correct surface in the VNC server */
+        console_select(0);
+    } else {
+        graphic_hw_update(vd->dcl.con);
+    }
 
     vnc_write(vs, "RFB 003.008\n", 12);
     vnc_flush(vs);
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/4] vnc: allow the Buffer to shrink again

[Daniel P. Berrange]

On Thu, Aug 27, 2015 at 12:18:52PM +0200, Peter Lieven wrote:
> currently the Buffer can only grow. This increases Qemu memory footprint
> dramatically since normally the biggest VNC updates are at connection time.
> But also after a VNC session has terminated there is one persistent buffer
> in queue->buffer which I have seen to increase to over 100MB and it is never
> getting smaller again.

Do you have any idea what caused the buffer to increase to 100MB in size
in the first place ? I would expect a full screen update would cause the
biggest buffer usage, and even for a 1920x1140 screen that should not
be anywhere near 100MB in size. IOW, i'm wondering if the 100MB usage
is symptomatic of a more serious bug somewhere else in the VNC code
that you're just masking you reducing buffer size afterwards.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH 2/4] vnc: allow the Buffer to shrink again

[Peter Lieven]

Am 27.08.2015 um 12:39 schrieb Daniel P. Berrange:
> On Thu, Aug 27, 2015 at 12:18:52PM +0200, Peter Lieven wrote:
>> currently the Buffer can only grow. This increases Qemu memory footprint
>> dramatically since normally the biggest VNC updates are at connection time.
>> But also after a VNC session has terminated there is one persistent buffer
>> in queue->buffer which I have seen to increase to over 100MB and it is never
>> getting smaller again.
> Do you have any idea what caused the buffer to increase to 100MB in size
> in the first place ? I would expect a full screen update would cause the
> biggest buffer usage, and even for a 1920x1140 screen that should not
> be anywhere near 100MB in size. IOW, i'm wondering if the 100MB usage
> is symptomatic of a more serious bug somewhere else in the VNC code
> that you're just masking you reducing buffer size afterwards.

Maybe my commit message was a bit unclear. The memory usage of all buffers
went up to 100MB. The issue with the Buffer is that its not shrinking and that
the date is effectivly in 3 Buffers before its transferred on the wire. Plus depending
on the encoding used there are even more internal buffers.

Peter

Re: [Qemu-devel] [PATCH 1/4] vnc: make the Buffer capacity increase in powers of two

[Gerd Hoffmann]

On Do, 2015-08-27 at 12:18 +0200, Peter Lieven wrote:
> This makes sure the number of reallocs is in O(log N).

Looks good, applied.

Re: [Qemu-devel] [PATCH 2/4] vnc: allow the Buffer to shrink again

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

On Do, 2015-08-27 at 11:39 +0100, Daniel P. Berrange wrote:
> On Thu, Aug 27, 2015 at 12:18:52PM +0200, Peter Lieven wrote:
> > currently the Buffer can only grow. This increases Qemu memory footprint
> > dramatically since normally the biggest VNC updates are at connection time.
> > But also after a VNC session has terminated there is one persistent buffer
> > in queue->buffer which I have seen to increase to over 100MB and it is never
> > getting smaller again.
> 
> Do you have any idea what caused the buffer to increase to 100MB in size
> in the first place ? I would expect a full screen update would cause the
> biggest buffer usage, and even for a 1920x1140 screen that should not
> be anywhere near 100MB in size. IOW, i'm wondering if the 100MB usage
> is symptomatic of a more serious bug somewhere else in the VNC code
> that you're just masking you reducing buffer size afterwards.

Cooked up a buffer stats patch (attached).
Buffer sizes are nowhere near 100MB for me (as expected by Daniel).
Individual buffers are in the 1 -> 4 MB range (1920x1080), even summed
up this is more like 10 not 100 MB.

So, big question remains why they are that big for you?

Beside that I think it makes sense to have the shrinking logic in
buffer_reserve too so we don't have to add buffer_shrink calls all over
the place.

cheers,
  Gerd


[-- Attachment #2: 0001-debug-vnc-buffer-stats.patch --]
[-- Type: text/x-patch, Size: 3442 bytes --]

From e5d9353d02983db0081e44026767adf9f5a72832 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu, 3 Sep 2015 11:44:18 +0200
Subject: [PATCH] [debug] vnc buffer stats

---
 ui/vnc-jobs.c |  1 +
 ui/vnc.c      | 38 ++++++++++++++++++++++++++++++++++++++
 ui/vnc.h      |  2 ++
 3 files changed, 41 insertions(+)

diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
index 22c9abc..1338771 100644
--- a/ui/vnc-jobs.c
+++ b/ui/vnc-jobs.c
@@ -302,6 +302,7 @@ static VncJobQueue *vnc_queue_init(void)
 
     qemu_cond_init(&queue->cond);
     qemu_mutex_init(&queue->mutex);
+    buffer_init(&queue->buffer, 0, "queue");
     QTAILQ_INIT(&queue->jobs);
     return queue;
 }
diff --git a/ui/vnc.c b/ui/vnc.c
index 7a17954..59be765 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -655,12 +655,29 @@ void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
 
 #define BUFFER_MIN_INIT_SIZE 4096
 
+void buffer_init(Buffer *buffer, int fd, const char *name)
+{
+    if (fd) {
+        buffer->name = g_strdup_printf("%d/%s", fd, name);
+    } else {
+        buffer->name = g_strdup(name);
+    }
+}
+
 void buffer_reserve(Buffer *buffer, size_t len)
 {
+    bool inc = false;
+
+    assert(buffer->name);
     if ((buffer->capacity - buffer->offset) < len) {
         buffer->capacity = pow2ceil(buffer->capacity + len);
         buffer->capacity = MAX(buffer->capacity, BUFFER_MIN_INIT_SIZE);
         buffer->buffer = g_realloc(buffer->buffer, buffer->capacity);
+        inc = true;
+    }
+    if (inc) {
+        fprintf(stderr, "%s: %-10s: %4zd kB\n", __func__,
+                buffer->name, buffer->capacity / 1024);
     }
 }
 
@@ -681,6 +698,7 @@ void buffer_reset(Buffer *buffer)
 
 void buffer_free(Buffer *buffer)
 {
+    fprintf(stderr, "%s: %s\n", __func__, buffer->name);
     g_free(buffer->buffer);
     buffer->offset = 0;
     buffer->capacity = 0;
@@ -3027,6 +3045,26 @@ static void vnc_connect(VncDisplay *vd, int csock,
     vs->csock = csock;
     vs->vd = vd;
 
+    buffer_init(&vs->input, vs->csock, "in");
+    buffer_init(&vs->output, vs->csock, "out");
+    buffer_init(&vs->ws_input, vs->csock, "w-in");
+    buffer_init(&vs->ws_output, vs->csock, "w-out");
+    buffer_init(&vs->jobs_buffer, vs->csock, "jobs");
+
+    buffer_init(&vs->tight.tight, vs->csock, "t-tight");
+    buffer_init(&vs->tight.zlib, vs->csock, "t-zlib");
+    buffer_init(&vs->tight.gradient, vs->csock, "t-gradient");
+#ifdef CONFIG_VNC_JPEG
+    buffer_init(&vs->tight.jpeg, vs->csock, "t-jpeg");
+#endif
+#ifdef CONFIG_VNC_PNG
+    buffer_init(&vs->tight.png, vs->csock, "t-png");
+#endif
+    buffer_init(&vs->zlib.zlib, vs->csock, "zlib");
+    buffer_init(&vs->zrle.zrle, vs->csock, "z-zrle");
+    buffer_init(&vs->zrle.fb, vs->csock, "z-fb");
+    buffer_init(&vs->zrle.zlib, vs->csock, "z-zlib");
+
     if (skipauth) {
 	vs->auth = VNC_AUTH_NONE;
 	vs->subauth = VNC_AUTH_INVALID;
diff --git a/ui/vnc.h b/ui/vnc.h
index 814d720..95878c3 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -57,6 +57,7 @@
 
 typedef struct Buffer
 {
+    char *name;
     size_t capacity;
     size_t offset;
     uint8_t *buffer;
@@ -539,6 +540,7 @@ void start_client_init(VncState *vs);
 void start_auth_vnc(VncState *vs);
 
 /* Buffer management */
+void buffer_init(Buffer *buffer, int fd, const char *name);
 void buffer_reserve(Buffer *buffer, size_t len);
 void buffer_reset(Buffer *buffer);
 void buffer_free(Buffer *buffer);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/4] vnc: destroy server surface if no client is connected

[Gerd Hoffmann]

On Do, 2015-08-27 at 12:18 +0200, Peter Lieven wrote:
> if no client is connected there is no need to keep the server
> surface. Throw it away and replace it with a dummy surface to
> save memory.

No dummy surface please.  Just set vd->server = NULL.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/4] vnc: allow the Buffer to shrink again

[Peter Lieven]

Am 03.09.2015 um 11:52 schrieb Gerd Hoffmann:
> On Do, 2015-08-27 at 11:39 +0100, Daniel P. Berrange wrote:
>> On Thu, Aug 27, 2015 at 12:18:52PM +0200, Peter Lieven wrote:
>>> currently the Buffer can only grow. This increases Qemu memory footprint
>>> dramatically since normally the biggest VNC updates are at connection time.
>>> But also after a VNC session has terminated there is one persistent buffer
>>> in queue->buffer which I have seen to increase to over 100MB and it is never
>>> getting smaller again.
>> Do you have any idea what caused the buffer to increase to 100MB in size
>> in the first place ? I would expect a full screen update would cause the
>> biggest buffer usage, and even for a 1920x1140 screen that should not
>> be anywhere near 100MB in size. IOW, i'm wondering if the 100MB usage
>> is symptomatic of a more serious bug somewhere else in the VNC code
>> that you're just masking you reducing buffer size afterwards.
> Cooked up a buffer stats patch (attached).
> Buffer sizes are nowhere near 100MB for me (as expected by Daniel).
> Individual buffers are in the 1 -> 4 MB range (1920x1080), even summed
> up this is more like 10 not 100 MB.
>
> So, big question remains why they are that big for you?

I will try the patch out. Thank you.

>
> Beside that I think it makes sense to have the shrinking logic in
> buffer_reserve too so we don't have to add buffer_shrink calls all over
> the place.

We need a possibility to shrink the buffer after it has been used.
Especially the queue->buffer.

Thanks,
Peter

Re: [Qemu-devel] [PATCH 4/4] vnc: destroy server surface if no client is connected

[Peter Lieven]

Am 03.09.2015 um 11:57 schrieb Gerd Hoffmann:
> On Do, 2015-08-27 at 12:18 +0200, Peter Lieven wrote:
>> if no client is connected there is no need to keep the server
>> surface. Throw it away and replace it with a dummy surface to
>> save memory.
> No dummy surface please.  Just set vd->server = NULL.

I can do that, but I have to check for vd->server == NULL at some points then.

Peter

Re: [Qemu-devel] [PATCH 1/4] vnc: make the Buffer capacity increase in powers of two

[Peter Lieven]

Am 03.09.2015 um 10:56 schrieb Gerd Hoffmann:
> On Do, 2015-08-27 at 12:18 +0200, Peter Lieven wrote:
>> This makes sure the number of reallocs is in O(log N).
> Looks good, applied.
>

I think there is a small error. The new capacity should be calculated as follows:

diff --git a/ui/vnc.c b/ui/vnc.c
index 8cfd2d8..79d3ff3 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -658,7 +658,7 @@ void vnc_framebuffer_update(VncState *vs, int x, int y, int w, int h,
  void buffer_reserve(Buffer *buffer, size_t len)
  {
      if ((buffer->capacity - buffer->offset) < len) {
-        buffer->capacity = pow2ceil(buffer->capacity + len);
+        buffer->capacity = pow2ceil(buffer->offset + len);
          buffer->capacity = MAX(buffer->capacity, BUFFER_MIN_INIT_SIZE);
          buffer->buffer = g_realloc(buffer->buffer, buffer->capacity);
      }


BR,
Peter

Re: [Qemu-devel] [PATCH 2/4] vnc: allow the Buffer to shrink again

[Peter Lieven]

Am 03.09.2015 um 11:52 schrieb Gerd Hoffmann:
> On Do, 2015-08-27 at 11:39 +0100, Daniel P. Berrange wrote:
>> On Thu, Aug 27, 2015 at 12:18:52PM +0200, Peter Lieven wrote:
>>> currently the Buffer can only grow. This increases Qemu memory footprint
>>> dramatically since normally the biggest VNC updates are at connection time.
>>> But also after a VNC session has terminated there is one persistent buffer
>>> in queue->buffer which I have seen to increase to over 100MB and it is never
>>> getting smaller again.
>> Do you have any idea what caused the buffer to increase to 100MB in size
>> in the first place ? I would expect a full screen update would cause the
>> biggest buffer usage, and even for a 1920x1140 screen that should not
>> be anywhere near 100MB in size. IOW, i'm wondering if the 100MB usage
>> is symptomatic of a more serious bug somewhere else in the VNC code
>> that you're just masking you reducing buffer size afterwards.
> Cooked up a buffer stats patch (attached).
> Buffer sizes are nowhere near 100MB for me (as expected by Daniel).
> Individual buffers are in the 1 -> 4 MB range (1920x1080), even summed
> up this is more like 10 not 100 MB.
>
> So, big question remains why they are that big for you?

Simple case to create at least a reasonable allocation: -vga vmware and a client that prefers Hextile encoding.

~/git/qemu$ x86_64-softmmu/qemu-system-x86_64 -m 4096 -enable-kvm -cdrom  ~/Downloads/ubuntu-14.04.3-desktop-amd64.iso -k de -usb -device usb-tablet -nodefaults -serial null -parallel null -cpu Westmere,enforce -vga vmware -monitor stdio -vnc :0
QEMU 2.4.50 monitor - type 'help' for more information
(qemu) buffer_reserve: 11/out    :    4 kB
buffer_reserve: 11/in     :    4 kB
buffer_reserve: queue     :    4 kB
buffer_reserve: queue     :    8 kB
buffer_reserve: 11/jobs   :    8 kB
buffer_reserve: 11/out    :   16 kB
buffer_reserve: queue     :   16 kB
buffer_reserve: queue     :   32 kB
buffer_reserve: queue     :   64 kB
buffer_reserve: queue     :  128 kB
buffer_reserve: queue     :  256 kB
buffer_reserve: 11/jobs   :  256 kB
buffer_reserve: 11/out    :  256 kB
buffer_reserve: queue     :  512 kB
buffer_reserve: queue     : 1024 kB
buffer_reserve: queue     : 2048 kB
buffer_reserve: 11/jobs   : 2048 kB
buffer_reserve: 11/out    : 2048 kB
buffer_reserve: queue     : 4096 kB
buffer_reserve: queue     : 8192 kB
buffer_reserve: queue     : 16384 kB
buffer_reserve: 11/jobs   : 16384 kB
buffer_reserve: 11/out    : 16384 kB

Its not 100MB, but 50MB.

Same with Tight encoding:

(qemu) buffer_reserve: 11/out    :    4 kB
buffer_reserve: 11/in     :    4 kB
buffer_reserve: queue     :    4 kB
buffer_reserve: 11/t-tight:    4 kB
buffer_reserve: 11/t-tight:    8 kB
buffer_reserve: 11/t-tight:   16 kB
buffer_reserve: 11/t-tight:   32 kB
buffer_reserve: 11/t-tight:   64 kB
buffer_reserve: 11/t-tight:  128 kB
buffer_reserve: 11/t-tight:  256 kB
buffer_reserve: 11/t-tight:  512 kB
buffer_reserve: 11/t-zlib :    4 kB
buffer_reserve: 11/jobs   :    4 kB
buffer_reserve: 11/t-zlib :   64 kB
buffer_reserve: 11/t-tight: 1024 kB
buffer_reserve: 11/t-tight: 2048 kB
buffer_reserve: 11/t-tight: 4096 kB
buffer_reserve: 11/t-tight: 8192 kB
buffer_reserve: 11/t-tight: 16384 kB
buffer_reserve: 11/t-zlib :  128 kB
buffer_reserve: queue     :    8 kB
buffer_reserve: 11/jobs   :    8 kB
buffer_reserve: 11/out    :    8 kB
buffer_reserve: 11/t-zlib :  256 kB
buffer_reserve: queue     :   64 kB
buffer_reserve: queue     :  128 kB
buffer_reserve: queue     :  256 kB
buffer_reserve: queue     :  512 kB
buffer_reserve: queue     : 1024 kB
buffer_reserve: queue     : 2048 kB
buffer_reserve: queue     : 4096 kB
buffer_reserve: queue     : 8192 kB
buffer_reserve: 11/jobs   : 8192 kB
buffer_reserve: 11/out    : 8192 kB

~40MB.

Peter

Re: [Qemu-devel] [PATCH 1/4] vnc: make the Buffer capacity increase in powers of two

[Gerd Hoffmann]

On Do, 2015-09-03 at 12:29 +0200, Peter Lieven wrote:
> -        buffer->capacity = pow2ceil(buffer->capacity + len);
> +        buffer->capacity = pow2ceil(buffer->offset + len);

Updated patch.

thanks,
  Gerd

Re: [Qemu-devel] [PATCH 4/4] vnc: destroy server surface if no client is connected

[Gerd Hoffmann]

On Do, 2015-09-03 at 12:08 +0200, Peter Lieven wrote:
> Am 03.09.2015 um 11:57 schrieb Gerd Hoffmann:
> > On Do, 2015-08-27 at 12:18 +0200, Peter Lieven wrote:
> >> if no client is connected there is no need to keep the server
> >> surface. Throw it away and replace it with a dummy surface to
> >> save memory.
> > No dummy surface please.  Just set vd->server = NULL.
> 
> I can do that, but I have to check for vd->server == NULL at some points then.

Sure.  That'll shortcut code paths which should not have any effect
anyway.  You probably also want factor out server surface initialization
into a function which is called for both first vnc connect and surface
changes.

Oh, and btw: in case the surface changes without resolution/depth
changing (guest page flip) we might simply skip surface
(re-)initialization.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/4] vnc: allow the Buffer to shrink again

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 833 bytes --]

  Hi,

> > Beside that I think it makes sense to have the shrinking logic in
> > buffer_reserve too so we don't have to add buffer_shrink calls all over
> > the place.
> 
> We need a possibility to shrink the buffer after it has been used.
> Especially the queue->buffer.

That works fine.  Test patch attached.

I'm not sure this is the way to go though.  I see alot of growing and
shrinking.  We also do alot of coping (each realloc, but also from
buffer to buffer).

We might be better off redoing the whole buffer management, at least
once we are done with encoding one frame.  Passing on a *pointer* to the
buffer, once sent to the wire just free the buffer.  Allocate a new one
for the next frame.  That way we copy around less data and also don't
have to worry about big unused buffers in the first place ...

cheers,
  Gerd


[-- Attachment #2: 0001-test-buffer-shrink-in-buffer_reserve.patch --]
[-- Type: text/x-patch, Size: 1443 bytes --]

From caa6c3b78595343c651bf4db96dc25db0b163486 Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Thu, 3 Sep 2015 13:41:39 +0200
Subject: [PATCH] [test] buffer shrink in buffer_reserve

---
 ui/vnc.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index aa6b0a5..728cbbe 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -667,6 +667,7 @@ void buffer_init(Buffer *buffer, int fd, const char *name)
 void buffer_reserve(Buffer *buffer, size_t len)
 {
     bool inc = false;
+    bool dec = false;
 
     assert(buffer->name);
     if ((buffer->capacity - buffer->offset) < len) {
@@ -675,9 +676,17 @@ void buffer_reserve(Buffer *buffer, size_t len)
         buffer->buffer = g_realloc(buffer->buffer, buffer->capacity);
         inc = true;
     }
-    if (inc) {
-        fprintf(stderr, "%s: %-10s: %4zd kB\n", __func__,
-                buffer->name, buffer->capacity / 1024);
+    if (((buffer->offset + len) < buffer->capacity >> 4) &&
+        (buffer->capacity > 64 * 1024)) {
+        buffer->capacity >>= 1;
+        buffer->capacity = MAX(buffer->capacity, 64 * 1024);
+        buffer->buffer = g_realloc(buffer->buffer, buffer->capacity);
+        dec = true;
+    }
+    if (inc || dec) {
+        fprintf(stderr, "%s: %-10s: %4zd kB %s\n", __func__,
+                buffer->name, buffer->capacity / 1024,
+                inc ? "+" : "-");
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/4] vnc: allow the Buffer to shrink again

[Gerd Hoffmann]

  Hi,

> buffer_reserve: 11/t-tight: 4096 kB
> buffer_reserve: 11/t-tight: 8192 kB
> buffer_reserve: 11/t-tight: 16384 kB

A bit more than what I see (4096 usually, 8192 once).  No idea why.

> buffer_reserve: queue     : 8192 kB
> buffer_reserve: 11/jobs   : 8192 kB
> buffer_reserve: 11/out    : 8192 kB

Noticeable more than what I see.

Is this local or remote client?  Maybe these stay smaller for me (local
client) simply because qemu can put the data to the wire faster.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 2/4] vnc: allow the Buffer to shrink again

[Peter Lieven]

Am 03.09.2015 um 13:57 schrieb Gerd Hoffmann:
>    Hi,
>
>> buffer_reserve: 11/t-tight: 4096 kB
>> buffer_reserve: 11/t-tight: 8192 kB
>> buffer_reserve: 11/t-tight: 16384 kB
> A bit more than what I see (4096 usually, 8192 once).  No idea why.
>
>> buffer_reserve: queue     : 8192 kB
>> buffer_reserve: 11/jobs   : 8192 kB
>> buffer_reserve: 11/out    : 8192 kB
> Noticeable more than what I see.
>
> Is this local or remote client?  Maybe these stay smaller for me (local
> client) simply because qemu can put the data to the wire faster.

Local. But maybe you don't have a fullscreen background image in
the vserver?

Peter

Re: [Qemu-devel] [PATCH 2/4] vnc: allow the Buffer to shrink again

[Peter Lieven]

Am 03.09.2015 um 13:52 schrieb Gerd Hoffmann:
>    Hi,
>
>>> Beside that I think it makes sense to have the shrinking logic in
>>> buffer_reserve too so we don't have to add buffer_shrink calls all over
>>> the place.
>> We need a possibility to shrink the buffer after it has been used.
>> Especially the queue->buffer.
> That works fine.  Test patch attached.

Not completely. queue->buffer may remain big, if you disconnect in the
wrong moment. Maybe there should be a buffer_free if the last client
disconnects?

In general I like your modified patch because the shrinking is slower than
in my version. for 64*1024 you should introduce a macro.

>
> I'm not sure this is the way to go though.  I see alot of growing and
> shrinking.  We also do alot of coping (each realloc, but also from
> buffer to buffer).
>
> We might be better off redoing the whole buffer management, at least
> once we are done with encoding one frame.  Passing on a *pointer* to the
> buffer, once sent to the wire just free the buffer.  Allocate a new one
> for the next frame.  That way we copy around less data and also don't
> have to worry about big unused buffers in the first place ...

Maybe this should be the permanent solution.

Peter