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

[Pekka Paalanen]

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 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?

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?

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().

> 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.

> 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?

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.

> 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

> I think I need to retract this too-speedy Reviewed-by... :/

Darn.

A yak here, and a yak there...


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 *
> >>  
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160209/d4c4ad6a/attachment.sig>