[Spice-devel] RedPipeItem lifespan, past, present and ... bug

[Frediano Ziglio]

> 
> On Thu, 2016-05-19 at 18:25 +0200, Christophe Fergeau wrote:
> > Hey,
> > 
> > On Thu, May 19, 2016 at 06:14:52AM -0400, Frediano Ziglio wrote:
> > > Possible future changes (please comment):
> > > - would be good if red_channel_client_wait_pipe_item_sent could work even
> > >   without the hold_item, one possible implementation is adding a fake
> > > RedPipeItem
> > >   which when removed will set a flag causing the loop to exit (better
> > >   would
> > > be
> > >   to remove the function and implement this stuff in another way, there
> > >   are
> > > only
> > >   a single call to make sure there are no pending drawing on a surface);
> > 
> > Yeah, I was wondering too if a fake pipe item could be used to be
> > notified when the preceding pipe item has been sent.
> 
> With GObject, we could potentially add an "item-sent" signal that is emitted
> whenever an item is sent. Then wait_pipe_item_sent() could run a loop while
> handling this signal to and quit the loop when the appropriate item is sent?
> 

I think GObject is overkilling, this path is really cold, this code is
executed only when a surface is destroyed and not even for each destroy.
Adding a single bit just to support this is a waste of resources.
I posted a patch that implement this using a "marker" item.

> > 
> > > - remove the RingItem and make possible to add the item to multiple
> > > clients,
> > > this
> > >   IMHO would make stuff much easier to understand;
> 
> yes.
> 

Posted some patches for type safety in order to be able to remove
the link field safely without code crashing in unexplored paths.

> > > - remove hold_item and use always red_pipe_item_ref;
> > 
> > Yup, I would do that, hold_item really looks like a convoluted _ref()
> > used when not all pipe items had a refcount.
> 
> agreed
> 

Posted a patch for this.

> > 
> > > - add another callback to RedPipeItem to send the item to make all
> > > send_item
> > >   callbacks really small;
> > 
> > It probably makes sense too, maybe longer term. I don't think this
> > is strictly related to the bug you discussed earlier though.
> 
> hmm, what would this new callback do? marshall the pipe item?
> 

Yes, but it's future work I think.

> > 
> > 
> > > - remove atomic operation on RedPipeItem reference counter, I think was
> > > added
> > >   to make sure there was no thread issues but it's just slowing down big
> > > servers
> > >   with many cores.
> > 
> > Imo this would need to go with annotations saying in which thread the code
> > is running (thinking about the display thread). Probably not a huge
> > issue right now.
> > 

Obviously I have a patch even for this (not that hard...)

Have no idea how to solve the CharDevice issue (not a big issue till we don't
support multiple clients). If you look at the code (main_channel_client_push_agent_data)
there are still RedPipeItem which points to RedPipeItem, I think this is
mainly wrong. CharDevice expect a RedPipeItem for messages but only uses for
reference counting (which is even wrong!).
I was thinking about either:
- add a RedDataBuffer to pass data to CharDevice, something like
  struct RedDataBuffer {
     int refcount;
     void (*free)(struct RedDataBuffer);
     void *data;
     uint32_t data_len;
  }
- removing link from RedPipeItem (to make possible to add the same item to
  multiple pipes). And of course rename RedPipeItem to RedMessage or something
  like that. In the future I think this is the right direction however
  requires some analysis also on the best structures to use (I would
  like to understand the order of the data structure we are using but
  this depends also on the different pipes length based on different
  usages and statistics, mostly Ring functions are O(1) which is good)

I think I'll move to other stuff for now.

Frediano