On 09/02/16 05:56 AM, Pekka Paalanen wrote: > On Mon, 08 Feb 2016 12:23:58 -0600 > Derek Foreman <[email protected]> wrote: > >> On 05/02/16 08:50 AM, Derek Foreman wrote: >>> On 05/02/16 07:25 AM, Pekka Paalanen wrote: >>>> From: Pekka Paalanen <[email protected]> >>>> >>>> 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 <[email protected]> >>> >>> Looks good to me, >>> Reviewed-by: Derek Foreman <[email protected]> >> >> 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 >>>> [email protected] >>>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
