[Spice-devel] [PATCH 01/16] UpgradeItem: use base PipeItem for refcounting
Frediano Ziglio
fziglio at redhat.com
Thu Apr 21 11:26:28 UTC 2016
>
> On Wed, 2016-04-20 at 04:18 -0400, Frediano Ziglio wrote:
> > >
> > > No need to re-implement refcounting in this subclass. However, I needed
> > > to add a new 'dcc' member to UpgradeItem to be able to unref properly.
> > > ---
> > > Change since last patch: take ref on dcc
> > >
> > > server/dcc.c | 18 +-----------------
> > > server/display-channel.c | 4 +---
> > > server/display-channel.h | 2 +-
> > > server/stream.c | 18 ++++++++++++++++--
> > > 4 files changed, 19 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/server/dcc.c b/server/dcc.c
> > > index 91c3f82..5193c17 100644
> > > --- a/server/dcc.c
> > > +++ b/server/dcc.c
> > > @@ -1591,29 +1591,15 @@ int dcc_handle_migrate_data(DisplayChannelClient
> > > *dcc, uint32_t size, void *mess
> > > return TRUE;
> > > }
> > >
> > > -static void upgrade_item_unref(DisplayChannel *display, UpgradeItem
> > > *item)
> > > -{
> > > - if (--item->refs != 0)
> > > - return;
> > > -
> > > - display_channel_drawable_unref(display, item->drawable);
> > > - free(item->rects);
> > > - free(item);
> > > -}
> > > -
> > > static void release_item_after_push(DisplayChannelClient *dcc, PipeItem
> > > *item)
> > > {
> > > - DisplayChannel *display = DCC_TO_DC(dcc);
> > > -
> > > switch (item->type) {
> > > case PIPE_ITEM_TYPE_DRAW:
> > > case PIPE_ITEM_TYPE_IMAGE:
> > > case PIPE_ITEM_TYPE_STREAM_CLIP:
> > > case PIPE_ITEM_TYPE_MONITORS_CONFIG:
> > > - pipe_item_unref(item);
> > > - break;
> > > case PIPE_ITEM_TYPE_UPGRADE:
> > > - upgrade_item_unref(display, (UpgradeItem *)item);
> > > + pipe_item_unref(item);
> > > break;
> > > case PIPE_ITEM_TYPE_GL_SCANOUT:
> > > case PIPE_ITEM_TYPE_GL_DRAW:
> > > @@ -1651,8 +1637,6 @@ static void
> > > release_item_before_push(DisplayChannelClient *dcc, PipeItem *item)
> > > }
> > > case PIPE_ITEM_TYPE_STREAM_CLIP:
> > > case PIPE_ITEM_TYPE_UPGRADE:
> > > - upgrade_item_unref(display, (UpgradeItem *)item);
> > > - break;
> > > case PIPE_ITEM_TYPE_IMAGE:
> > > case PIPE_ITEM_TYPE_MONITORS_CONFIG:
> > > pipe_item_unref(item);
> > > diff --git a/server/display-channel.c b/server/display-channel.c
> > > index 88dbc74..98b4a43 100644
> > > --- a/server/display-channel.c
> > > +++ b/server/display-channel.c
> > > @@ -1973,10 +1973,8 @@ static void hold_item(RedChannelClient *rcc,
> > > PipeItem
> > > *item)
> > > case PIPE_ITEM_TYPE_DRAW:
> > > case PIPE_ITEM_TYPE_IMAGE:
> > > case PIPE_ITEM_TYPE_STREAM_CLIP:
> > > - pipe_item_ref(item);
> > > - break;
> > > case PIPE_ITEM_TYPE_UPGRADE:
> > > - ((UpgradeItem *)item)->refs++;
> > > + pipe_item_ref(item);
> > > break;
> > > default:
> > > spice_warn_if_reached();
> > > diff --git a/server/display-channel.h b/server/display-channel.h
> > > index 6b053de..8944f06 100644
> > > --- a/server/display-channel.h
> > > +++ b/server/display-channel.h
> > > @@ -244,7 +244,7 @@ typedef struct SurfaceDestroyItem {
> > >
> > > typedef struct UpgradeItem {
> > > PipeItem base;
> > > - int refs;
> > > + DisplayChannelClient *dcc;
> >
> > Here a display channel is enough I think.
> >
> > > Drawable *drawable;
> > > SpiceClipRects *rects;
> > > } UpgradeItem;
> > > diff --git a/server/stream.c b/server/stream.c
> > > index ae37a62..80cfe2e 100644
> > > --- a/server/stream.c
> > > +++ b/server/stream.c
> > > @@ -758,6 +758,18 @@ void stream_agent_stop(StreamAgent *agent)
> > > }
> > > }
> > >
> > > +static void upgrade_item_free(UpgradeItem *item)
> > > +{
> > > + g_return_if_fail(item != NULL);
> > > + DisplayChannel *display = DCC_TO_DC(item->dcc);
> > > + g_return_if_fail(item->base.refcount != 0);
> > > +
> > > + display_channel_drawable_unref(display, item->drawable);
> >
> > I'm still pondering this.
> > My initial question was mainly a question (even to me) than a
> > proposal. Here the problem is that this can create a circular
> > reference counting. The problem is quite hidden by the fact that
> > we don't still have a way to free part of our stuff (DisplayChannel
> > is one). This make hard to verify this, I should have pushed the
> > inclusion of my cleanup branch when was refused.
> >
> > Jonathon... did you test for leaks?
>
> No I didn't. I did just now and didn't notice any leaks. But you're right
> that
> it basically creates a circular ref, which could be problematic. It would
> probably be better to use a weak ref, although I can't do until after the
> GObject conversions have been merged. What if I remove the ref and add a
> FIXME
> to use a weak ref in the future?
>
> >
> > > + free(item->rects);
> > > + red_channel_client_unref(RED_CHANNEL_CLIENT(item->dcc));
> > > + free(item);
> > > +}
> > > +
> > > /*
> > > * after dcc_detach_stream_gracefully is called for all the display
> > > channel
> > > clients,
> > > * detach_stream should be called. See comment (1).
> > > @@ -798,10 +810,12 @@ static void
> > > dcc_detach_stream_gracefully(DisplayChannelClient *dcc,
> > > rect_debug(&stream->current->red_drawable->bbox);
> > > rcc = RED_CHANNEL_CLIENT(dcc);
> > > upgrade_item = spice_new(UpgradeItem, 1);
> > > - upgrade_item->refs = 1;
> > > - pipe_item_init(&upgrade_item->base, PIPE_ITEM_TYPE_UPGRADE);
> > > + pipe_item_init_full(&upgrade_item->base, PIPE_ITEM_TYPE_UPGRADE,
> > > + (GDestroyNotify)upgrade_item_free);
> > > upgrade_item->drawable = stream->current;
> > > upgrade_item->drawable->refs++;
> > > + upgrade_item->dcc = dcc;
> > > + red_channel_client_ref(RED_CHANNEL_CLIENT(upgrade_item->dcc));
> > > n_rects =
> > > pixman_region32_n_rects(&upgrade_item->drawable
> > > ->tree_item.base.rgn);
> > > upgrade_item->rects = spice_malloc_n_m(n_rects,
> > > sizeof(SpiceRect),
> > > sizeof(SpiceClipRects));
> > > upgrade_item->rects->num_rects = n_rects;
> >
I have some though about this.
First: we just need a DisplayChannel to free the bound related
to the item and DisplayChannel has currently infinite life (and
surely the lifetime SHOULD be more than the item even in the
future).
I had some high level though about PipeItem stuff.
"What's a PipeItem?"
I think the initial definition was something like:
"A RedChannel have an associated Pipe formed by PipeItems which
should be sent to the client."
(this could be ignore the items FROM the client).
Now... there are not a well defined "Pipe", (a C structure or
similar) which make the PipeItem C definition a bit hard to
understand.
I think PipeItem and Pipe idea should be similar to a List and
a ListItem. Usually a ListItem in C is a structure which is
always contained in a List and contains no reference counting
as there is a strict 1-N relationship between ListItem and List.
What does that mean? If you need to access the List from a ListItem
you can safely store a pointer from ListItem to the List as the
lifetime of the List if bigger than ListItem.
Back to our PipeItem. Why we had to have a reference counter?
The reason is that someone wants to extend the life independently
from the PipeItem. This have the consequence that the PipeItem
can now live more than the Pipe which break a bit the similarity
with the simple ListItem described before. So a name question
could be: "If PipeItem is not an item of a Pipe... what is it?"
Silly question but I think the reply is: "a message" (refer to
the relationship with spice.proto messages, are quite 1-to-1).
I have still to answer the question "Why we need a reference
counter in PipeItem?" (note: this does not means that adding
the reference counting to the base class is wrong as we use it!).
Currently the reference counter is quite spread in the code...
I doubt it's usage is wrong and PipeItem should be like the ListItem.
Weak pointer. The introduction of a weak pointer as dcc basically
means: "UpgradeItem can live more than DCC". If this is false...
we don't need a weak pointer but a plain one is perfectly fine.
A weak pointer can be implemented as a simple pointer which is
reset when deleted from the dcc (no need to wait GObject).
Surely as said DC is currently safe to be stored so I'll
post an updated patch to this one.
As the other questions on this post... if somebody have
considerations/replies feel free to comment, I don't know
if I'll be able to reply soon.
Frediano
More information about the Spice-devel
mailing list