[patch] slob: fix gfp flags for order-0 page allocations

[patch] slob: fix gfp flags for order-0 page allocations

[David Rientjes]

kmalloc_node() may allocate higher order slob pages, but the __GFP_COMP
bit is only passed to the page allocator and not represented in the
tracepoint event.  The bit should be passed to trace_kmalloc_node() as
well.

Signed-off-by: David Rientjes <rientjes@google.com>
---
 mm/slob.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/mm/slob.c b/mm/slob.c
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -500,7 +500,9 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
 	} else {
 		unsigned int order = get_order(size);
 
-		ret = slob_new_pages(gfp | __GFP_COMP, get_order(size), node);
+		if (likely(order))
+			gfp |= __GFP_COMP;
+		ret = slob_new_pages(gfp, order, node);
 		if (ret) {
 			struct page *page;
 			page = virt_to_page(ret);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] slob: fix gfp flags for order-0 page allocations

[Christoph Lameter]

> diff --git a/mm/slob.c b/mm/slob.c
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -500,7 +500,9 @@ void *__kmalloc_node(size_t size, gfp_t gfp, int node)
>  	} else {
>  		unsigned int order = get_order(size);
>
> -		ret = slob_new_pages(gfp | __GFP_COMP, get_order(size), node);
> +		if (likely(order))
> +			gfp |= __GFP_COMP;
> +		ret = slob_new_pages(gfp, order, node);
>  		if (ret) {
>  			struct page *page;

Also gets rid of the double get_order().

Reviewed-by: Christoph Lameter <cl@linux.com>


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] slob: fix gfp flags for order-0 page allocations

[Matt Mackall]

On Sun, 2010-08-22 at 16:16 -0700, David Rientjes wrote:
> kmalloc_node() may allocate higher order slob pages, but the __GFP_COMP
> bit is only passed to the page allocator and not represented in the
> tracepoint event.  The bit should be passed to trace_kmalloc_node() as
> well.
> 
> Signed-off-by: David Rientjes <rientjes@google.com>

>  		unsigned int order = get_order(size);
>  
> -		ret = slob_new_pages(gfp | __GFP_COMP, get_order(size), node);
> +		if (likely(order))
> +			gfp |= __GFP_COMP;

Why is it likely? I would hope that the majority of page allocations are
in fact order 0.

-- 
Mathematics is the supreme nostalgia of our time.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] slob: fix gfp flags for order-0 page allocations

[David Rientjes]

On Mon, 23 Aug 2010, Matt Mackall wrote:

> > kmalloc_node() may allocate higher order slob pages, but the __GFP_COMP
> > bit is only passed to the page allocator and not represented in the
> > tracepoint event.  The bit should be passed to trace_kmalloc_node() as
> > well.
> > 
> > Signed-off-by: David Rientjes <rientjes@google.com>
> 
> >  		unsigned int order = get_order(size);
> >  
> > -		ret = slob_new_pages(gfp | __GFP_COMP, get_order(size), node);
> > +		if (likely(order))
> > +			gfp |= __GFP_COMP;
> 
> Why is it likely? I would hope that the majority of page allocations are
> in fact order 0.
> 

This code only executes when size >= PAGE_SIZE + align, so I would assume 
that the vast majority of times this is actually higher order allocs 
(which is probably why __GFP_COMP was implicitly added to the gfpmask in 
the first place).  Is there evidence to show otherwise?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] slob: fix gfp flags for order-0 page allocations

[Matt Mackall]

On Mon, 2010-08-23 at 21:36 -0700, David Rientjes wrote:
> On Mon, 23 Aug 2010, Matt Mackall wrote:
> 
> > > kmalloc_node() may allocate higher order slob pages, but the __GFP_COMP
> > > bit is only passed to the page allocator and not represented in the
> > > tracepoint event.  The bit should be passed to trace_kmalloc_node() as
> > > well.
> > > 
> > > Signed-off-by: David Rientjes <rientjes@google.com>
> > 
> > >  		unsigned int order = get_order(size);
> > >  
> > > -		ret = slob_new_pages(gfp | __GFP_COMP, get_order(size), node);
> > > +		if (likely(order))
> > > +			gfp |= __GFP_COMP;
> > 
> > Why is it likely? I would hope that the majority of page allocations are
> > in fact order 0.
> > 
> 
> This code only executes when size >= PAGE_SIZE + align, so I would assume 
> that the vast majority of times this is actually higher order allocs 
> (which is probably why __GFP_COMP was implicitly added to the gfpmask in 
> the first place).  Is there evidence to show otherwise?

(peeks at code)

Ok, that + should be a -. But yes, you're right, the bucket around an
order-0 allocation is quite small.

Acked-by: Matt Mackall <mpm@selenic.com>


By the way, has anyone seen anything like this leak reported?

/proc/slabinfo:

kmalloc-32        1113344 1113344     32  128    1 : tunables    0    0
0 : slabdata   8698   8698      0

That's /proc/slabinfo on my laptop with SLUB. It looks like my last
reboot popped me back to 2.6.33 so it may also be old news, but I
couldn't spot any reports with Google.

-- 
Mathematics is the supreme nostalgia of our time.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] slob: fix gfp flags for order-0 page allocations

[Christoph Lameter]

On Tue, 24 Aug 2010, Matt Mackall wrote:

> kmalloc-32        1113344 1113344     32  128    1 : tunables    0    0
> 0 : slabdata   8698   8698      0
>
> That's /proc/slabinfo on my laptop with SLUB. It looks like my last
> reboot popped me back to 2.6.33 so it may also be old news, but I
> couldn't spot any reports with Google.

Boot with "slub_debug" as a kernel parameter

and then do a

cat /sys/kernel/slab/kmalloc-32/alloc_calls

to find the caller allocating the objets.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] slob: fix gfp flags for order-0 page allocations

[Pekka Enberg]

On Tue, 24 Aug 2010, Matt Mackall wrote:
> (peeks at code)
>
> Ok, that + should be a -. But yes, you're right, the bucket around an
> order-0 allocation is quite small.
>
> Acked-by: Matt Mackall <mpm@selenic.com>

Applied, thanks!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch] slob: fix gfp flags for order-0 page allocations

[David Rientjes]

On Tue, 24 Aug 2010, Christoph Lameter wrote:

> > kmalloc-32        1113344 1113344     32  128    1 : tunables    0    0
> > 0 : slabdata   8698   8698      0
> >
> > That's /proc/slabinfo on my laptop with SLUB. It looks like my last
> > reboot popped me back to 2.6.33 so it may also be old news, but I
> > couldn't spot any reports with Google.
> 
> Boot with "slub_debug" as a kernel parameter
> 
> and then do a
> 
> cat /sys/kernel/slab/kmalloc-32/alloc_calls
> 
> to find the caller allocating the objets.
> 

I'd suspect this was anon_vma, and enabling CONFIG_DEBUG_KMEMLEAK would 
probably reveal exactly where it's getting leaked.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

DRM-related kmalloc-32 memory leak in 2.6.35

[Matt Mackall]

On Tue, 2010-08-24 at 10:37 -0500, Christoph Lameter wrote:
> On Tue, 24 Aug 2010, Matt Mackall wrote:
> 
> > kmalloc-32        1113344 1113344     32  128    1 : tunables    0    0
> > 0 : slabdata   8698   8698      0
> >
> > That's /proc/slabinfo on my laptop with SLUB. It looks like my last
> > reboot popped me back to 2.6.33 so it may also be old news, but I
> > couldn't spot any reports with Google.
> 
> Boot with "slub_debug" as a kernel parameter
> 
> and then do a
> 
> cat /sys/kernel/slab/kmalloc-32/alloc_calls
> 
> to find the caller allocating the objets.

Still present in 2.6.35. Appears to be DRM:

    845 drm_vm_open_locked+0x72/0x109 age=43/37572/59269 pid=2089
cpus=0-1

That's after about a minute of uptime. Grows to 100k in about a day.

dmesg bits:
[    0.834653] [drm] Initialized drm 1.1.0 20060810
[    0.834986] pci 0000:00:02.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
[    0.834995] pci 0000:00:02.0: setting latency timer to 64
[    1.002572] mtrr: type mismatch for e0000000,10000000 old: write-back new: write-combining
[    1.002580] [drm] MTRR allocation failed.  Graphics performance may suffer.
[    1.019880] acpi device:03: registered as cooling_device2
[    1.021520] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input3
[    1.021543] ACPI: Video Device [GFX0] (multi-head: yes  rom: no  post: no)
[    1.021855] [drm] Initialized i915 1.6.0 20080730 for 0000:00:02.0 on minor 0

This is with:

00:02.0 VGA compatible controller: Intel Corporation Mobile GM965/GL960
Integrated Graphics Controller (rev 03)
00:02.1 Display controller: Intel Corporation Mobile GM965/GL960
Integrated Graphics Controller (rev 03)

-- 
Mathematics is the supreme nostalgia of our time.

Re: DRM-related kmalloc-32 memory leak in 2.6.35

[Andrew Morton]

On Wed, 25 Aug 2010 15:59:09 -0500
Matt Mackall <mpm@selenic.com> wrote:

> On Tue, 2010-08-24 at 10:37 -0500, Christoph Lameter wrote:
> > On Tue, 24 Aug 2010, Matt Mackall wrote:
> > 
> > > kmalloc-32        1113344 1113344     32  128    1 : tunables    0    0
> > > 0 : slabdata   8698   8698      0
> > >
> > > That's /proc/slabinfo on my laptop with SLUB. It looks like my last
> > > reboot popped me back to 2.6.33 so it may also be old news, but I
> > > couldn't spot any reports with Google.
> > 
> > Boot with "slub_debug" as a kernel parameter
> > 
> > and then do a
> > 
> > cat /sys/kernel/slab/kmalloc-32/alloc_calls
> > 
> > to find the caller allocating the objets.
> 
> Still present in 2.6.35. Appears to be DRM:
> 
>     845 drm_vm_open_locked+0x72/0x109 age=43/37572/59269 pid=2089
> cpus=0-1
> 
> That's after about a minute of uptime. Grows to 100k in about a day.
> 
> dmesg bits:
> [    0.834653] [drm] Initialized drm 1.1.0 20060810
> [    0.834986] pci 0000:00:02.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
> [    0.834995] pci 0000:00:02.0: setting latency timer to 64
> [    1.002572] mtrr: type mismatch for e0000000,10000000 old: write-back new: write-combining
> [    1.002580] [drm] MTRR allocation failed.  Graphics performance may suffer.
> [    1.019880] acpi device:03: registered as cooling_device2
> [    1.021520] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input3
> [    1.021543] ACPI: Video Device [GFX0] (multi-head: yes  rom: no  post: no)
> [    1.021855] [drm] Initialized i915 1.6.0 20080730 for 0000:00:02.0 on minor 0
> 
> This is with:
> 
> 00:02.0 VGA compatible controller: Intel Corporation Mobile GM965/GL960
> Integrated Graphics Controller (rev 03)
> 00:02.1 Display controller: Intel Corporation Mobile GM965/GL960
> Integrated Graphics Controller (rev 03)
> 

Matt tells me that this (drop-dead box-killing!) bug is still present
in current kernels.  Could someone take a look please?

The code seems very simple, and I couldn't spot a problem.  Probably
this means that I'm even simpler.

[PATCH] drm: Prune GEM vma entries

[Chris Wilson]

Hook the GEM vm open/close ops into the generic drm vm open/close so
that the vma entries are created and destroy appropriately.

Reported-by: Matt Mackall <mpm@selenic.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/drm_gem.c |    9 ++++++++-
 drivers/gpu/drm/drm_vm.c  |   28 ++++++++++++++++++----------
 include/drm/drmP.h        |    1 +
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index bf92d07..6fe2cd2 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -528,6 +528,10 @@ void drm_gem_vm_open(struct vm_area_struct *vma)
 	struct drm_gem_object *obj = vma->vm_private_data;
 
 	drm_gem_object_reference(obj);
+
+	mutex_lock(&obj->dev->struct_mutex);
+	drm_vm_open_locked(vma);
+	mutex_unlock(&obj->dev->struct_mutex);
 }
 EXPORT_SYMBOL(drm_gem_vm_open);
 
@@ -535,7 +539,10 @@ void drm_gem_vm_close(struct vm_area_struct *vma)
 {
 	struct drm_gem_object *obj = vma->vm_private_data;
 
-	drm_gem_object_unreference_unlocked(obj);
+	mutex_lock(&obj->dev->struct_mutex);
+	drm_vm_close_locked(vma);
+	drm_gem_object_unreference(obj);
+	mutex_unlock(&obj->dev->struct_mutex);
 }
 EXPORT_SYMBOL(drm_gem_vm_close);
 
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index fda6746..5df4506 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -433,15 +433,7 @@ static void drm_vm_open(struct vm_area_struct *vma)
 	mutex_unlock(&dev->struct_mutex);
 }
 
-/**
- * \c close method for all virtual memory types.
- *
- * \param vma virtual memory area.
- *
- * Search the \p vma private data entry in drm_device::vmalist, unlink it, and
- * free it.
- */
-static void drm_vm_close(struct vm_area_struct *vma)
+void drm_vm_close_locked(struct vm_area_struct *vma)
 {
 	struct drm_file *priv = vma->vm_file->private_data;
 	struct drm_device *dev = priv->minor->dev;
@@ -451,7 +443,6 @@ static void drm_vm_close(struct vm_area_struct *vma)
 		  vma->vm_start, vma->vm_end - vma->vm_start);
 	atomic_dec(&dev->vma_count);
 
-	mutex_lock(&dev->struct_mutex);
 	list_for_each_entry_safe(pt, temp, &dev->vmalist, head) {
 		if (pt->vma == vma) {
 			list_del(&pt->head);
@@ -459,6 +450,23 @@ static void drm_vm_close(struct vm_area_struct *vma)
 			break;
 		}
 	}
+}
+
+/**
+ * \c close method for all virtual memory types.
+ *
+ * \param vma virtual memory area.
+ *
+ * Search the \p vma private data entry in drm_device::vmalist, unlink it, and
+ * free it.
+ */
+static void drm_vm_close(struct vm_area_struct *vma)
+{
+	struct drm_file *priv = vma->vm_file->private_data;
+	struct drm_device *dev = priv->minor->dev;
+
+	mutex_lock(&dev->struct_mutex);
+	drm_vm_close_locked(vma);
 	mutex_unlock(&dev->struct_mutex);
 }
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 7809d23..774e1d4 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1175,6 +1175,7 @@ extern int drm_release(struct inode *inode, struct file *filp);
 extern int drm_mmap(struct file *filp, struct vm_area_struct *vma);
 extern int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma);
 extern void drm_vm_open_locked(struct vm_area_struct *vma);
+extern void drm_vm_close_locked(struct vm_area_struct *vma);
 extern resource_size_t drm_core_get_map_ofs(struct drm_local_map * map);
 extern resource_size_t drm_core_get_reg_ofs(struct drm_device *dev);
 extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);
-- 
1.7.1

Re: [PATCH] drm: Prune GEM vma entries

[Andrew Morton]

That was quick, thanks.

On Mon, 27 Sep 2010 21:08:36 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Hook the GEM vm open/close ops into the generic drm vm open/close so
> that the vma entries are created and destroy appropriately.

Please update the changelog to indicate that it fixes a memory leak.

> Reported-by: Matt Mackall <mpm@selenic.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

And here please add

Cc: <stable@kernel.org>

so that earlier kernels get reliably fixed.

Thanks.

Re: [PATCH] drm: Prune GEM vma entries

[Andrew Morton]

That was quick, thanks.

On Mon, 27 Sep 2010 21:08:36 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Hook the GEM vm open/close ops into the generic drm vm open/close so
> that the vma entries are created and destroy appropriately.

Please update the changelog to indicate that it fixes a memory leak.

> Reported-by: Matt Mackall <mpm@selenic.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

And here please add

Cc: <stable@kernel.org>

so that earlier kernels get reliably fixed.

Thanks.

[PATCH] drm: Prune GEM vma entries

[Chris Wilson]

Hook the GEM vm open/close ops into the generic drm vm open/close so
that the private vma entries are created and destroy appropriately.
Fixes the leak of the drm_vma_entries during the lifetime of the filp.

Reported-by: Matt Mackall <mpm@selenic.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Acked-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: stable@kernel.org
---
 drivers/gpu/drm/drm_gem.c |    9 ++++++++-
 drivers/gpu/drm/drm_vm.c  |   28 ++++++++++++++++++----------
 include/drm/drmP.h        |    1 +
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index bf92d07..6fe2cd2 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -528,6 +528,10 @@ void drm_gem_vm_open(struct vm_area_struct *vma)
 	struct drm_gem_object *obj = vma->vm_private_data;
 
 	drm_gem_object_reference(obj);
+
+	mutex_lock(&obj->dev->struct_mutex);
+	drm_vm_open_locked(vma);
+	mutex_unlock(&obj->dev->struct_mutex);
 }
 EXPORT_SYMBOL(drm_gem_vm_open);
 
@@ -535,7 +539,10 @@ void drm_gem_vm_close(struct vm_area_struct *vma)
 {
 	struct drm_gem_object *obj = vma->vm_private_data;
 
-	drm_gem_object_unreference_unlocked(obj);
+	mutex_lock(&obj->dev->struct_mutex);
+	drm_vm_close_locked(vma);
+	drm_gem_object_unreference(obj);
+	mutex_unlock(&obj->dev->struct_mutex);
 }
 EXPORT_SYMBOL(drm_gem_vm_close);
 
diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
index fda6746..5df4506 100644
--- a/drivers/gpu/drm/drm_vm.c
+++ b/drivers/gpu/drm/drm_vm.c
@@ -433,15 +433,7 @@ static void drm_vm_open(struct vm_area_struct *vma)
 	mutex_unlock(&dev->struct_mutex);
 }
 
-/**
- * \c close method for all virtual memory types.
- *
- * \param vma virtual memory area.
- *
- * Search the \p vma private data entry in drm_device::vmalist, unlink it, and
- * free it.
- */
-static void drm_vm_close(struct vm_area_struct *vma)
+void drm_vm_close_locked(struct vm_area_struct *vma)
 {
 	struct drm_file *priv = vma->vm_file->private_data;
 	struct drm_device *dev = priv->minor->dev;
@@ -451,7 +443,6 @@ static void drm_vm_close(struct vm_area_struct *vma)
 		  vma->vm_start, vma->vm_end - vma->vm_start);
 	atomic_dec(&dev->vma_count);
 
-	mutex_lock(&dev->struct_mutex);
 	list_for_each_entry_safe(pt, temp, &dev->vmalist, head) {
 		if (pt->vma == vma) {
 			list_del(&pt->head);
@@ -459,6 +450,23 @@ static void drm_vm_close(struct vm_area_struct *vma)
 			break;
 		}
 	}
+}
+
+/**
+ * \c close method for all virtual memory types.
+ *
+ * \param vma virtual memory area.
+ *
+ * Search the \p vma private data entry in drm_device::vmalist, unlink it, and
+ * free it.
+ */
+static void drm_vm_close(struct vm_area_struct *vma)
+{
+	struct drm_file *priv = vma->vm_file->private_data;
+	struct drm_device *dev = priv->minor->dev;
+
+	mutex_lock(&dev->struct_mutex);
+	drm_vm_close_locked(vma);
 	mutex_unlock(&dev->struct_mutex);
 }
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 7809d23..774e1d4 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1175,6 +1175,7 @@ extern int drm_release(struct inode *inode, struct file *filp);
 extern int drm_mmap(struct file *filp, struct vm_area_struct *vma);
 extern int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma);
 extern void drm_vm_open_locked(struct vm_area_struct *vma);
+extern void drm_vm_close_locked(struct vm_area_struct *vma);
 extern resource_size_t drm_core_get_map_ofs(struct drm_local_map * map);
 extern resource_size_t drm_core_get_reg_ofs(struct drm_device *dev);
 extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);
-- 
1.7.1

Re: DRM-related kmalloc-32 memory leak in 2.6.35

[David Rientjes]

On Mon, 27 Sep 2010, Andrew Morton wrote:

> > Still present in 2.6.35. Appears to be DRM:
> > 
> >     845 drm_vm_open_locked+0x72/0x109 age=43/37572/59269 pid=2089
> > cpus=0-1
> > 
> > That's after about a minute of uptime. Grows to 100k in about a day.
> > 
> > dmesg bits:
> > [    0.834653] [drm] Initialized drm 1.1.0 20060810
> > [    0.834986] pci 0000:00:02.0: PCI INT A -> GSI 16 (level, low) -> IRQ 16
> > [    0.834995] pci 0000:00:02.0: setting latency timer to 64
> > [    1.002572] mtrr: type mismatch for e0000000,10000000 old: write-back new: write-combining
> > [    1.002580] [drm] MTRR allocation failed.  Graphics performance may suffer.
> > [    1.019880] acpi device:03: registered as cooling_device2
> > [    1.021520] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input3
> > [    1.021543] ACPI: Video Device [GFX0] (multi-head: yes  rom: no  post: no)
> > [    1.021855] [drm] Initialized i915 1.6.0 20080730 for 0000:00:02.0 on minor 0
> > 
> > This is with:
> > 
> > 00:02.0 VGA compatible controller: Intel Corporation Mobile GM965/GL960
> > Integrated Graphics Controller (rev 03)
> > 00:02.1 Display controller: Intel Corporation Mobile GM965/GL960
> > Integrated Graphics Controller (rev 03)
> > 
> 
> Matt tells me that this (drop-dead box-killing!) bug is still present
> in current kernels.  Could someone take a look please?
> 
> The code seems very simple, and I couldn't spot a problem.  Probably
> this means that I'm even simpler.
> 

I'm wondering what reading /sys/kernel/debug/dri/.../vma says when 
DRM_DEBUG_CODE is enabled, it seems to have been written for this type of 
debugging.

Re: [PATCH] drm: Prune GEM vma entries

[Matt Mackall]

On Mon, 2010-09-27 at 21:08 +0100, Chris Wilson wrote:
> Hook the GEM vm open/close ops into the generic drm vm open/close so
> that the vma entries are created and destroy appropriately.
> 
> Reported-by: Matt Mackall <mpm@selenic.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

All signs point to this being the correct fix, but I won't have time to
test it while I'm in Japan.

Paul, does this work for you?

> ---
>  drivers/gpu/drm/drm_gem.c |    9 ++++++++-
>  drivers/gpu/drm/drm_vm.c  |   28 ++++++++++++++++++----------
>  include/drm/drmP.h        |    1 +
>  3 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index bf92d07..6fe2cd2 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -528,6 +528,10 @@ void drm_gem_vm_open(struct vm_area_struct *vma)
>  	struct drm_gem_object *obj = vma->vm_private_data;
>  
>  	drm_gem_object_reference(obj);
> +
> +	mutex_lock(&obj->dev->struct_mutex);
> +	drm_vm_open_locked(vma);
> +	mutex_unlock(&obj->dev->struct_mutex);
>  }
>  EXPORT_SYMBOL(drm_gem_vm_open);
>  
> @@ -535,7 +539,10 @@ void drm_gem_vm_close(struct vm_area_struct *vma)
>  {
>  	struct drm_gem_object *obj = vma->vm_private_data;
>  
> -	drm_gem_object_unreference_unlocked(obj);
> +	mutex_lock(&obj->dev->struct_mutex);
> +	drm_vm_close_locked(vma);
> +	drm_gem_object_unreference(obj);
> +	mutex_unlock(&obj->dev->struct_mutex);
>  }
>  EXPORT_SYMBOL(drm_gem_vm_close);
>  
> diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
> index fda6746..5df4506 100644
> --- a/drivers/gpu/drm/drm_vm.c
> +++ b/drivers/gpu/drm/drm_vm.c
> @@ -433,15 +433,7 @@ static void drm_vm_open(struct vm_area_struct *vma)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> -/**
> - * \c close method for all virtual memory types.
> - *
> - * \param vma virtual memory area.
> - *
> - * Search the \p vma private data entry in drm_device::vmalist, unlink it, and
> - * free it.
> - */
> -static void drm_vm_close(struct vm_area_struct *vma)
> +void drm_vm_close_locked(struct vm_area_struct *vma)
>  {
>  	struct drm_file *priv = vma->vm_file->private_data;
>  	struct drm_device *dev = priv->minor->dev;
> @@ -451,7 +443,6 @@ static void drm_vm_close(struct vm_area_struct *vma)
>  		  vma->vm_start, vma->vm_end - vma->vm_start);
>  	atomic_dec(&dev->vma_count);
>  
> -	mutex_lock(&dev->struct_mutex);
>  	list_for_each_entry_safe(pt, temp, &dev->vmalist, head) {
>  		if (pt->vma == vma) {
>  			list_del(&pt->head);
> @@ -459,6 +450,23 @@ static void drm_vm_close(struct vm_area_struct *vma)
>  			break;
>  		}
>  	}
> +}
> +
> +/**
> + * \c close method for all virtual memory types.
> + *
> + * \param vma virtual memory area.
> + *
> + * Search the \p vma private data entry in drm_device::vmalist, unlink it, and
> + * free it.
> + */
> +static void drm_vm_close(struct vm_area_struct *vma)
> +{
> +	struct drm_file *priv = vma->vm_file->private_data;
> +	struct drm_device *dev = priv->minor->dev;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	drm_vm_close_locked(vma);
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 7809d23..774e1d4 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1175,6 +1175,7 @@ extern int drm_release(struct inode *inode, struct file *filp);
>  extern int drm_mmap(struct file *filp, struct vm_area_struct *vma);
>  extern int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma);
>  extern void drm_vm_open_locked(struct vm_area_struct *vma);
> +extern void drm_vm_close_locked(struct vm_area_struct *vma);
>  extern resource_size_t drm_core_get_map_ofs(struct drm_local_map * map);
>  extern resource_size_t drm_core_get_reg_ofs(struct drm_device *dev);
>  extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);


-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] drm: Prune GEM vma entries

[Paul Rolland]

Hello,

On Fri, 01 Oct 2010 19:09:56 -0500
Matt Mackall <mpm@selenic.com> wrote:

> On Mon, 2010-09-27 at 21:08 +0100, Chris Wilson wrote:
> > Hook the GEM vm open/close ops into the generic drm vm open/close so
> > that the vma entries are created and destroy appropriately.
> > 
> > Reported-by: Matt Mackall <mpm@selenic.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> All signs point to this being the correct fix, but I won't have time to
> test it while I'm in Japan.
> 
> Paul, does this work for you?

I've just finished building a kernel with this patch applied.
I now need to keep it running for at least one day to make sure that the
leak is gone for good.

I'll keep you updated on that.

Regards,
Paul

Re: [PATCH] drm: Prune GEM vma entries

[Paul Rolland]

Hello,

You can add a: 
Tested-by: Paul Rolland <rol@as2917.net>

This is indeed fixing the memory leak in size-32 pool on my machine.

Thanks a lot,
Paul

On Mon, 27 Sep 2010 21:08:36 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Hook the GEM vm open/close ops into the generic drm vm open/close so
> that the vma entries are created and destroy appropriately.
> 
> Reported-by: Matt Mackall <mpm@selenic.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_gem.c |    9 ++++++++-
>  drivers/gpu/drm/drm_vm.c  |   28 ++++++++++++++++++----------
>  include/drm/drmP.h        |    1 +
>  3 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index bf92d07..6fe2cd2 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -528,6 +528,10 @@ void drm_gem_vm_open(struct vm_area_struct *vma)
>  	struct drm_gem_object *obj = vma->vm_private_data;
>  
>  	drm_gem_object_reference(obj);
> +
> +	mutex_lock(&obj->dev->struct_mutex);
> +	drm_vm_open_locked(vma);
> +	mutex_unlock(&obj->dev->struct_mutex);
>  }
>  EXPORT_SYMBOL(drm_gem_vm_open);
>  
> @@ -535,7 +539,10 @@ void drm_gem_vm_close(struct vm_area_struct *vma)
>  {
>  	struct drm_gem_object *obj = vma->vm_private_data;
>  
> -	drm_gem_object_unreference_unlocked(obj);
> +	mutex_lock(&obj->dev->struct_mutex);
> +	drm_vm_close_locked(vma);
> +	drm_gem_object_unreference(obj);
> +	mutex_unlock(&obj->dev->struct_mutex);
>  }
>  EXPORT_SYMBOL(drm_gem_vm_close);
>  
> diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
> index fda6746..5df4506 100644
> --- a/drivers/gpu/drm/drm_vm.c
> +++ b/drivers/gpu/drm/drm_vm.c
> @@ -433,15 +433,7 @@ static void drm_vm_open(struct vm_area_struct *vma)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> -/**
> - * \c close method for all virtual memory types.
> - *
> - * \param vma virtual memory area.
> - *
> - * Search the \p vma private data entry in drm_device::vmalist, unlink it, and
> - * free it.
> - */
> -static void drm_vm_close(struct vm_area_struct *vma)
> +void drm_vm_close_locked(struct vm_area_struct *vma)
>  {
>  	struct drm_file *priv = vma->vm_file->private_data;
>  	struct drm_device *dev = priv->minor->dev;
> @@ -451,7 +443,6 @@ static void drm_vm_close(struct vm_area_struct *vma)
>  		  vma->vm_start, vma->vm_end - vma->vm_start);
>  	atomic_dec(&dev->vma_count);
>  
> -	mutex_lock(&dev->struct_mutex);
>  	list_for_each_entry_safe(pt, temp, &dev->vmalist, head) {
>  		if (pt->vma == vma) {
>  			list_del(&pt->head);
> @@ -459,6 +450,23 @@ static void drm_vm_close(struct vm_area_struct *vma)
>  			break;
>  		}
>  	}
> +}
> +
> +/**
> + * \c close method for all virtual memory types.
> + *
> + * \param vma virtual memory area.
> + *
> + * Search the \p vma private data entry in drm_device::vmalist, unlink it, and
> + * free it.
> + */
> +static void drm_vm_close(struct vm_area_struct *vma)
> +{
> +	struct drm_file *priv = vma->vm_file->private_data;
> +	struct drm_device *dev = priv->minor->dev;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	drm_vm_close_locked(vma);
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 7809d23..774e1d4 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1175,6 +1175,7 @@ extern int drm_release(struct inode *inode, struct file *filp);
>  extern int drm_mmap(struct file *filp, struct vm_area_struct *vma);
>  extern int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma);
>  extern void drm_vm_open_locked(struct vm_area_struct *vma);
> +extern void drm_vm_close_locked(struct vm_area_struct *vma);
>  extern resource_size_t drm_core_get_map_ofs(struct drm_local_map * map);
>  extern resource_size_t drm_core_get_reg_ofs(struct drm_device *dev);
>  extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);

Re: [PATCH] drm: Prune GEM vma entries

[Paul Rolland]

Hello,

You can add a: 
Tested-by: Paul Rolland <rol@as2917.net>

This is indeed fixing the memory leak in size-32 pool on my machine.

Thanks a lot,
Paul

On Mon, 27 Sep 2010 21:08:36 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Hook the GEM vm open/close ops into the generic drm vm open/close so
> that the vma entries are created and destroy appropriately.
> 
> Reported-by: Matt Mackall <mpm@selenic.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/drm_gem.c |    9 ++++++++-
>  drivers/gpu/drm/drm_vm.c  |   28 ++++++++++++++++++----------
>  include/drm/drmP.h        |    1 +
>  3 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index bf92d07..6fe2cd2 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -528,6 +528,10 @@ void drm_gem_vm_open(struct vm_area_struct *vma)
>  	struct drm_gem_object *obj = vma->vm_private_data;
>  
>  	drm_gem_object_reference(obj);
> +
> +	mutex_lock(&obj->dev->struct_mutex);
> +	drm_vm_open_locked(vma);
> +	mutex_unlock(&obj->dev->struct_mutex);
>  }
>  EXPORT_SYMBOL(drm_gem_vm_open);
>  
> @@ -535,7 +539,10 @@ void drm_gem_vm_close(struct vm_area_struct *vma)
>  {
>  	struct drm_gem_object *obj = vma->vm_private_data;
>  
> -	drm_gem_object_unreference_unlocked(obj);
> +	mutex_lock(&obj->dev->struct_mutex);
> +	drm_vm_close_locked(vma);
> +	drm_gem_object_unreference(obj);
> +	mutex_unlock(&obj->dev->struct_mutex);
>  }
>  EXPORT_SYMBOL(drm_gem_vm_close);
>  
> diff --git a/drivers/gpu/drm/drm_vm.c b/drivers/gpu/drm/drm_vm.c
> index fda6746..5df4506 100644
> --- a/drivers/gpu/drm/drm_vm.c
> +++ b/drivers/gpu/drm/drm_vm.c
> @@ -433,15 +433,7 @@ static void drm_vm_open(struct vm_area_struct *vma)
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> -/**
> - * \c close method for all virtual memory types.
> - *
> - * \param vma virtual memory area.
> - *
> - * Search the \p vma private data entry in drm_device::vmalist, unlink it, and
> - * free it.
> - */
> -static void drm_vm_close(struct vm_area_struct *vma)
> +void drm_vm_close_locked(struct vm_area_struct *vma)
>  {
>  	struct drm_file *priv = vma->vm_file->private_data;
>  	struct drm_device *dev = priv->minor->dev;
> @@ -451,7 +443,6 @@ static void drm_vm_close(struct vm_area_struct *vma)
>  		  vma->vm_start, vma->vm_end - vma->vm_start);
>  	atomic_dec(&dev->vma_count);
>  
> -	mutex_lock(&dev->struct_mutex);
>  	list_for_each_entry_safe(pt, temp, &dev->vmalist, head) {
>  		if (pt->vma == vma) {
>  			list_del(&pt->head);
> @@ -459,6 +450,23 @@ static void drm_vm_close(struct vm_area_struct *vma)
>  			break;
>  		}
>  	}
> +}
> +
> +/**
> + * \c close method for all virtual memory types.
> + *
> + * \param vma virtual memory area.
> + *
> + * Search the \p vma private data entry in drm_device::vmalist, unlink it, and
> + * free it.
> + */
> +static void drm_vm_close(struct vm_area_struct *vma)
> +{
> +	struct drm_file *priv = vma->vm_file->private_data;
> +	struct drm_device *dev = priv->minor->dev;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	drm_vm_close_locked(vma);
>  	mutex_unlock(&dev->struct_mutex);
>  }
>  
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 7809d23..774e1d4 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1175,6 +1175,7 @@ extern int drm_release(struct inode *inode, struct file *filp);
>  extern int drm_mmap(struct file *filp, struct vm_area_struct *vma);
>  extern int drm_mmap_locked(struct file *filp, struct vm_area_struct *vma);
>  extern void drm_vm_open_locked(struct vm_area_struct *vma);
> +extern void drm_vm_close_locked(struct vm_area_struct *vma);
>  extern resource_size_t drm_core_get_map_ofs(struct drm_local_map * map);
>  extern resource_size_t drm_core_get_reg_ofs(struct drm_device *dev);
>  extern unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait);