[Cluster-devel] [RFC 0/2] refcount: attempt to avoid imbalance warnings

[Cluster-devel] [RFC 0/2] refcount: attempt to avoid imbalance warnings

[Alexander Aring]

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(-)

-- 
2.31.1

[Cluster-devel] [RFC 1/2] refcount: add __cond_lock() for conditional lock refcount API

[Alexander Aring]

This patch adds the __cond_lock() macro to refcounts conditional lock
API. Currently sparse cannot detect the conditional lock handling of
refcount_dec_and_lock() functionality and prints a context imbalance
warning like:

warning: context imbalance in 'put_rsb' - unexpected unlock

with this patch and having the refcount_dec_and_lock() functionality
inside the if condition to decide whenever doing unlock or not the
warning disappears.

The patch follows a similar naming scheme like raw_spin_trylock() by
adding a "raw_" prefix to refcount_dec_and_lock() functionality and
introduce a macro for the replaced functions that uses __cond_lock()
to signal that an acquire depends on the return value of the passed
function.

A cast to bool seems to be necessary because __cond_lock() does return a
non-boolean scalar type.

The __must_check annotation was tested and is still working with this
patch applied.

Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 include/linux/refcount.h | 21 ++++++++++++++++-----
 lib/refcount.c           | 23 ++++++++++++-----------
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/include/linux/refcount.h b/include/linux/refcount.h
index b8a6e387f8f9..be7b970ce475 100644
--- a/include/linux/refcount.h
+++ b/include/linux/refcount.h
@@ -361,9 +361,20 @@ 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_lock_irqsave(refcount_t *r,
-						       spinlock_t *lock,
-						       unsigned long *flags);
+extern __must_check bool raw_refcount_dec_and_mutex_lock(refcount_t *r,
+							 struct mutex *lock);
+#define refcount_dec_and_mutex_lock(r, lock) \
+	((bool)(__cond_lock(lock, raw_refcount_dec_and_mutex_lock(r, lock))))
+
+extern __must_check bool raw_refcount_dec_and_lock(refcount_t *r,
+						   spinlock_t *lock);
+#define refcount_dec_and_lock(r, lock) \
+	((bool)(__cond_lock(lock, raw_refcount_dec_and_lock(r, lock))))
+
+extern __must_check bool raw_refcount_dec_and_lock_irqsave(refcount_t *r,
+							   spinlock_t *lock,
+							   unsigned long *flags);
+#define refcount_dec_and_lock_irqsave(r, lock, flags) \
+	((bool)(__cond_lock(lock, raw_refcount_dec_and_lock_irqsave(r, lock, flags))))
+
 #endif /* _LINUX_REFCOUNT_H */
diff --git a/lib/refcount.c b/lib/refcount.c
index a207a8f22b3c..1a8c7b9aba23 100644
--- a/lib/refcount.c
+++ b/lib/refcount.c
@@ -110,7 +110,7 @@ EXPORT_SYMBOL(refcount_dec_not_one);
  * Return: true and hold mutex if able to decrement refcount to 0, false
  *         otherwise
  */
-bool refcount_dec_and_mutex_lock(refcount_t *r, struct mutex *lock)
+bool raw_refcount_dec_and_mutex_lock(refcount_t *r, struct mutex *lock)
 {
 	if (refcount_dec_not_one(r))
 		return false;
@@ -123,11 +123,11 @@ bool refcount_dec_and_mutex_lock(refcount_t *r, struct mutex *lock)
 
 	return true;
 }
-EXPORT_SYMBOL(refcount_dec_and_mutex_lock);
+EXPORT_SYMBOL(raw_refcount_dec_and_mutex_lock);
 
 /**
- * refcount_dec_and_lock - return holding spinlock if able to decrement
- *                         refcount to 0
+ * raw_refcount_dec_and_lock - return holding spinlock if able to decrement
+ *			       refcount to 0
  * @r: the refcount
  * @lock: the spinlock to be locked
  *
@@ -141,7 +141,7 @@ EXPORT_SYMBOL(refcount_dec_and_mutex_lock);
  * Return: true and hold spinlock if able to decrement refcount to 0, false
  *         otherwise
  */
-bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
+bool raw_refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
 {
 	if (refcount_dec_not_one(r))
 		return false;
@@ -154,11 +154,12 @@ bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
 
 	return true;
 }
-EXPORT_SYMBOL(refcount_dec_and_lock);
+EXPORT_SYMBOL(raw_refcount_dec_and_lock);
 
 /**
- * refcount_dec_and_lock_irqsave - return holding spinlock with disabled
- *                                 interrupts if able to decrement refcount to 0
+ * raw_refcount_dec_and_lock_irqsave - return holding spinlock with disabled
+ *				       interrupts if able to decrement
+ *				       refcount to 0
  * @r: the refcount
  * @lock: the spinlock to be locked
  * @flags: saved IRQ-flags if the is acquired
@@ -169,8 +170,8 @@ EXPORT_SYMBOL(refcount_dec_and_lock);
  * Return: true and hold spinlock if able to decrement refcount to 0, false
  *         otherwise
  */
-bool refcount_dec_and_lock_irqsave(refcount_t *r, spinlock_t *lock,
-				   unsigned long *flags)
+bool raw_refcount_dec_and_lock_irqsave(refcount_t *r, spinlock_t *lock,
+				       unsigned long *flags)
 {
 	if (refcount_dec_not_one(r))
 		return false;
@@ -183,4 +184,4 @@ bool refcount_dec_and_lock_irqsave(refcount_t *r, spinlock_t *lock,
 
 	return true;
 }
-EXPORT_SYMBOL(refcount_dec_and_lock_irqsave);
+EXPORT_SYMBOL(raw_refcount_dec_and_lock_irqsave);
-- 
2.31.1

[Cluster-devel] [RFC 1/2] refcount: add __cond_lock() for conditional lock refcount API

[Peter Zijlstra]

On Thu, Jun 30, 2022 at 09:59:33AM -0400, Alexander Aring wrote:
> This patch adds the __cond_lock() macro to refcounts conditional lock
> API. Currently sparse cannot detect the conditional lock handling of
> refcount_dec_and_lock() functionality and prints a context imbalance
> warning like:
> 
> warning: context imbalance in 'put_rsb' - unexpected unlock
> 
> with this patch and having the refcount_dec_and_lock() functionality
> inside the if condition to decide whenever doing unlock or not the
> warning disappears.
> 
> The patch follows a similar naming scheme like raw_spin_trylock() by
> adding a "raw_" prefix to refcount_dec_and_lock() functionality and
> introduce a macro for the replaced functions that uses __cond_lock()
> to signal that an acquire depends on the return value of the passed
> function.
> 
> A cast to bool seems to be necessary because __cond_lock() does return a
> non-boolean scalar type.

I hate the __cond_lock() think with a passions. Please just fix sparse
to not suck.

[Cluster-devel] [RFC 2/2] kref: move kref_put_lock() callback to caller

[Alexander Aring]

This patch moves the release callback call to the caller of kref_put_lock()
functionality. Since refcount_dec_and_lock() uses __cond_lock() we get
the following warning for e.g. net/sunrpc/svcauth.c:

warning: context imbalance in 'auth_domain_put' - wrong count at exit

The warning occurs now because it seems that before __cond_lock() change
sparse was able to detect the correct locking behaviour. Now it thinks
there is an additional lock acquire. However the __cond_lock()
instrumentation in refcount_dec_and_lock() was making it possible to
avoid sparse warnings by evaluating the return value and unlock the lock
if conditional necessary.

This patch solves the problem by just do the passed release callback
call based by the return value of kref_put_lock() and not inside of
kref_put_lock() and evaluating the return value of
refcount_dec_and_lock() that surprisingly sparse can recognize.

It seems it's only possible to have the one way or the other. This patch
changes the kref_put_lock() in way that it works like
refcount_dec_and_lock() way with __cond_lock().

Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 include/linux/kref.h | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/include/linux/kref.h b/include/linux/kref.h
index d32e21a2538c..a70d45940d55 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -68,27 +68,19 @@ static inline int kref_put(struct kref *kref, void (*release)(struct kref *kref)
 	return 0;
 }
 
-static inline int kref_put_mutex(struct kref *kref,
-				 void (*release)(struct kref *kref),
-				 struct mutex *lock)
+static inline bool raw_kref_put_mutex(struct kref *kref, struct mutex *lock)
 {
-	if (refcount_dec_and_mutex_lock(&kref->refcount, lock)) {
-		release(kref);
-		return 1;
-	}
-	return 0;
+	return refcount_dec_and_mutex_lock(&kref->refcount, lock);
 }
+#define kref_put_mutex(kref, release, lock) \
+	((raw_kref_put_mutex(kref, lock)) ? ({ release(kref); 1; }) : 0)
 
-static inline int kref_put_lock(struct kref *kref,
-				void (*release)(struct kref *kref),
-				spinlock_t *lock)
+static inline bool raw_kref_put_lock(struct kref *kref, spinlock_t *lock)
 {
-	if (refcount_dec_and_lock(&kref->refcount, lock)) {
-		release(kref);
-		return 1;
-	}
-	return 0;
+	return refcount_dec_and_lock(&kref->refcount, lock);
 }
+#define kref_put_lock(kref, release, lock) \
+	((raw_kref_put_lock(kref, lock)) ? ({ release(kref); 1; }) : 0)
 
 /**
  * kref_get_unless_zero - Increment refcount for object unless it is zero.
-- 
2.31.1

[Cluster-devel] [RFC 0/2] refcount: attempt to avoid imbalance warnings

[Linus Torvalds]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sparse.patch
Type: text/x-patch
Size: 2345 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20220630/eec69d2f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: kernel.patch
Type: text/x-patch
Size: 2002 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/cluster-devel/attachments/20220630/eec69d2f/attachment-0001.bin>

[Cluster-devel] [RFC 0/2] refcount: attempt to avoid imbalance warnings

[Peter Zijlstra]

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!

[Cluster-devel] [RFC 0/2] refcount: attempt to avoid imbalance warnings

[Alexander Aring]

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

[Cluster-devel] [RFC 0/2] refcount: attempt to avoid imbalance warnings

[Alexander Aring]

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