[PATCH] xwayland: Close the shm fd as early as possible

[Pekka Paalanen]

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.

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.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <http://lists.x.org/archives/xorg-devel/attachments/20160205/55a3d648/attachment.sig>