| Message ID | 1388419013-17016-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 30, 2013 at 01:56:48PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > When we call kmstest_get_cairo_ctx() and create a context, we do a > gem_mmap. The problem is that we lose the mmap pointer, so we leak it. > This patch stores the pointer and frees it at kmstest_remove_fb. > > Huge test suites like kms_flip do this operation thousands of times, > which makes the virtual memory size increase until the test suite gets > killed. Today, without this patch, we can't even run 50% of the > kms_flip tests due to this problem. To test this, just "./kms_flip", > then run "top" and sort it by the VIRT column. Bleh. kmstest is a bad example of how to handle cairo object lifetimes and this patch just makes it worse. -Chris
2013/12/30 Chris Wilson <chris@chris-wilson.co.uk>: > On Mon, Dec 30, 2013 at 01:56:48PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> When we call kmstest_get_cairo_ctx() and create a context, we do a >> gem_mmap. The problem is that we lose the mmap pointer, so we leak it. >> This patch stores the pointer and frees it at kmstest_remove_fb. >> >> Huge test suites like kms_flip do this operation thousands of times, >> which makes the virtual memory size increase until the test suite gets >> killed. Today, without this patch, we can't even run 50% of the >> kms_flip tests due to this problem. To test this, just "./kms_flip", >> then run "top" and sort it by the VIRT column. > > Bleh. kmstest is a bad example of how to handle cairo object lifetimes > and this patch just makes it worse. I definitely can't say I love the current code, but we need a fix somehow. Do you have any specific suggestions that I could try? > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Mon, Dec 30, 2013 at 03:53:10PM -0200, Paulo Zanoni wrote: > 2013/12/30 Chris Wilson <chris@chris-wilson.co.uk>: > > On Mon, Dec 30, 2013 at 01:56:48PM -0200, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> When we call kmstest_get_cairo_ctx() and create a context, we do a > >> gem_mmap. The problem is that we lose the mmap pointer, so we leak it. > >> This patch stores the pointer and frees it at kmstest_remove_fb. > >> > >> Huge test suites like kms_flip do this operation thousands of times, > >> which makes the virtual memory size increase until the test suite gets > >> killed. Today, without this patch, we can't even run 50% of the > >> kms_flip tests due to this problem. To test this, just "./kms_flip", > >> then run "top" and sort it by the VIRT column. > > > > Bleh. kmstest is a bad example of how to handle cairo object lifetimes > > and this patch just makes it worse. > > I definitely can't say I love the current code, but we need a fix > somehow. Do you have any specific suggestions that I could try? I have something I can push in the near future. -Chris
On Mon, Dec 30, 2013 at 06:16:20PM +0000, Chris Wilson wrote: > On Mon, Dec 30, 2013 at 03:53:10PM -0200, Paulo Zanoni wrote: > > 2013/12/30 Chris Wilson <chris@chris-wilson.co.uk>: > > > On Mon, Dec 30, 2013 at 01:56:48PM -0200, Paulo Zanoni wrote: > > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > >> > > >> When we call kmstest_get_cairo_ctx() and create a context, we do a > > >> gem_mmap. The problem is that we lose the mmap pointer, so we leak it. > > >> This patch stores the pointer and frees it at kmstest_remove_fb. > > >> > > >> Huge test suites like kms_flip do this operation thousands of times, > > >> which makes the virtual memory size increase until the test suite gets > > >> killed. Today, without this patch, we can't even run 50% of the > > >> kms_flip tests due to this problem. To test this, just "./kms_flip", > > >> then run "top" and sort it by the VIRT column. > > > > > > Bleh. kmstest is a bad example of how to handle cairo object lifetimes > > > and this patch just makes it worse. > > > > I definitely can't say I love the current code, but we need a fix > > somehow. Do you have any specific suggestions that I could try? > > I have something I can push in the near future. I've taken care of this GEM bo leak, thanks. -Chris
2013/12/31 Chris Wilson <chris@chris-wilson.co.uk>: > On Mon, Dec 30, 2013 at 06:16:20PM +0000, Chris Wilson wrote: >> On Mon, Dec 30, 2013 at 03:53:10PM -0200, Paulo Zanoni wrote: >> > 2013/12/30 Chris Wilson <chris@chris-wilson.co.uk>: >> > > On Mon, Dec 30, 2013 at 01:56:48PM -0200, Paulo Zanoni wrote: >> > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > >> >> > >> When we call kmstest_get_cairo_ctx() and create a context, we do a >> > >> gem_mmap. The problem is that we lose the mmap pointer, so we leak it. >> > >> This patch stores the pointer and frees it at kmstest_remove_fb. >> > >> >> > >> Huge test suites like kms_flip do this operation thousands of times, >> > >> which makes the virtual memory size increase until the test suite gets >> > >> killed. Today, without this patch, we can't even run 50% of the >> > >> kms_flip tests due to this problem. To test this, just "./kms_flip", >> > >> then run "top" and sort it by the VIRT column. >> > > >> > > Bleh. kmstest is a bad example of how to handle cairo object lifetimes >> > > and this patch just makes it worse. >> > >> > I definitely can't say I love the current code, but we need a fix >> > somehow. Do you have any specific suggestions that I could try? >> >> I have something I can push in the near future. > > I've taken care of this GEM bo leak, thanks. Yay \o/ I confirm your patch fixes the leak. Thanks!! > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 57795b1..f032cd3 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -303,6 +303,7 @@ unsigned int kmstest_create_fb(int fd, int width, int height, int bpp, fb->height = height; fb->tiling = tiled; fb->drm_format = bpp_depth_to_drm_format(bpp, depth); + fb->fb_ptr = NULL; return fb->fb_id; } @@ -353,6 +354,7 @@ unsigned int kmstest_create_fb2(int fd, int width, int height, uint32_t format, fb->tiling = tiled; fb->drm_format = format; fb->fb_id = fb_id; + fb->fb_ptr = NULL; return fb_id; } @@ -372,13 +374,14 @@ static cairo_surface_t *create_image_surface(int fd, struct kmstest_fb *fb) { cairo_surface_t *surface; cairo_format_t cformat; - void *fb_ptr; cformat = drm_format_to_cairo(fb->drm_format); - fb_ptr = gem_mmap(fd, fb->gem_handle, fb->size, PROT_READ | PROT_WRITE); - surface = cairo_image_surface_create_for_data((unsigned char *)fb_ptr, - cformat, fb->width, - fb->height, fb->stride); + fb->fb_ptr = gem_mmap(fd, fb->gem_handle, fb->size, + PROT_READ | PROT_WRITE); + surface = cairo_image_surface_create_for_data( + (unsigned char *)fb->fb_ptr, + cformat, fb->width, + fb->height, fb->stride); assert(surface); return surface; @@ -423,6 +426,8 @@ void kmstest_remove_fb(int fd, struct kmstest_fb *fb) { if (fb->cairo_ctx) cairo_destroy(fb->cairo_ctx); + if (fb->fb_ptr) + munmap(fb->fb_ptr, fb->size); do_or_die(drmModeRmFB(fd, fb->fb_id)); gem_close(fd, fb->gem_handle); } diff --git a/lib/igt_kms.h b/lib/igt_kms.h index f61f8e5..f9494d0 100644 --- a/lib/igt_kms.h +++ b/lib/igt_kms.h @@ -55,6 +55,7 @@ struct kmstest_fb { unsigned tiling; unsigned size; cairo_t *cairo_ctx; + void *fb_ptr; }; enum kmstest_text_align {