Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
On 18. 01. 2023. 11:39, Das, Nirmoy wrote:
>>>
>>> Copying Mirsad who reported the issue in case he is still happy to give it
>>> a quick test. Mirsad, I don't know if you are subscribed to one of the two
>>> mailing lists where series was posted. In case not, you can grab both
>>> patches from https://patchwork.freedesktop.org/series/112952/.
>>>
>>> Nirmoy - we also have an IGT written by Chuansheng -
>>> https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more
>>> generic one could be placed in gem_mmap_offset test but this one works too
>>> in my testing and is IMO better than nothing.
>>>
>>> Finally, let me add some tags below:
>>>
>>> On 17/01/2023 17:52, Nirmoy Das wrote:
drm_vma_node_allow() and drm_vma_node_revoke() should be called in
balanced pairs. We call drm_vma_node_allow() once per-file everytime a
user calls mmap_offset, but only call drm_vma_node_revoke once per-file
on each mmap_offset. As the mmap_offset is reused by the client, the
per-file vm_count may remain non-zero and the rbtree leaked.
Call drm_vma_node_allow_once() instead to prevent that memory leak.
Cc: Tvrtko Ursulin
Cc: Andi Shyti
>>>
>>> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather
>>> than a plain list")
>>> Reported-by: Chuansheng Liu
>>> Reported-by: Mirsad Todorovac
>>> Cc: # v5.7+
>>> Reviewed-by: Tvrtko Ursulin
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 4f69bff63068..2aac6bf78740 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
out:
if (file)
- drm_vma_node_allow(&mmo->vma_node, file);
+ drm_vma_node_allow_once(&mmo->vma_node, file);
return mmo;
err:
>>
>> The drm/i915 patch seems OK and there are currently no memory leaks as of
>> reported by /sys/kernel/debug/kmemleak under the same Chrome load that
>> triggered
>> the initial bug ...
>
>
> Thanks, Mirsad for quickly checking this!
P.S.
Dear Nirmoy,
To let me explain, as I work on the Academy of Fine Arts and the Faculty of
Graphic Arts,
video and multimedia drivers are of my special interest, so I kinda enjoyed
this one.
No need to thank. ;-)
In case of more bugs discovered, I will be glad to test on my available
platforms,
considering it a part of my research. I can justify time spent on graphic and
multimedia
drivers, so it is not a big deal.
Regards,
Mirsad
--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
The European Union
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
Hi,
On 1/18/23 11:39, Das, Nirmoy wrote:
On 1/18/2023 11:26 AM, Mirsad Todorovac wrote:
Hi,
On 1/18/23 10:19, Tvrtko Ursulin wrote:
Thanks for working on this, it looks good to me and it aligns with how i915
uses the facility.
Copying Mirsad who reported the issue in case he is still happy to give it a quick test. Mirsad, I don't know if you are
subscribed to one of the two mailing lists where series was posted. In case not, you can grab both patches from
https://patchwork.freedesktop.org/series/112952/.
Nirmoy - we also have an IGT written by Chuansheng - https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more
generic one could be placed in gem_mmap_offset test but this one works too in my testing and is IMO better than nothing.
Finally, let me add some tags below:
On 17/01/2023 17:52, Nirmoy Das wrote:
drm_vma_node_allow() and drm_vma_node_revoke() should be called in
balanced pairs. We call drm_vma_node_allow() once per-file everytime a
user calls mmap_offset, but only call drm_vma_node_revoke once per-file
on each mmap_offset. As the mmap_offset is reused by the client, the
per-file vm_count may remain non-zero and the rbtree leaked.
Call drm_vma_node_allow_once() instead to prevent that memory leak.
Cc: Tvrtko Ursulin
Cc: Andi Shyti
Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a
plain list")
Reported-by: Chuansheng Liu
Reported-by: Mirsad Todorovac
Cc: # v5.7+
Reviewed-by: Tvrtko Ursulin
Regards,
Tvrtko
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 4f69bff63068..2aac6bf78740 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
out:
if (file)
- drm_vma_node_allow(&mmo->vma_node, file);
+ drm_vma_node_allow_once(&mmo->vma_node, file);
return mmo;
err:
The drm/i915 patch seems OK and there are currently no memory leaks as of
reported by /sys/kernel/debug/kmemleak under the same Chrome load that triggered
the initial bug ...
Thanks, Mirsad for quickly checking this!
There was no problem, Nirmoy, everything applied neatly :)
Regards,
Mirsad
--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
Re: [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
Hi,
On 1/18/23 10:19, Tvrtko Ursulin wrote:
Thanks for working on this, it looks good to me and it aligns with how i915
uses the facility.
Copying Mirsad who reported the issue in case he is still happy to give it a quick test. Mirsad, I don't know if you are subscribed
to one of the two mailing lists where series was posted. In case not, you can grab both patches from
https://patchwork.freedesktop.org/series/112952/.
Nirmoy - we also have an IGT written by Chuansheng - https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more
generic one could be placed in gem_mmap_offset test but this one works too in my testing and is IMO better than nothing.
Finally, let me add some tags below:
On 17/01/2023 17:52, Nirmoy Das wrote:
drm_vma_node_allow() and drm_vma_node_revoke() should be called in
balanced pairs. We call drm_vma_node_allow() once per-file everytime a
user calls mmap_offset, but only call drm_vma_node_revoke once per-file
on each mmap_offset. As the mmap_offset is reused by the client, the
per-file vm_count may remain non-zero and the rbtree leaked.
Call drm_vma_node_allow_once() instead to prevent that memory leak.
Cc: Tvrtko Ursulin
Cc: Andi Shyti
Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a
plain list")
Reported-by: Chuansheng Liu
Reported-by: Mirsad Todorovac
Cc: # v5.7+
Reviewed-by: Tvrtko Ursulin
Regards,
Tvrtko
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 4f69bff63068..2aac6bf78740 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
out:
if (file)
- drm_vma_node_allow(&mmo->vma_node, file);
+ drm_vma_node_allow_once(&mmo->vma_node, file);
return mmo;
err:
The drm/i915 patch seems OK and there are currently no memory leaks as of
reported by /sys/kernel/debug/kmemleak under the same Chrome load that triggered
the initial bug ...
Will post you if there are any changes.
Regards,
Mirsad
--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
On 1/18/2023 11:26 AM, Mirsad Todorovac wrote:
Hi,
On 1/18/23 10:19, Tvrtko Ursulin wrote:
Thanks for working on this, it looks good to me and it aligns with
how i915 uses the facility.
Copying Mirsad who reported the issue in case he is still happy to
give it a quick test. Mirsad, I don't know if you are subscribed to
one of the two mailing lists where series was posted. In case not,
you can grab both patches from
https://patchwork.freedesktop.org/series/112952/.
Nirmoy - we also have an IGT written by Chuansheng -
https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4.
A more generic one could be placed in gem_mmap_offset test but this
one works too in my testing and is IMO better than nothing.
Finally, let me add some tags below:
On 17/01/2023 17:52, Nirmoy Das wrote:
drm_vma_node_allow() and drm_vma_node_revoke() should be called in
balanced pairs. We call drm_vma_node_allow() once per-file everytime a
user calls mmap_offset, but only call drm_vma_node_revoke once per-file
on each mmap_offset. As the mmap_offset is reused by the client, the
per-file vm_count may remain non-zero and the rbtree leaked.
Call drm_vma_node_allow_once() instead to prevent that memory leak.
Cc: Tvrtko Ursulin
Cc: Andi Shyti
Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree
rather than a plain list")
Reported-by: Chuansheng Liu
Reported-by: Mirsad Todorovac
Cc: # v5.7+
Reviewed-by: Tvrtko Ursulin
Regards,
Tvrtko
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 4f69bff63068..2aac6bf78740 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
out:
if (file)
- drm_vma_node_allow(&mmo->vma_node, file);
+ drm_vma_node_allow_once(&mmo->vma_node, file);
return mmo;
err:
The drm/i915 patch seems OK and there are currently no memory leaks as of
reported by /sys/kernel/debug/kmemleak under the same Chrome load that
triggered
the initial bug ...
Thanks, Mirsad for quickly checking this!
Nirmoy
Will post you if there are any changes.
Regards,
Mirsad
Re: [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
Hi,
On Wed, Jan 18, 2023 at 09:19:40AM +, Tvrtko Ursulin wrote:
>
>
> Hi,
>
> Thanks for working on this, it looks good to me and it aligns with how i915
> uses the facility.
>
> Copying Mirsad who reported the issue in case he is still happy to give it a
> quick test. Mirsad, I don't know if you are subscribed to one of the two
> mailing lists where series was posted. In case not, you can grab both patches
> from https://patchwork.freedesktop.org/series/112952/.
>
> Nirmoy - we also have an IGT written by Chuansheng -
> https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more
> generic one could be placed in gem_mmap_offset test but this one works too in
> my testing and is IMO better than nothing.
>
> Finally, let me add some tags below:
>
> On 17/01/2023 17:52, Nirmoy Das wrote:
> > drm_vma_node_allow() and drm_vma_node_revoke() should be called in
> > balanced pairs. We call drm_vma_node_allow() once per-file everytime a
> > user calls mmap_offset, but only call drm_vma_node_revoke once per-file
> > on each mmap_offset. As the mmap_offset is reused by the client, the
> > per-file vm_count may remain non-zero and the rbtree leaked.
> >
> > Call drm_vma_node_allow_once() instead to prevent that memory leak.
> >
> > Cc: Tvrtko Ursulin
> > Cc: Andi Shyti
>
> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather
> than a plain list")
> Reported-by: Chuansheng Liu
> Reported-by: Mirsad Todorovac
> Cc: # v5.7+
> Reviewed-by: Tvrtko Ursulin
Nirmoy, you can also add:
Reviewed-by: Andi Shyti
Will this go through the drm branch?
Andi
> Regards,
>
> Tvrtko
>
> >
> > Signed-off-by: Nirmoy Das
> > ---
> > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > index 4f69bff63068..2aac6bf78740 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
> > GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
> > out:
> > if (file)
> > - drm_vma_node_allow(&mmo->vma_node, file);
> > + drm_vma_node_allow_once(&mmo->vma_node, file);
> > return mmo;
> > err:
Re: [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
Hi Tvrtko,
On 1/18/2023 10:19 AM, Tvrtko Ursulin wrote:
Hi,
Thanks for working on this, it looks good to me and it aligns with how
i915 uses the facility.
Copying Mirsad who reported the issue in case he is still happy to
give it a quick test. Mirsad, I don't know if you are subscribed to
one of the two mailing lists where series was posted. In case not, you
can grab both patches from
https://patchwork.freedesktop.org/series/112952/.
Nirmoy - we also have an IGT written by Chuansheng -
https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A
more generic one could be placed in gem_mmap_offset test but this one
works too in my testing and is IMO better than nothing.
This looks good to me. let's get this merge and I can look into
improving it at later point.
Finally, let me add some tags below:
On 17/01/2023 17:52, Nirmoy Das wrote:
drm_vma_node_allow() and drm_vma_node_revoke() should be called in
balanced pairs. We call drm_vma_node_allow() once per-file everytime a
user calls mmap_offset, but only call drm_vma_node_revoke once per-file
on each mmap_offset. As the mmap_offset is reused by the client, the
per-file vm_count may remain non-zero and the rbtree leaked.
Call drm_vma_node_allow_once() instead to prevent that memory leak.
Cc: Tvrtko Ursulin
Cc: Andi Shyti
Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree
rather than a plain list")
Reported-by: Chuansheng Liu
Reported-by: Mirsad Todorovac
Cc: # v5.7+
Reviewed-by: Tvrtko Ursulin
Thanks for your review and those extra tags.
Nirmoy
Regards,
Tvrtko
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 4f69bff63068..2aac6bf78740 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
out:
if (file)
- drm_vma_node_allow(&mmo->vma_node, file);
+ drm_vma_node_allow_once(&mmo->vma_node, file);
return mmo;
err:
Re: [PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
Hi,
Thanks for working on this, it looks good to me and it aligns with how i915
uses the facility.
Copying Mirsad who reported the issue in case he is still happy to give it a
quick test. Mirsad, I don't know if you are subscribed to one of the two
mailing lists where series was posted. In case not, you can grab both patches
from https://patchwork.freedesktop.org/series/112952/.
Nirmoy - we also have an IGT written by Chuansheng -
https://patchwork.freedesktop.org/patch/515720/?series=101035&rev=4. A more
generic one could be placed in gem_mmap_offset test but this one works too in my
testing and is IMO better than nothing.
Finally, let me add some tags below:
On 17/01/2023 17:52, Nirmoy Das wrote:
drm_vma_node_allow() and drm_vma_node_revoke() should be called in
balanced pairs. We call drm_vma_node_allow() once per-file everytime a
user calls mmap_offset, but only call drm_vma_node_revoke once per-file
on each mmap_offset. As the mmap_offset is reused by the client, the
per-file vm_count may remain non-zero and the rbtree leaked.
Call drm_vma_node_allow_once() instead to prevent that memory leak.
Cc: Tvrtko Ursulin
Cc: Andi Shyti
Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a
plain list")
Reported-by: Chuansheng Liu
Reported-by: Mirsad Todorovac
Cc: # v5.7+
Reviewed-by: Tvrtko Ursulin
Regards,
Tvrtko
Signed-off-by: Nirmoy Das
---
drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 4f69bff63068..2aac6bf78740 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
out:
if (file)
- drm_vma_node_allow(&mmo->vma_node, file);
+ drm_vma_node_allow_once(&mmo->vma_node, file);
return mmo;
err:
[PATCH 2/2] drm/i915: Fix a memory leak with reused mmap_offset
drm_vma_node_allow() and drm_vma_node_revoke() should be called in balanced pairs. We call drm_vma_node_allow() once per-file everytime a user calls mmap_offset, but only call drm_vma_node_revoke once per-file on each mmap_offset. As the mmap_offset is reused by the client, the per-file vm_count may remain non-zero and the rbtree leaked. Call drm_vma_node_allow_once() instead to prevent that memory leak. Cc: Tvrtko Ursulin Cc: Andi Shyti Signed-off-by: Nirmoy Das --- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c index 4f69bff63068..2aac6bf78740 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c @@ -697,7 +697,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj, GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo); out: if (file) - drm_vma_node_allow(&mmo->vma_node, file); + drm_vma_node_allow_once(&mmo->vma_node, file); return mmo; err: -- 2.39.0
