[Spice-devel,01/30] Do not release too much drawables

Submitted by Frediano Ziglio on June 7, 2016, 10:17 a.m.

Details

Message ID 1465294688-14978-2-git-send-email-fziglio@redhat.com
State New
Headers show
Series "Better encapsulation of image encoding stuff" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio June 7, 2016, 10:17 a.m.
Accumulate counter, do not reset for each client.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
 server/dcc-encoders.c    | 5 ++---
 server/dcc-encoders.h    | 3 ++-
 server/display-channel.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
index 5570798..cc235fa 100644
--- a/server/dcc-encoders.c
+++ b/server/dcc-encoders.c
@@ -532,13 +532,12 @@  void dcc_free_glz_drawable(DisplayChannelClient *dcc, RedGlzDrawable *drawable)
  * Drawable (their qxl drawables are released too).
  * NOTE - the caller should prevent encoding using the dictionary during the operation
  */
-int dcc_free_some_independent_glz_drawables(DisplayChannelClient *dcc)
+int dcc_free_some_independent_glz_drawables(DisplayChannelClient *dcc, int n)
 {
     RingItem *ring_link;
-    int n = 0;
 
     if (!dcc) {
-        return 0;
+        return n;
     }
     ring_link = ring_get_head(&dcc->glz_drawables);
     while ((n < RED_RELEASE_BUNCH_SIZE) && (ring_link != NULL)) {
diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
index 0d3e96a9..fed8d58 100644
--- a/server/dcc-encoders.h
+++ b/server/dcc-encoders.h
@@ -40,7 +40,8 @@  void             dcc_encoders_init                           (DisplayChannelClie
 void             dcc_encoders_free                           (DisplayChannelClient *dcc);
 void             dcc_free_glz_drawable                       (DisplayChannelClient *dcc,
                                                               RedGlzDrawable *drawable);
-int              dcc_free_some_independent_glz_drawables     (DisplayChannelClient *dcc);
+int              dcc_free_some_independent_glz_drawables     (DisplayChannelClient *dcc,
+                                                              int release_count);
 void             dcc_free_glz_drawables                      (DisplayChannelClient *dcc);
 void             dcc_free_glz_drawables_to_free              (DisplayChannelClient* dcc);
 void             dcc_freeze_glz                              (DisplayChannelClient *dcc);
diff --git a/server/display-channel.c b/server/display-channel.c
index 2888cad..db059b4 100644
--- a/server/display-channel.c
+++ b/server/display-channel.c
@@ -1267,7 +1267,7 @@  void display_channel_free_some(DisplayChannel *display)
             // encoding using the dictionary is prevented since the following operations might
             // change the dictionary
             pthread_rwlock_wrlock(&glz_dict->encode_lock);
-            n = dcc_free_some_independent_glz_drawables(dcc);
+            n = dcc_free_some_independent_glz_drawables(dcc, n);
         }
     }
 

Comments

I have to admit that I'm not very familiar with this bit of the code. But it's
clearly a behavior change and it seems like one that could have non-obvious or
hard-to-test consequences. Can you give a bit more justification for the change?
Does it fix an observed issue? It seems that these glz drawables are specific to
each client, so it doesn't seem "wrong" to free the same number for each
client. 

Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>


On Tue, 2016-06-07 at 11:17 +0100, Frediano Ziglio wrote:
> Accumulate counter, do not reset for each client.
> 
> Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> ---
>  server/dcc-encoders.c    | 5 ++---
>  server/dcc-encoders.h    | 3 ++-
>  server/display-channel.c | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> index 5570798..cc235fa 100644
> --- a/server/dcc-encoders.c
> +++ b/server/dcc-encoders.c
> @@ -532,13 +532,12 @@ void dcc_free_glz_drawable(DisplayChannelClient *dcc,
> RedGlzDrawable *drawable)
>   * Drawable (their qxl drawables are released too).
>   * NOTE - the caller should prevent encoding using the dictionary during the
> operation
>   */
> -int dcc_free_some_independent_glz_drawables(DisplayChannelClient *dcc)
> +int dcc_free_some_independent_glz_drawables(DisplayChannelClient *dcc, int n)
>  {
>      RingItem *ring_link;
> -    int n = 0;
>  
>      if (!dcc) {
> -        return 0;
> +        return n;
>      }
>      ring_link = ring_get_head(&dcc->glz_drawables);
>      while ((n < RED_RELEASE_BUNCH_SIZE) && (ring_link != NULL)) {
> diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
> index 0d3e96a9..fed8d58 100644
> --- a/server/dcc-encoders.h
> +++ b/server/dcc-encoders.h
> @@ -40,7 +40,8 @@
> void             dcc_encoders_init                           (DisplayChannelCl
> ie
>  void             dcc_encoders_free                           (DisplayChannelC
> lient *dcc);
>  void             dcc_free_glz_drawable                       (DisplayChannelC
> lient *dcc,
>                                                                RedGlzDrawable
> *drawable);
> -int              dcc_free_some_independent_glz_drawables     (DisplayChannelC
> lient *dcc);
> +int              dcc_free_some_independent_glz_drawables     (DisplayChannelC
> lient *dcc,
> +                                                              int
> release_count);
>  void             dcc_free_glz_drawables                      (DisplayChannelC
> lient *dcc);
>  void             dcc_free_glz_drawables_to_free              (DisplayChannelC
> lient* dcc);
>  void             dcc_freeze_glz                              (DisplayChannelC
> lient *dcc);
> diff --git a/server/display-channel.c b/server/display-channel.c
> index 2888cad..db059b4 100644
> --- a/server/display-channel.c
> +++ b/server/display-channel.c
> @@ -1267,7 +1267,7 @@ void display_channel_free_some(DisplayChannel *display)
>              // encoding using the dictionary is prevented since the following
> operations might
>              // change the dictionary
>              pthread_rwlock_wrlock(&glz_dict->encode_lock);
> -            n = dcc_free_some_independent_glz_drawables(dcc);
> +            n = dcc_free_some_independent_glz_drawables(dcc, n);
>          }
>      }
>
> 
> I have to admit that I'm not very familiar with this bit of the code. But
> it's
> clearly a behavior change and it seems like one that could have non-obvious
> or
> hard-to-test consequences. Can you give a bit more justification for the
> change?
> Does it fix an observed issue? It seems that these glz drawables are specific
> to
> each client, so it doesn't seem "wrong" to free the same number for each
> client.
> 
> Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
> 

I'm more confident but after some consideration I think the code is
wrong with and without the patch, just not problem happens as there
is only one possible client so the result does not change.

The limit counter is expressed in number of Drawables while the
function I patched uses number of RedGlzDrawable!

I should write some document about these Glz stuff, I think I'll
move the patch, not really specific to this patchset.

Frediano

> 
> On Tue, 2016-06-07 at 11:17 +0100, Frediano Ziglio wrote:
> > Accumulate counter, do not reset for each client.
> > 
> > Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
> > ---
> >  server/dcc-encoders.c    | 5 ++---
> >  server/dcc-encoders.h    | 3 ++-
> >  server/display-channel.c | 2 +-
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> > index 5570798..cc235fa 100644
> > --- a/server/dcc-encoders.c
> > +++ b/server/dcc-encoders.c
> > @@ -532,13 +532,12 @@ void dcc_free_glz_drawable(DisplayChannelClient *dcc,
> > RedGlzDrawable *drawable)
> >   * Drawable (their qxl drawables are released too).
> >   * NOTE - the caller should prevent encoding using the dictionary during
> >   the
> > operation
> >   */
> > -int dcc_free_some_independent_glz_drawables(DisplayChannelClient *dcc)
> > +int dcc_free_some_independent_glz_drawables(DisplayChannelClient *dcc, int
> > n)
> >  {
> >      RingItem *ring_link;
> > -    int n = 0;
> >  
> >      if (!dcc) {
> > -        return 0;
> > +        return n;
> >      }
> >      ring_link = ring_get_head(&dcc->glz_drawables);
> >      while ((n < RED_RELEASE_BUNCH_SIZE) && (ring_link != NULL)) {
> > diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
> > index 0d3e96a9..fed8d58 100644
> > --- a/server/dcc-encoders.h
> > +++ b/server/dcc-encoders.h
> > @@ -40,7 +40,8 @@
> > void             dcc_encoders_init
> >                            (DisplayChannelCl
> > ie
> >  void             dcc_encoders_free
> >                             (DisplayChannelC
> > lient *dcc);
> >  void             dcc_free_glz_drawable
> >                         (DisplayChannelC
> > lient *dcc,
> >                                                                RedGlzDrawable
> > *drawable);
> > -int              dcc_free_some_independent_glz_drawables
> >      (DisplayChannelC
> > lient *dcc);
> > +int              dcc_free_some_independent_glz_drawables
> >      (DisplayChannelC
> > lient *dcc,
> > +                                                              int
> > release_count);
> >  void             dcc_free_glz_drawables
> >                        (DisplayChannelC
> > lient *dcc);
> >  void             dcc_free_glz_drawables_to_free
> >                (DisplayChannelC
> > lient* dcc);
> >  void             dcc_freeze_glz
> >                                (DisplayChannelC
> > lient *dcc);
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 2888cad..db059b4 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1267,7 +1267,7 @@ void display_channel_free_some(DisplayChannel
> > *display)
> >              // encoding using the dictionary is prevented since the
> >              following
> > operations might
> >              // change the dictionary
> >              pthread_rwlock_wrlock(&glz_dict->encode_lock);
> > -            n = dcc_free_some_independent_glz_drawables(dcc);
> > +            n = dcc_free_some_independent_glz_drawables(dcc, n);
> >          }
> >      }
> >  
>