[Spice-devel] [PATCH spice] spicec: Do not try to do accounting of pci memory

Hans de Goede hdegoede at redhat.com
Sat Oct 9 13:33:01 PDT 2010


Without this patch spicec reproducely hangs in
GlzDecoderWindow::pre_decode_update_window().

When GlzDecoderWindow::will_overflow() returns true,
GlzDecoderWindow::pre_decode_update_window(),
waits for a call to GlzDecoderWindow::post_decode()
to free up some memory

This happens even though there still is pci memory
available (otherwise the driver would not have
been able to send an image to decode in the first
place).

The GlzDecoderWindow::post_decode() call never happens
as the server is waiting for a reply to the decode
of the hanging image, causing the client to hang
for ever.

This patch fixes this by simply removing the
"attempted" pci memory accounting. As there is no
need for that, as the driver already must keep
track of pci memory usage.

I've verified that both the old and new Xorg drivers
take care of not overusing the pci memory themselves
I would expect the same to be true for the windows
driver.

Note the calculating of the glz_window_size in
red_client.cpp cannot be removed as the calculated
value is send as part of the SpiceMsgcDisplayInit on
connect.
---
 client/glz_decoder_window.cpp |   35 ++---------------------------------
 client/glz_decoder_window.h   |    6 +-----
 client/red_client.cpp         |    3 +--
 3 files changed, 4 insertions(+), 40 deletions(-)

diff --git a/client/glz_decoder_window.cpp b/client/glz_decoder_window.cpp
index 24dfce3..ab081f8 100644
--- a/client/glz_decoder_window.cpp
+++ b/client/glz_decoder_window.cpp
@@ -21,21 +21,12 @@
 #include "utils.h"
 
 #define INIT_IMAGES_CAPACITY 100
-#define WIN_OVERFLOW_FACTOR 1.5
 #define WIN_REALLOC_FACTOR 1.5
 
-GlzDecoderWindow::GlzDecoderWindow(int pixels_capacity, GlzDecoderDebug &debug_calls)
-    : _pixels_capacity (pixels_capacity)
-    , _aborting (false)
+GlzDecoderWindow::GlzDecoderWindow(GlzDecoderDebug &debug_calls)
+    : _aborting (false)
     , _debug_calls (debug_calls)
 {
-    if (_pixels_capacity > LZ_MAX_WINDOW_SIZE) {
-        std::string erro_str;
-        string_printf(erro_str, "Glz Window capacity exceeds the limit %d",
-                      _pixels_capacity);
-        _debug_calls.error(erro_str);
-    }
-
     _images_capacity = INIT_IMAGES_CAPACITY;
     _images = new  GlzDecodedImage*[_images_capacity];
     if (!_images) {
@@ -105,19 +96,6 @@ void GlzDecoderWindow::clear()
     init();
 }
 
-void GlzDecoderWindow::set_pixels_capacity(int pixels_capacity)
-{
-    Lock lock(_win_modifiers_mutex);
-
-    if (pixels_capacity > LZ_MAX_WINDOW_SIZE) {
-        std::string erro_str;
-        string_printf(erro_str, "Glz Window capacity exceeds the limit %d",
-                      pixels_capacity);
-        _debug_calls.error(erro_str);
-    }
-    _pixels_capacity = pixels_capacity;
-}
-
 void GlzDecoderWindow::init()
 {
     _missing_list.clear();
@@ -127,7 +105,6 @@ void GlzDecoderWindow::init()
     _head_idx = 0;
     _tail_image_id = 0;
     _n_images = 1;
-    _n_pixels = 0;
 }
 
 void GlzDecoderWindow::release_images()
@@ -146,18 +123,12 @@ inline bool GlzDecoderWindow::is_empty()
     return (!_n_images);
 }
 
-/* approximated overflow. Considers only the size that currently occupies the window and
-   not the size of the missing images. TODO: consider other measures */
 inline bool GlzDecoderWindow::will_overflow(uint64_t image_id, uint64_t relative_head_id)
 {
     if (image_id <= _tail_image_id) {
         return false;
     }
 
-    if (_n_pixels > (WIN_OVERFLOW_FACTOR * _pixels_capacity)) {
-        return true;
-    }
-
     if (!_missing_list.empty() && (_missing_list.front() < relative_head_id)) {
         // two non overlapping windows
         return true;
@@ -276,7 +247,6 @@ void GlzDecoderWindow::add_decoded_image(GlzDecodedImage *image)
     Lock lock(_new_image_mutex);
     GLZ_ASSERT(_debug_calls, image->get_id() <= _tail_image_id);
     _images[calc_image_win_idx(image->get_id())] = image;
-    _n_pixels += image->get_size();
     _new_image_cond.notify_all();
 }
 
@@ -339,7 +309,6 @@ inline void GlzDecoderWindow::remove_head(uint64_t new_head_image_id)
 
     for (int i = 0; i < n_images_remove; i++) {
         int index = (_head_idx + i) % _images_capacity;
-        _n_pixels -= _images[index]->get_size();
         delete _images[index];
         _images[index] = NULL;
     }
diff --git a/client/glz_decoder_window.h b/client/glz_decoder_window.h
index a2848bb..e061c73 100644
--- a/client/glz_decoder_window.h
+++ b/client/glz_decoder_window.h
@@ -33,7 +33,7 @@ typedef int DecodedImageWinId;
 
 class GlzDecoderWindow {
 public:
-    GlzDecoderWindow(int pixels_capacity, GlzDecoderDebug &debug_calls);
+    GlzDecoderWindow(GlzDecoderDebug &debug_calls);
     virtual ~GlzDecoderWindow();
 
     DecodedImageWinId pre_decode(uint64_t image_id, uint64_t relative_head_id);
@@ -50,8 +50,6 @@ public:
     /* NOTE - clear mustn't be called if the window is currently used by a decoder*/
     void clear();
 
-    void set_pixels_capacity(int pixels_capacity);
-
 private:
     void wait_for_image(int index);
     void add_image(GlzDecodedImage *image);
@@ -75,14 +73,12 @@ private:
     void release_images();
 
 private:
-    int _pixels_capacity;
     GlzDecodedImage **_images; // cyclic window
     int _head_idx;            // index in images array (not image id)
     uint64_t _tail_image_id;
     int _images_capacity;
     int _n_images;            // _n_images counts all the images in
                               // the window, including the missing ones
-    uint64_t _n_pixels;
 
     std::list<uint64_t> _missing_list;
 
diff --git a/client/red_client.cpp b/client/red_client.cpp
index 61974b5..b9e1421 100644
--- a/client/red_client.cpp
+++ b/client/red_client.cpp
@@ -367,7 +367,7 @@ RedClient::RedClient(Application& application)
     , _agent_caps_size(0)
     , _agent_caps(NULL)
     , _migrate (*this)
-    , _glz_window (0, _glz_debug)
+    , _glz_window (_glz_debug)
 {
     Platform::set_clipboard_listener(this);
     MainChannelLoop* message_loop = static_cast<MainChannelLoop*>(get_message_handler());
@@ -951,7 +951,6 @@ void RedClient::handle_init(RedPeer::InMessage* message)
     _connection_id = init->session_id;
     set_mm_time(init->multi_media_time);
     calc_pixmap_cach_and_glz_window_size(init->display_channels_hint, init->ram_hint);
-    _glz_window.set_pixels_capacity(_glz_window_size);
     set_mouse_mode(init->supported_mouse_modes, init->current_mouse_mode);
     _agent_tokens = init->agent_tokens;
     _agent_connected = !!init->agent_connected;
-- 
1.7.2.2



More information about the Spice-devel mailing list