Re: [PATCH] drm/vc4: Fix false positive WARN() backtrace on refcount_inc() usage

[Stefan Wahren]

Hi Daniel,


Am 24.11.2017 um 15:52 schrieb Daniel Vetter:

On Thu, Nov 23, 2017 at 09:24:11AM +0100, Boris Brezillon wrote:

On Wed, 22 Nov 2017 16:13:31 -0800
Eric Anholt  wrote:


Boris Brezillon  writes:


On Wed, 22 Nov 2017 13:16:08 -0800
Eric Anholt  wrote:
  

Boris Brezillon  writes:
   

With CONFIG_REFCOUNT_FULL enabled, refcount_inc() complains when it's
passed a refcount object that has its counter set to 0. In this driver,
this is a valid use case since we want to increment ->usecnt only when
the BO object starts to be used by real HW components and this is
definitely not the case when the BO is created.

Fix the problem by using refcount_inc_not_zero() instead of
refcount_inc() and fallback to refcount_set(1) when
refcount_inc_not_zero() returns false. Note that this 2-steps operation
is not racy here because the whole section is protected by a mutex
which guarantees that the counter does not change between the
refcount_inc_not_zero() and refcount_set() calls.

If we're not following the model, and protecting the refcount by a
mutex, shouldn't we just be using addition and subtraction instead of
refcount's atomics?

Actually, this mutex is protecting the bo->madv value which has to be
checked when the counter reaches 0 (when decrementing) or 1 (when
incrementing). We just benefit from this protection here, but ideally
it would be better to have an refcount_inc_allow_zero() as suggested by
Daniel.

Let me restate this to see if it makes sense: The refcount is always >=
0, this is is the only path that increases the refcount from 0 to 1, and
it's (incidentally) protected by a mutex, so there's no race between the
attempted increase from nonzero and the set from nonzero to 1.

Yep.


This seems fine to me as a bugfix.

The discussion is going on in the other thread, let's wait a bit
before taking a decision.

Let's not block the bugfix on reaching perfection imo. I'd merge this as
the minimal fix, and then apply the pretty paint once we have a clear idea
for that.
-Daniel


sorry but i can't find it in the DRI tree.

Regards
Stefan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Fix false positive WARN() backtrace on refcount_inc() usage

[Boris Brezillon]

On Thu, 7 Dec 2017 13:31:34 +0100
Stefan Wahren  wrote:

> Hi Daniel,
> 
> 
> Am 24.11.2017 um 15:52 schrieb Daniel Vetter:
> > On Thu, Nov 23, 2017 at 09:24:11AM +0100, Boris Brezillon wrote:  
> >> On Wed, 22 Nov 2017 16:13:31 -0800
> >> Eric Anholt  wrote:
> >>  
> >>> Boris Brezillon  writes:
> >>>  
>  On Wed, 22 Nov 2017 13:16:08 -0800
>  Eric Anholt  wrote:
>  
> > Boris Brezillon  writes:
> >  
> >> With CONFIG_REFCOUNT_FULL enabled, refcount_inc() complains when it's
> >> passed a refcount object that has its counter set to 0. In this driver,
> >> this is a valid use case since we want to increment ->usecnt only when
> >> the BO object starts to be used by real HW components and this is
> >> definitely not the case when the BO is created.
> >>
> >> Fix the problem by using refcount_inc_not_zero() instead of
> >> refcount_inc() and fallback to refcount_set(1) when
> >> refcount_inc_not_zero() returns false. Note that this 2-steps operation
> >> is not racy here because the whole section is protected by a mutex
> >> which guarantees that the counter does not change between the
> >> refcount_inc_not_zero() and refcount_set() calls.  
> > If we're not following the model, and protecting the refcount by a
> > mutex, shouldn't we just be using addition and subtraction instead of
> > refcount's atomics?  
>  Actually, this mutex is protecting the bo->madv value which has to be
>  checked when the counter reaches 0 (when decrementing) or 1 (when
>  incrementing). We just benefit from this protection here, but ideally
>  it would be better to have an refcount_inc_allow_zero() as suggested by
>  Daniel.  
> >>> Let me restate this to see if it makes sense: The refcount is always >=
> >>> 0, this is is the only path that increases the refcount from 0 to 1, and
> >>> it's (incidentally) protected by a mutex, so there's no race between the
> >>> attempted increase from nonzero and the set from nonzero to 1.  
> >> Yep.
> >>  
> >>> This seems fine to me as a bugfix.  
> >> The discussion is going on in the other thread, let's wait a bit
> >> before taking a decision.  
> > Let's not block the bugfix on reaching perfection imo. I'd merge this as
> > the minimal fix, and then apply the pretty paint once we have a clear idea
> > for that.
> > -Daniel  
> 
> sorry but i can't find it in the DRI tree.

Sorry for the delay. I applied it this morning [1] and it should appear
in the next -rc.

Regards,

Boris

[1]https://cgit.freedesktop.org/drm/drm-misc/log/?h=drm-misc-fixes
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Fix false positive WARN() backtrace on refcount_inc() usage

[Daniel Vetter]

On Thu, Nov 23, 2017 at 09:24:11AM +0100, Boris Brezillon wrote:
> On Wed, 22 Nov 2017 16:13:31 -0800
> Eric Anholt  wrote:
> 
> > Boris Brezillon  writes:
> > 
> > > On Wed, 22 Nov 2017 13:16:08 -0800
> > > Eric Anholt  wrote:
> > >  
> > >> Boris Brezillon  writes:
> > >>   
> > >> > With CONFIG_REFCOUNT_FULL enabled, refcount_inc() complains when it's
> > >> > passed a refcount object that has its counter set to 0. In this driver,
> > >> > this is a valid use case since we want to increment ->usecnt only when
> > >> > the BO object starts to be used by real HW components and this is
> > >> > definitely not the case when the BO is created.
> > >> >
> > >> > Fix the problem by using refcount_inc_not_zero() instead of
> > >> > refcount_inc() and fallback to refcount_set(1) when
> > >> > refcount_inc_not_zero() returns false. Note that this 2-steps operation
> > >> > is not racy here because the whole section is protected by a mutex
> > >> > which guarantees that the counter does not change between the
> > >> > refcount_inc_not_zero() and refcount_set() calls.
> > >> 
> > >> If we're not following the model, and protecting the refcount by a
> > >> mutex, shouldn't we just be using addition and subtraction instead of
> > >> refcount's atomics?  
> > >
> > > Actually, this mutex is protecting the bo->madv value which has to be
> > > checked when the counter reaches 0 (when decrementing) or 1 (when
> > > incrementing). We just benefit from this protection here, but ideally
> > > it would be better to have an refcount_inc_allow_zero() as suggested by
> > > Daniel.  
> > 
> > Let me restate this to see if it makes sense: The refcount is always >=
> > 0, this is is the only path that increases the refcount from 0 to 1, and
> > it's (incidentally) protected by a mutex, so there's no race between the
> > attempted increase from nonzero and the set from nonzero to 1.
> 
> Yep.
> 
> > 
> > This seems fine to me as a bugfix.
> 
> The discussion is going on in the other thread, let's wait a bit
> before taking a decision.

Let's not block the bugfix on reaching perfection imo. I'd merge this as
the minimal fix, and then apply the pretty paint once we have a clear idea
for that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Fix false positive WARN() backtrace on refcount_inc() usage

[Boris Brezillon]

On Wed, 22 Nov 2017 16:13:31 -0800
Eric Anholt  wrote:

> Boris Brezillon  writes:
> 
> > On Wed, 22 Nov 2017 13:16:08 -0800
> > Eric Anholt  wrote:
> >  
> >> Boris Brezillon  writes:
> >>   
> >> > With CONFIG_REFCOUNT_FULL enabled, refcount_inc() complains when it's
> >> > passed a refcount object that has its counter set to 0. In this driver,
> >> > this is a valid use case since we want to increment ->usecnt only when
> >> > the BO object starts to be used by real HW components and this is
> >> > definitely not the case when the BO is created.
> >> >
> >> > Fix the problem by using refcount_inc_not_zero() instead of
> >> > refcount_inc() and fallback to refcount_set(1) when
> >> > refcount_inc_not_zero() returns false. Note that this 2-steps operation
> >> > is not racy here because the whole section is protected by a mutex
> >> > which guarantees that the counter does not change between the
> >> > refcount_inc_not_zero() and refcount_set() calls.
> >> 
> >> If we're not following the model, and protecting the refcount by a
> >> mutex, shouldn't we just be using addition and subtraction instead of
> >> refcount's atomics?  
> >
> > Actually, this mutex is protecting the bo->madv value which has to be
> > checked when the counter reaches 0 (when decrementing) or 1 (when
> > incrementing). We just benefit from this protection here, but ideally
> > it would be better to have an refcount_inc_allow_zero() as suggested by
> > Daniel.  
> 
> Let me restate this to see if it makes sense: The refcount is always >=
> 0, this is is the only path that increases the refcount from 0 to 1, and
> it's (incidentally) protected by a mutex, so there's no race between the
> attempted increase from nonzero and the set from nonzero to 1.

Yep.

> 
> This seems fine to me as a bugfix.

The discussion is going on in the other thread, let's wait a bit
before taking a decision.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Fix false positive WARN() backtrace on refcount_inc() usage

[Eric Anholt]

Boris Brezillon  writes:

> On Wed, 22 Nov 2017 13:16:08 -0800
> Eric Anholt  wrote:
>
>> Boris Brezillon  writes:
>> 
>> > With CONFIG_REFCOUNT_FULL enabled, refcount_inc() complains when it's
>> > passed a refcount object that has its counter set to 0. In this driver,
>> > this is a valid use case since we want to increment ->usecnt only when
>> > the BO object starts to be used by real HW components and this is
>> > definitely not the case when the BO is created.
>> >
>> > Fix the problem by using refcount_inc_not_zero() instead of
>> > refcount_inc() and fallback to refcount_set(1) when
>> > refcount_inc_not_zero() returns false. Note that this 2-steps operation
>> > is not racy here because the whole section is protected by a mutex
>> > which guarantees that the counter does not change between the
>> > refcount_inc_not_zero() and refcount_set() calls.  
>> 
>> If we're not following the model, and protecting the refcount by a
>> mutex, shouldn't we just be using addition and subtraction instead of
>> refcount's atomics?
>
> Actually, this mutex is protecting the bo->madv value which has to be
> checked when the counter reaches 0 (when decrementing) or 1 (when
> incrementing). We just benefit from this protection here, but ideally
> it would be better to have an refcount_inc_allow_zero() as suggested by
> Daniel.

Let me restate this to see if it makes sense: The refcount is always >=
0, this is is the only path that increases the refcount from 0 to 1, and
it's (incidentally) protected by a mutex, so there's no race between the
attempted increase from nonzero and the set from nonzero to 1.

This seems fine to me as a bugfix.


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Fix false positive WARN() backtrace on refcount_inc() usage

[Boris Brezillon]

On Wed, 22 Nov 2017 13:16:08 -0800
Eric Anholt  wrote:

> Boris Brezillon  writes:
> 
> > With CONFIG_REFCOUNT_FULL enabled, refcount_inc() complains when it's
> > passed a refcount object that has its counter set to 0. In this driver,
> > this is a valid use case since we want to increment ->usecnt only when
> > the BO object starts to be used by real HW components and this is
> > definitely not the case when the BO is created.
> >
> > Fix the problem by using refcount_inc_not_zero() instead of
> > refcount_inc() and fallback to refcount_set(1) when
> > refcount_inc_not_zero() returns false. Note that this 2-steps operation
> > is not racy here because the whole section is protected by a mutex
> > which guarantees that the counter does not change between the
> > refcount_inc_not_zero() and refcount_set() calls.  
> 
> If we're not following the model, and protecting the refcount by a
> mutex, shouldn't we just be using addition and subtraction instead of
> refcount's atomics?

Actually, this mutex is protecting the bo->madv value which has to be
checked when the counter reaches 0 (when decrementing) or 1 (when
incrementing). We just benefit from this protection here, but ideally
it would be better to have an refcount_inc_allow_zero() as suggested by
Daniel.

Anyway, I can also use a regular counter and protect it with the madv
mutex, it's just that I thought using the existing refcount
infrastructure would be safer than open-coding it (actually, I found
several unbalanced refcounting issues thanks to this API while
developing the feature).
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Fix false positive WARN() backtrace on refcount_inc() usage

[Eric Anholt]

Boris Brezillon  writes:

> With CONFIG_REFCOUNT_FULL enabled, refcount_inc() complains when it's
> passed a refcount object that has its counter set to 0. In this driver,
> this is a valid use case since we want to increment ->usecnt only when
> the BO object starts to be used by real HW components and this is
> definitely not the case when the BO is created.
>
> Fix the problem by using refcount_inc_not_zero() instead of
> refcount_inc() and fallback to refcount_set(1) when
> refcount_inc_not_zero() returns false. Note that this 2-steps operation
> is not racy here because the whole section is protected by a mutex
> which guarantees that the counter does not change between the
> refcount_inc_not_zero() and refcount_set() calls.

If we're not following the model, and protecting the refcount by a
mutex, shouldn't we just be using addition and subtraction instead of
refcount's atomics?


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/vc4: Fix false positive WARN() backtrace on refcount_inc() usage

[Boris Brezillon]

With CONFIG_REFCOUNT_FULL enabled, refcount_inc() complains when it's
passed a refcount object that has its counter set to 0. In this driver,
this is a valid use case since we want to increment ->usecnt only when
the BO object starts to be used by real HW components and this is
definitely not the case when the BO is created.

Fix the problem by using refcount_inc_not_zero() instead of
refcount_inc() and fallback to refcount_set(1) when
refcount_inc_not_zero() returns false. Note that this 2-steps operation
is not racy here because the whole section is protected by a mutex
which guarantees that the counter does not change between the
refcount_inc_not_zero() and refcount_set() calls.

Fixes: b9f19259b84d ("drm/vc4: Add the DRM_IOCTL_VC4_GEM_MADVISE ioctl")
Reported-by: Stefan Wahren 
Signed-off-by: Boris Brezillon 
---
 drivers/gpu/drm/vc4/vc4_bo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c
index 4ae45d7dac42..2decc8e2c79f 100644
--- a/drivers/gpu/drm/vc4/vc4_bo.c
+++ b/drivers/gpu/drm/vc4/vc4_bo.c
@@ -637,7 +637,8 @@ int vc4_bo_inc_usecnt(struct vc4_bo *bo)
mutex_lock(>madv_lock);
switch (bo->madv) {
case VC4_MADV_WILLNEED:
-   refcount_inc(>usecnt);
+   if (!refcount_inc_not_zero(>usecnt))
+   refcount_set(>usecnt, 1);
ret = 0;
break;
case VC4_MADV_DONTNEED:
-- 
2.11.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel