[PATCH v2 wayland] shm: Add shm_buffer ref and shm_pool unref functions

[Derek Foreman]

On 16/10/15 08:34 AM, Pekka Paalanen wrote:
> On Mon,  5 Oct 2015 13:12:12 -0500
> Derek Foreman <derekf at osg.samsung.com> wrote:
> 
>> Sometimes the compositor wants to make sure a shm pool doesn't disappear
>> out from under it.
>>
>> For example, in Enlightenment, rendering happens in a separate thread
>> while the main thread can still dispatch events.  If a client is destroyed
>> during rendering, all its resources are cleaned up and its shm pools are
>> unmapped.  This causes the rendering thread to segfault.
>>
>> This patch adds a way for the compositor to increment the refcount of the
>> shm pool so it can't disappear, and decrement it when it's finished.
>>
>> The ref/unref are asymmetrical (ref returns the pool) because it's
>> possible the buffer itself will be gone when you need to unref the pool.
>>
>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
>> ---
>>
>> Changes from v1:
>>  renamed functions to wl_shm_buffer_ref_pool and wl_shm_pool_unref
>>  added doxy
>>
>>  src/wayland-server-core.h |  7 +++++++
>>  src/wayland-shm.c         | 36 ++++++++++++++++++++++++++++++++++++
>>  2 files changed, 43 insertions(+)
>>
>> diff --git a/src/wayland-server-core.h b/src/wayland-server-core.h
>> index e605432..4c2bdfe 100644
>> --- a/src/wayland-server-core.h
>> +++ b/src/wayland-server-core.h
>> @@ -362,6 +362,7 @@ wl_resource_get_destroy_listener(struct wl_resource *resource,
>>  	     resource = tmp,							\
>>  	     tmp = wl_resource_from_link(wl_resource_get_link(resource)->next))
>>  
>> +struct wl_shm_pool;
>>  struct wl_shm_buffer;
>>  
>>  void
>> @@ -388,6 +389,12 @@ wl_shm_buffer_get_width(struct wl_shm_buffer *buffer);
>>  int32_t
>>  wl_shm_buffer_get_height(struct wl_shm_buffer *buffer);
>>  
>> +struct wl_shm_pool *
>> +wl_shm_buffer_ref_pool(struct wl_shm_buffer *buffer);
>> +
>> +void
>> +wl_shm_pool_unref(struct wl_shm_pool *pool);
>> +
>>  int
>>  wl_display_init_shm(struct wl_display *display);
>>  
>> diff --git a/src/wayland-shm.c b/src/wayland-shm.c
>> index 5c419fa..4f9d9d0 100644
>> --- a/src/wayland-shm.c
>> +++ b/src/wayland-shm.c
>> @@ -412,6 +412,42 @@ wl_shm_buffer_get_height(struct wl_shm_buffer *buffer)
>>  	return buffer->height;
>>  }
>>  
>> +/** Get a reference to a shm_buffer's shm_pool
>> + *
>> + * \param buffer The buffer object
>> + *
>> + * Returns a pointer to a buffer's shm_pool and increases the
>> + * shm_pool refcount.
>> + *
>> + * This refcount keeps the shm pool from being destroyed on client
>> + * disconnect, so the compositor must call wl_shm_pool_unref on it
>> + * when finished.
> 
> It's not the disconnect directly, but cleaning up all wl_resources
> after a disconnect causes the side-effect of the pool refcount dropping
> to zero. Not sure that's an important detail to mention here, though.
> The client can also do this without a disconnect, by just destroying
> all relevant wl_proxies.
> 

Yeah, true - the only information I want to convey is that the
programmer must remember to call the other function to decrease the
refcount.  That is, the programmer has interfered with the normal
refcounting.

>> + *
>> + * \memberof wl_shm_buffer
> 
> See also: wl_shm_pool_unref.
> 
>> + */
>> +WL_EXPORT struct wl_shm_pool *
>> +wl_shm_buffer_ref_pool(struct wl_shm_buffer *buffer)
>> +{
>> +	assert(buffer->pool->refcount);
>> +
>> +	buffer->pool->refcount++;
>> +	return buffer->pool;
>> +}
>> +
>> +/** Unreference a shm_pool
>> + *
>> + * \param buffer The pool object
>> + *
>> + * Drops a reference to a wl_shm_pool object.
>> + *
>> + * \memberof wl_shm_buffer
> 
> wl_shm_buffer?
> 
> I think we already have an empty wl_shm_pool section in the docs,
> wouldn't this belong there, with a see also wl_shm_buffer_ref_pool()?

Yes.

> If Giulio or anyone still needs to get the fd of a shm buffer or
> anything, that would be naturally done by taking a ref for the
> wl_shm_pool, and then asking the pool for its fd and size.

That's my thinking too...

I've been griefing him on that fd getter patch because it keeps fds open
for every pool, but I'm wondering if that's a silly complaint.  Seems my
per process limit is 65535 open fds here - it could be easily exhausted
by a rogue client, but in reasonable usage that's probably not going to
be hit.  But that's a concern for another email thread. :)

>> + */
>> +WL_EXPORT void
>> +wl_shm_pool_unref(struct wl_shm_pool *pool)
>> +{
>> +	shm_pool_unref(pool);
>> +}
>> +
>>  static void
>>  reraise_sigbus(void)
>>  {
> 
> Anyway, documentation bikeshedding apart, this patch looks good to me,
> so:
> 
> Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
> 
> I did not test this in any way.

I'll post a v3 with some doc changes in case anyone wants to bike shed
and land it if there are no complaints.

Thanks,
Derek

> Thanks,
> pq
> 
> 
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>