[PATCH] xwayland: Close the shm fd as early as possible
Olivier Fourdan
ofourdan at redhat.com
Tue Sep 6 16:06:47 UTC 2016
----- Original Message -----
> Hi all,
>
> ----- Original Message -----
> > On Fri, 5 Feb 2016 14:44:29 +0100
> > Rui Matos <tiagomatos at gmail.com> wrote:
> >
> > > Keeping the shm fd open beyond pixmap creation means we can easily
> > > reach the open file descriptor limit if an X client asks us to create
> > > that many pixmaps. Instead, let's get the wl_buffer immediatly so that
> > > we can destroy the shm pool and close the fd before being asked to
> > > create more.
> > > ---
> > >
> > > On Fri, Feb 5, 2016 at 1:53 PM, Pekka Paalanen <ppaalanen at gmail.com>
> > > wrote:
> > > > You don't need the fd or the pool to write into the buffer, or attach
> > > > and commit the wl_buffer, or...
> > >
> > > You are right, of course. My initial attempt at this was crashing and
> > > I quickly dismissed this approach without investigating why it was
> > > crashing more closely. And sure enough, it was just a thinko on my
> > > part.
> > >
> > > Thanks!
> > >
> > > Rui
> > >
> > > hw/xwayland/xwayland-shm.c | 46
> > > +++++++++++++++++++---------------------------
> > > 1 file changed, 19 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/hw/xwayland/xwayland-shm.c b/hw/xwayland/xwayland-shm.c
> > > index 7072be4..25ee03c 100644
> > > --- a/hw/xwayland/xwayland-shm.c
> > > +++ b/hw/xwayland/xwayland-shm.c
> > > @@ -36,7 +36,6 @@
> > >
> > > struct xwl_pixmap {
> > > struct wl_buffer *buffer;
> > > - int fd;
> > > void *data;
> > > size_t size;
> > > };
> > > @@ -174,9 +173,13 @@ PixmapPtr
> > > xwl_shm_create_pixmap(ScreenPtr screen,
> > > int width, int height, int depth, unsigned int
> > > hint)
> > > {
> > > - PixmapPtr pixmap;
> > > + struct xwl_screen *xwl_screen = xwl_screen_get(screen);
> > > struct xwl_pixmap *xwl_pixmap;
> > > + struct wl_shm_pool *pool;
> > > + PixmapPtr pixmap;
> > > size_t size, stride;
> > > + uint32_t format;
> > > + int fd;
> > >
> > > if (hint == CREATE_PIXMAP_USAGE_GLYPH_PICTURE ||
> > > (width == 0 && height == 0) || depth < 15)
> > > @@ -194,12 +197,12 @@ xwl_shm_create_pixmap(ScreenPtr screen,
> > > size = stride * height;
> > > xwl_pixmap->buffer = NULL;
> > > xwl_pixmap->size = size;
> > > - xwl_pixmap->fd = os_create_anonymous_file(size);
> > > - if (xwl_pixmap->fd < 0)
> > > + fd = os_create_anonymous_file(size);
> > > + if (fd < 0)
> > > goto err_free_xwl_pixmap;
> > >
> > > xwl_pixmap->data = mmap(NULL, size, PROT_READ | PROT_WRITE,
> > > - MAP_SHARED, xwl_pixmap->fd, 0);
> > > + MAP_SHARED, fd, 0);
> > > if (xwl_pixmap->data == MAP_FAILED)
> > > goto err_close_fd;
> > >
> > > @@ -208,6 +211,15 @@ xwl_shm_create_pixmap(ScreenPtr screen,
> > > stride, xwl_pixmap->data))
> > > goto err_munmap;
> > >
> > > + format = shm_format_for_depth(pixmap->drawable.depth);
> > > + pool = wl_shm_create_pool(xwl_screen->shm, fd, xwl_pixmap->size);
> > > + xwl_pixmap->buffer = wl_shm_pool_create_buffer(pool, 0,
> > > +
> > > pixmap->drawable.width,
> > > +
> > > pixmap->drawable.height,
> > > + pixmap->devKind,
> > > format);
> > > + wl_shm_pool_destroy(pool);
> > > + close(fd);
> > > +
> > > xwl_pixmap_set_private(pixmap, xwl_pixmap);
> > >
> > > return pixmap;
> > > @@ -215,7 +227,7 @@ xwl_shm_create_pixmap(ScreenPtr screen,
> > > err_munmap:
> > > munmap(xwl_pixmap->data, size);
> > > err_close_fd:
> > > - close(xwl_pixmap->fd);
> > > + close(fd);
> > > err_free_xwl_pixmap:
> > > free(xwl_pixmap);
> > > err_destroy_pixmap:
> > > @@ -233,7 +245,6 @@ xwl_shm_destroy_pixmap(PixmapPtr pixmap)
> > > if (xwl_pixmap->buffer)
> > > wl_buffer_destroy(xwl_pixmap->buffer);
> > > munmap(xwl_pixmap->data, xwl_pixmap->size);
> > > - close(xwl_pixmap->fd);
> > > free(xwl_pixmap);
> > > }
> > >
> > > @@ -243,26 +254,7 @@ xwl_shm_destroy_pixmap(PixmapPtr pixmap)
> > > struct wl_buffer *
> > > xwl_shm_pixmap_get_wl_buffer(PixmapPtr pixmap)
> > > {
> > > - struct xwl_screen *xwl_screen =
> > > xwl_screen_get(pixmap->drawable.pScreen);
> > > - struct xwl_pixmap *xwl_pixmap = xwl_pixmap_get(pixmap);
> > > - struct wl_shm_pool *pool;
> > > - uint32_t format;
> > > -
> > > - if (xwl_pixmap->buffer)
> > > - return xwl_pixmap->buffer;
> > > -
> > > - pool = wl_shm_create_pool(xwl_screen->shm,
> > > - xwl_pixmap->fd, xwl_pixmap->size);
> > > -
> > > - format = shm_format_for_depth(pixmap->drawable.depth);
> > > - xwl_pixmap->buffer = wl_shm_pool_create_buffer(pool, 0,
> > > -
> > > pixmap->drawable.width,
> > > -
> > > pixmap->drawable.height,
> > > - pixmap->devKind,
> > > format);
> > > -
> > > - wl_shm_pool_destroy(pool);
> > > -
> > > - return xwl_pixmap->buffer;
> > > + return xwl_pixmap_get(pixmap)->buffer;
> > > }
> > >
> > > Bool
> >
> > Hi Rui,
> >
> > the idea here looks fine to me.
> >
> > However, I've been wondering, surely there was a reason why it wasn't
> > coded like this in the first place?
> >
> > Could you check if this patch causes Xwayland to create lots of
> > wl_buffers it will never attach and commit to a wl_surface? If it does,
> > that was probably the reason.
>
> Was that evaluated?
>
> AFAICS, when using glamor, the only code path that still uses the
> xwl_shm_*_pixmap() is the cursor code, so even with glamor, running several
> X11 apps which create several cursors each will quickly cause Xwayland to
> reach the limit of file descriptors.
>
> > If that is true, then one has to weigh between creating wl_buffers
> > immediately vs. creating them only when actually needed for a
> > wl_surface.
> >
> > Maybe Daniel knows? CC'ing also Olivier for his information.
>
> I cannot tell why this was done the way it is, but I think this patch does
> improve things for e.g. downstream bug
> https://bugzilla.redhat.com/show_bug.cgi?id=1338979
Blimey! copy/paste failed, that should have read https://bugzilla.redhat.com/show_bug.cgi?id=1373451
> so:
>
> Tested-by: Olivier Fourdan <ofourdan at redhat.com>
>
> Cheers,
> Olivier
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: https://lists.x.org/mailman/listinfo/xorg-devel
More information about the xorg-devel
mailing list