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

[Arnon Gilboa]

ACK

Hans de Goede wrote:
> 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;
>