>
> On Wed, 2016-06-15 at 10:37 +0100, Frediano Ziglio wrote:
> > Signed-off-by: Frediano Ziglio <fziglio@xxxxxxxxxx>
> > ---
> > server/dcc-encoders.c | 14 ++++++++++++++
> > server/dcc-encoders.h | 2 ++
> > server/display-channel.c | 18 +++++-------------
> > 3 files changed, 21 insertions(+), 13 deletions(-)
> >
> > diff --git a/server/dcc-encoders.c b/server/dcc-encoders.c
> > index 6fccb17..a5bc328 100644
> > --- a/server/dcc-encoders.c
> > +++ b/server/dcc-encoders.c
> > @@ -548,6 +548,20 @@ static void red_glz_drawable_free(RedGlzDrawable
> > *glz_drawable)
> > }
> > }
> >
> > +void image_encoders_glz_encode_lock(ImageEncoders *enc)
> > +{
> > + if (enc->glz_dict) {
> > + pthread_rwlock_wrlock(&enc->glz_dict->encode_lock);
> > + }
> > +}
> > +
> > +void image_encoders_glz_encode_unlock(ImageEncoders *enc)
> > +{
> > + if (enc->glz_dict) {
> > + pthread_rwlock_unlock(&enc->glz_dict->encode_lock);
> > + }
> > +}
> > +
> > /*
> > * Remove from the global lz dictionary some glz_drawables that have no
> > reference to
> > * Drawable (their qxl drawables are released too).
> > diff --git a/server/dcc-encoders.h b/server/dcc-encoders.h
> > index a38de41..e13c809 100644
> > --- a/server/dcc-encoders.h
> > +++ b/server/dcc-encoders.h
> > @@ -48,6 +48,8 @@ void
> > image_encoders_free_glz_drawables_to_free(ImageEncoders* enc);
> > gboolean image_encoders_glz_create(ImageEncoders *enc, uint8_t id);
> > void image_encoders_glz_get_restore_data(ImageEncoders *enc,
> > uint8_t *out_id,
> > GlzEncDictRestoreData *out_data);
> > +void image_encoders_glz_encode_lock(ImageEncoders *enc);
> > +void image_encoders_glz_encode_unlock(ImageEncoders *enc);
> > void drawable_ring_free_glz_drawables(Ring *drawable_ring);
> > void drawable_ring_detach_glz_drawables(Ring *drawable_ring);
> >
> > diff --git a/server/display-channel.c b/server/display-channel.c
> > index 1b5a03a..15da137 100644
> > --- a/server/display-channel.c
> > +++ b/server/display-channel.c
> > @@ -1206,14 +1206,10 @@ void display_channel_free_some(DisplayChannel
> > *display)
> > spice_debug("#draw=%d, #glz_draw=%d", display->drawable_count,
> > display->encoder_globals.glz_drawable_count);
> > FOREACH_CLIENT(display, link, next, dcc) {
> > - GlzSharedDictionary *glz_dict = dcc->encoders.glz_dict;
> > -
> > - if (glz_dict) {
> > - // encoding using the dictionary is prevented since the
> > following
> > operations might
> > - // change the dictionary
> > - pthread_rwlock_wrlock(&glz_dict->encode_lock);
> > - n = image_encoders_free_some_independent_glz_drawables(&dcc-
> > >encoders);
> > - }
> > + // encoding using the dictionary is prevented since the following
> > operations might
> > + // change the dictionary
> > + image_encoders_glz_encode_lock(&dcc->encoders);
> > + n = image_encoders_free_some_independent_glz_drawables(&dcc-
> > >encoders);
>
> This is a change of behavior. Previously, we only called
> _free_some_independent_glz_drawables() if glz_dict was non-NULL. Now we call
> it
> regardless of whether it's NULL or not. If that's intentional, an explanation
> (at least in the commit log) is warranted.
>
Fixed in following version.
> > }
> >
> > while (!ring_is_empty(&display->current_list) && n++ <
> > RED_RELEASE_BUNCH_SIZE) {
> > @@ -1221,11 +1217,7 @@ void display_channel_free_some(DisplayChannel
> > *display)
> > }
> >
> > FOREACH_CLIENT(display, link, next, dcc) {
> > - GlzSharedDictionary *glz_dict = dcc->encoders.glz_dict;
> > -
> > - if (glz_dict) {
> > - pthread_rwlock_unlock(&glz_dict->encode_lock);
> > - }
> > + image_encoders_glz_encode_unlock(&dcc->encoders);
>
> I don't think there's currently any way that encoders->glz_dict might be
> changed
> between the lock above and the unlock here, but the fact that you no longer
> cache the glz_dict value in a local variable opens us up to this risk. If
> enc-
> >glz_dict was NULL when we called image_encoders_glz_encode_lock() and then
> >was
> set to non-NULL between there and here, calling _unlock() here will attempt
> to
> unlock a lock that had not been locked. Maybe this is not possible and not
> worth
> worrying about, but I thought I'd mention it.
>
No, this is exactly as before, glz_dict local is computed again on
the second loop.
> > }
> > }
> >
>
> Reviewed-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
>
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel