mbox series

[RFC,0/2] refcount: attempt to avoid imbalance warnings

Message ID 20220630135934.1799248-1-aahringo@redhat.com (mailing list archive)
Headers show
Series refcount: attempt to avoid imbalance warnings | expand

Message

Alexander Aring June 30, 2022, 1:59 p.m. UTC 
Hi,

This patch tries to avoid some sparse warnings related to
refcount_dec_and_lock() and kref_put_lock().

I send this patch series as RFC because it was necessary to do a kref
change after adding __cond_lock() to refcount_dec_and_lock()
functionality.

For me it looks like we do a lot of acrobatics to avoid sparse warnings
here and I really don't know if it's worth the offer. However this is
what I have now...

- Alex

Alexander Aring (2):
  refcount: add __cond_lock() for conditional lock refcount API
  kref: move kref_put_lock() callback to caller

 include/linux/kref.h     | 24 ++++++++----------------
 include/linux/refcount.h | 21 ++++++++++++++++-----
 lib/refcount.c           | 23 ++++++++++++-----------
 3 files changed, 36 insertions(+), 32 deletions(-)

Comments

Linus Torvalds June 30, 2022, 4:34 p.m. UTC | #1 
On Thu, Jun 30, 2022 at 6:59 AM Alexander Aring <aahringo@redhat.com> wrote:
>
> I send this patch series as RFC because it was necessary to do a kref
> change after adding __cond_lock() to refcount_dec_and_lock()
> functionality.

Can you try something like this instead?

This is two separate patches - one for sparse, and one for the kernel.

This is only *very* lightly tested (ie I tested it on a single kernel
file that used refcount_dec_and_lock())

                Linus
Peter Zijlstra July 1, 2022, 8:47 a.m. UTC | #2 
On Thu, Jun 30, 2022 at 09:34:10AM -0700, Linus Torvalds wrote:

Not commenting on sparse, since I'm not much qualified there, however,

>  include/linux/compiler_types.h | 2 ++
>  include/linux/refcount.h       | 6 +++---
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index d08dfcb0ac68..4f2a819fd60a 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -24,6 +24,7 @@ static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
>  /* context/locking */
>  # define __must_hold(x)	__attribute__((context(x,1,1)))
>  # define __acquires(x)	__attribute__((context(x,0,1)))
> +# define __cond_acquires(x) __attribute__((context(x,0,-1)))
>  # define __releases(x)	__attribute__((context(x,1,0)))
>  # define __acquire(x)	__context__(x,1)
>  # define __release(x)	__context__(x,-1)
> @@ -50,6 +51,7 @@ static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
>  /* context/locking */
>  # define __must_hold(x)
>  # define __acquires(x)
> +# define __cond_acquires(x)
>  # define __releases(x)
>  # define __acquire(x)	(void)0
>  # define __release(x)	(void)0
> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
> index b8a6e387f8f9..a62fcca97486 100644
> --- a/include/linux/refcount.h
> +++ b/include/linux/refcount.h
> @@ -361,9 +361,9 @@ static inline void refcount_dec(refcount_t *r)
>  
>  extern __must_check bool refcount_dec_if_one(refcount_t *r);
>  extern __must_check bool refcount_dec_not_one(refcount_t *r);
> -extern __must_check bool refcount_dec_and_mutex_lock(refcount_t *r, struct mutex *lock);
> -extern __must_check bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock);
> +extern __must_check bool refcount_dec_and_mutex_lock(refcount_t *r, struct mutex *lock) __cond_acquires(lock);
> +extern __must_check bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock) __cond_acquires(lock);
>  extern __must_check bool refcount_dec_and_lock_irqsave(refcount_t *r,
>  						       spinlock_t *lock,
> -						       unsigned long *flags);
> +						       unsigned long *flags) __cond_acquires(lock);
>  #endif /* _LINUX_REFCOUNT_H */


YES!, thank you!
Alexander Aring July 1, 2022, 12:07 p.m. UTC | #3 
Hi,

On Thu, Jun 30, 2022 at 12:34 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Jun 30, 2022 at 6:59 AM Alexander Aring <aahringo@redhat.com> wrote:
> >
> > I send this patch series as RFC because it was necessary to do a kref
> > change after adding __cond_lock() to refcount_dec_and_lock()
> > functionality.
>
> Can you try something like this instead?
>
> This is two separate patches - one for sparse, and one for the kernel.
>
> This is only *very* lightly tested (ie I tested it on a single kernel
> file that used refcount_dec_and_lock())
>

yes that avoids the warnings for fs/dlm.c by calling unlock() when the
kref_put_lock() returns true.

However there exists other users of kref_put_lock() which drops a
sparse warning now after those patches e.g.  net/sunrpc/svcauth.c.
I think I can explain why. It is that kref_put_lock() has a release
callback and it's _optional_ that this release callback calls the
unlock(). If the release callback calls unlock() then the user of
kref_put_lock() signals this with a releases() annotation of the
passed release callback.

It seems that sparse is not detecting this annotation anymore when
it's passed as callback and the function pointer parameter declaration
of kref_put_lock() does not have such annotation. The annotation gets
"dropped" then.

If I change the parameter order and add a annotation to the release
callback, like:

__kref_put_lock(struct kref *kref, spinlock_t *lock,
               void (*release)(struct kref *kref) __releases(lock))
#define kref_put_lock(kref, release, lock) __kref_put_lock(kref, lock, release)

the problem is gone but forces every user to release the lock in the
release callback which isn't required and also cuts the API because
the lock which you want to call unlock() on can be not part of your
container_of(kref) struct.

Then I did a similar thing before which would solve it for every user
because there is simply no function pointer passed as parameter and
the annotation gets never "dropped":

#define kref_put_lock(kref, release, lock) \
(refcount_dec_and_lock(&(kref)->refcount, lock) ? ({ release(kref); 1; }) : 0)

Maybe a functionality of forwarding function annotation if passed as a
function pointer (function pointer declared without annotations) as in
e.g. kref_put_lock() can be added into sparse?

- Alex
Alexander Aring July 1, 2022, 7:09 p.m. UTC | #4 
Hi,

On Fri, Jul 1, 2022 at 8:07 AM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Thu, Jun 30, 2022 at 12:34 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > On Thu, Jun 30, 2022 at 6:59 AM Alexander Aring <aahringo@redhat.com> wrote:
> > >
> > > I send this patch series as RFC because it was necessary to do a kref
> > > change after adding __cond_lock() to refcount_dec_and_lock()
> > > functionality.
> >
> > Can you try something like this instead?
> >
> > This is two separate patches - one for sparse, and one for the kernel.
> >
> > This is only *very* lightly tested (ie I tested it on a single kernel
> > file that used refcount_dec_and_lock())
> >
>
> yes that avoids the warnings for fs/dlm.c by calling unlock() when the
> kref_put_lock() returns true.
>
> However there exists other users of kref_put_lock() which drops a
> sparse warning now after those patches e.g.  net/sunrpc/svcauth.c.
> I think I can explain why. It is that kref_put_lock() has a release
> callback and it's _optional_ that this release callback calls the
> unlock(). If the release callback calls unlock() then the user of
> kref_put_lock() signals this with a releases() annotation of the
> passed release callback.
>
> It seems that sparse is not detecting this annotation anymore when
> it's passed as callback and the function pointer parameter declaration
> of kref_put_lock() does not have such annotation. The annotation gets
> "dropped" then.
>
> If I change the parameter order and add a annotation to the release
> callback, like:
>
> __kref_put_lock(struct kref *kref, spinlock_t *lock,
>                void (*release)(struct kref *kref) __releases(lock))
> #define kref_put_lock(kref, release, lock) __kref_put_lock(kref, lock, release)
>
> the problem is gone but forces every user to release the lock in the
> release callback which isn't required and also cuts the API because
> the lock which you want to call unlock() on can be not part of your
> container_of(kref) struct.
>
> Then I did a similar thing before which would solve it for every user
> because there is simply no function pointer passed as parameter and
> the annotation gets never "dropped":
>
> #define kref_put_lock(kref, release, lock) \
> (refcount_dec_and_lock(&(kref)->refcount, lock) ? ({ release(kref); 1; }) : 0)
>
> Maybe a functionality of forwarding function annotation if passed as a
> function pointer (function pointer declared without annotations) as in
> e.g. kref_put_lock() can be added into sparse?

I think the explanation above is not quite right. I am questioning
myself now why it was working before... and I guess the answer is that
it was working for kref_put_lock() with the callback __releases()
handling. It has somehow now an additional acquire() because the
__cond_acquires() change.

Before the patch:

no warnings:

void foo_release(struct kref *kref)
__releases(&foo_lock)
{
        ...
        unlock(foo_lock);
}

...
kref_put_lock(&foo->kref, foo_release, &foo_lock);

shows context imbalance warnings:

void foo_release(struct kref *kref) { }

if (kref_put_lock(&foo->kref, foo_release, &foo_lock))
        unlock(foo_lock);

After the patch it's vice versa of showing warnings or not about
context imbalances.

- Alex