----- Original Message ----- > Hi all, > > ----- Original Message ----- > > On Fri, 5 Feb 2016 14:44:29 +0100 > > Rui Matos <tiagoma...@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 <ppaala...@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 <ofour...@redhat.com> > > Cheers, > Olivier > _______________________________________________ > xorg-devel@lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: https://lists.x.org/mailman/listinfo/xorg-devel _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel