| Message ID | 20230117175236.22317-2-nirmoy.das@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/drm_vma_manager: Add drm_vma_node_allow_once() | expand |
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 <tvrtko.ursulin@linux.intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list") Reported-by: Chuansheng Liu <chuansheng.liu@intel.com> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> Cc: <stable@vger.kernel.org> # v5.7+ Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > --- > 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:
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 <tvrtko.ursulin@linux.intel.com> >> Cc: Andi Shyti <andi.shyti@linux.intel.com> > > Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree > rather than a plain list") > Reported-by: Chuansheng Liu <chuansheng.liu@intel.com> > Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> > Cc: <stable@vger.kernel.org> # v5.7+ > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Thanks for your review and those extra tags. Nirmoy > > Regards, > > Tvrtko > >> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >> --- >> 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:
Hi, On Wed, Jan 18, 2023 at 09:19:40AM +0000, 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 <tvrtko.ursulin@linux.intel.com> > > Cc: Andi Shyti <andi.shyti@linux.intel.com> > > Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list") > Reported-by: Chuansheng Liu <chuansheng.liu@intel.com> > Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> > Cc: <stable@vger.kernel.org> # v5.7+ > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Nirmoy, you can also add: Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Will this go through the drm branch? Andi > Regards, > > Tvrtko > > > > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> > > --- > > 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:
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 <tvrtko.ursulin@linux.intel.com> >> Cc: Andi Shyti <andi.shyti@linux.intel.com> > > Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list") > Reported-by: Chuansheng Liu <chuansheng.liu@intel.com> > Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> > Cc: <stable@vger.kernel.org> # v5.7+ > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Regards, > > Tvrtko > >> >> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >> --- >> 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
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 <tvrtko.ursulin@linux.intel.com> >>> Cc: Andi Shyti <andi.shyti@linux.intel.com> >> >> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree >> rather than a plain list") >> Reported-by: Chuansheng Liu <chuansheng.liu@intel.com> >> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> >> Cc: <stable@vger.kernel.org> # v5.7+ >> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Regards, >> >> Tvrtko >> >>> >>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >>> --- >>> 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 >
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 <tvrtko.ursulin@linux.intel.com> >>>> Cc: Andi Shyti <andi.shyti@linux.intel.com> >>> >>> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list") >>> Reported-by: Chuansheng Liu <chuansheng.liu@intel.com> >>> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> >>> Cc: <stable@vger.kernel.org> # v5.7+ >>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> Regards, >>> >>> Tvrtko >>> >>>> >>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >>>> --- >>>> 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
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 <tvrtko.ursulin@linux.intel.com> >>>> Cc: Andi Shyti <andi.shyti@linux.intel.com> >>> >>> Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list") >>> Reported-by: Chuansheng Liu <chuansheng.liu@intel.com> >>> Reported-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> >>> Cc: <stable@vger.kernel.org> # v5.7+ >>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> Regards, >>> >>> Tvrtko >>> >>>> >>>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> >>>> --- >>>> 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
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:
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 <tvrtko.ursulin@linux.intel.com> Cc: Andi Shyti <andi.shyti@linux.intel.com> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)