[PATCH wayland] shm: add a note about shm pool base address changing

[Derek Foreman]

On 09/02/16 05:56 AM, Pekka Paalanen wrote:
> On Mon, 08 Feb 2016 12:23:58 -0600
> Derek Foreman <derekf at osg.samsung.com> wrote:
> 
>> On 05/02/16 08:50 AM, Derek Foreman wrote:
>>> On 05/02/16 07:25 AM, Pekka Paalanen wrote:  
>>>> From: Pekka Paalanen <pekka.paalanen at collabora.co.uk>
>>>>
>>>> Since shm_pool_resize() uses mremap(MREMAP_MAYMOVE), the pool's base
>>>> address may change at that point.
>>>>
>>>> If a compositor stores the pointer and a client enlarges the pool, the
>>>> compositor will have a stale pointer.
>>>>
>>>> Signed-off-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>  
>>>
>>> Looks good to me,
>>> Reviewed-by: Derek Foreman <derekf at osg.samsung.com>  
>>
>> Been thinking about this lately, and this *really* screws up
>> Enlightenment's asynchronous renderer - and I guess anyone else that
>> tries to render from a thread while dispatching in another.
>>
>> If we don't have any way to prevent this from happening out from under
>> us, our renderer can be killed intentionally by any client - this is not
>> good.
>>
>> Even the shm pool reference counting functions I added don't give us a
>> way out of this right now, as a resize will still change all the
>> pointers we've gathered before firing off the async rendering thread.
>>
>> Any suggestions on resolving this?
> 
> Hi Derek,
> 
> I can't see any, except "don't do that"...
> 
> Or would it be possible to delay the mremap() call?

I think that would fix things up perfectly.

> I presume you are happy executing any mremap()'s while your rendering
> thread is not busy, and while the rendering thread is busy nothing
> will need data from wl_buffers committed after the rendering was
> launched?

Completely true.  I don't think it would be sane any other way - if it
is then the patches I've just posted need a rethink...

> Would it be possible to use the explicit buffer referencing API you
> added to also count explicitly referenced buffers in a pool and delay
> mremap() until the count drops to zero the next time?

Yes, easily.

> This should probably be accompanied with a patch to make
> wl_shm_buffer_get_data() warn when the count is non-zero and there is a
> pending mremap().

Ah, I haven't done that yet but it's a good idea.  Will follow up with that.

>> I'd be happy changing the pool resize semantics to only allow it when
>> there are no unreleased buffers in the pool.  (or only allow it when the
>> compositor doesn't have a reference - but unreleased buffers would be
>> the only way a client could infer that...)
> 
> How would you know if there are unreleased buffers? Or when a
> compositor has a reference?
> 
> libwayland-server's shm implementation does not know anything about
> them. It only knows about wl_buffers getting created and destroyed.
> 
> Or you mean implement that in compositors? Ah, I assume so.

I'd have inferred it based on whether explicit references were taken or
not.  Irrelevant now, since the deferred mremap approach seems to solve
this all quite nicely.

>> Actually, I wouldn't be surprised if the only user of pool resize right
>> now is libwayland-cursor, and it does all its resizing before it ever
>> submits a buffer - I'd be quite happy to make that the required way to
>> use resize. ;)
> 
> By submit, you mean attach and commit?

Yes - some time after I said that I did a quick google search for other
uses of resize, and it appears there may be some that don't do it that
way. :)

> All that seems quite tricky to implement and would need implementing in
> each compositor. It would also need to be implemented in every
> compositor tentatively and tested in the wild for a period before we
> could assume no-one is using it in any awkward way.

Nod - I assumed only E would ever implement anything like this since I
don't think anyone else is using a separate render thread.

>> We really *really* need to be able to store pointers over dispatch to do
>> async rendering, so while this has never actually been possible, it has
>> at least not been documented as impossible yet.
> 
> I suppose you don't like the suggestion "make your own implementation
> of wl_shm instead of using libwayland-server's". :-P

Haha, not my favourite suggestion, but certainly a possibility. :)

Really not at all hard, so if you think I'm making things too ugly by
trying to make it work for Enlightenment, I can do that.

>> I think I need to retract this too-speedy Reviewed-by... :/
> 
> Darn.
> 
> A yak here, and a yak there...

Thanks for helping shave this one. :D

> 
> Thanks,
> pq
> 
> 
>>>> ---
>>>>  src/wayland-shm.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/src/wayland-shm.c b/src/wayland-shm.c
>>>> index a4343a4..7e42dcb 100644
>>>> --- a/src/wayland-shm.c
>>>> +++ b/src/wayland-shm.c
>>>> @@ -348,6 +348,10 @@ wl_shm_buffer_get_stride(struct wl_shm_buffer *buffer)
>>>>   * to crash you should call wl_shm_buffer_begin_access and
>>>>   * wl_shm_buffer_end_access around code that reads from the memory.
>>>>   *
>>>> + * @note The return value of this function must not be stored across
>>>> + * dispatching client requests. If a client resizes the underlying shm pool,
>>>> + * the resize request handler will remap, and the pool base address may change.
>>>> + *
>>>>   * \memberof wl_shm_buffer
>>>>   */
>>>>  WL_EXPORT void *
>>>>  
>>>>
>>>>
>>>> _______________________________________________
>>>> wayland-devel mailing list
>>>> wayland-devel at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel