[PATCH 0/2] cleanup: A couple extensions for conditional resource management

[PATCH 0/2] cleanup: A couple extensions for conditional resource management

[Dan Williams]

Hi Peter, Linus, Greg,

The cond_guard() patch has gone through several rounds of review and is
looking good to me, but is missing feedback from one of you that have
been grappling with a cross-kernel view of what these new facilities
should look like.

Separately I have been running into trouble trying to fit no_free_ptr()
into some cleanup patches and thought of another way to build on the
conditional syntax originated in scoped_cond_guard().

More specifically, scoped_cond_guard() introduced the concept of passing
a statement to the macro to handle the failure case. cond_guard()
extends that to be used within an existing scope to automatically
release a conditionally acquired lock rather than defining a new scope.

The cond_no_free_ptr() helper takes that concept for ending the cleanup
scope for objects when responsibility for freeing them has been
transferred to a 3rd object or subsystem.

---

Dan Williams (1):
      cleanup: Introduce cond_no_free_ptr()

Fabio M. De Francesco (1):
      cleanup: Add cond_guard() to conditional guards


 include/linux/cleanup.h |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

base-commit: 54be6c6c5ae8e0d93a6c4641cb7528eb0b6ba478

[PATCH 1/3] cleanup: Add cond_guard() to conditional guards

[Dan Williams]

From: Fabio M. De Francesco <fabio.maria.de.francesco@linux.intel.com>

Add cond_guard() macro to conditional guards.

cond_guard() is a guard to be used with the conditional variants of locks,
like down_read_trylock() or mutex_lock_interruptible().

It takes a statement (or statement-expression) that is passed as its
second argument. That statement (or statement-expression) is executed if
waiting for a lock is interrupted or if a _trylock() fails in case of
contention.

Usage example:

	cond_guard(mutex_intr, return -EINTR, &mutex);

Consistent with other usage of _guard(), locks are unlocked at the exit of
the scope where cond_guard() is called. This macro can be called multiple
times in the same scope.

Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Fabio M. De Francesco <fabio.maria.de.francesco@linux.intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/cleanup.h |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..602afb85da34 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -134,6 +134,19 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
  *	an anonymous instance of the (guard) class, not recommended for
  *	conditional locks.
  *
+ * cond_guard(name, fail, args...):
+ *	a guard to be used with the conditional variants of locks, like
+ *	down_read_trylock() or mutex_lock_interruptible(). 'fail' is a
+ *	statement or statement-expression that is executed if waiting for a
+ *	lock is interrupted or if a _trylock() fails in case of contention.
+ *
+ *	Example:
+ *
+ *		cond_guard(mutex_intr, return -EINTR, &mutex);
+ *
+ * 	This macro can be called multiple times in the same scope, for it
+ * 	declares unique instances of type 'name'.
+ *
  * scoped_guard (name, args...) { }:
  *	similar to CLASS(name, scope)(args), except the variable (with the
  *	explicit name 'scope') is declard in a for-loop such that its scope is
@@ -165,6 +178,13 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 
 #define __guard_ptr(_name) class_##_name##_lock_ptr
 
+#define __cond_guard(__unique, _name, _fail, args...) \
+	CLASS(_name, __unique)(args); \
+	if (!__guard_ptr(_name)(&__unique)) _fail; \
+	else { }
+#define cond_guard(_name, _fail, args...) \
+	__cond_guard(__UNIQUE_ID(scope), _name, _fail, args)
+
 #define scoped_guard(_name, args...)					\
 	for (CLASS(_name, scope)(args),					\
 	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)

Re: [PATCH 1/3] cleanup: Add cond_guard() to conditional guards

[Linus Torvalds]

On Tue, 27 Feb 2024 at 08:48, Dan Williams <dan.j.williams@intel.com> wrote:
>
>         cond_guard(mutex_intr, return -EINTR, &mutex);

Again, this is *not* helping make code readable and less likely to have bugs.

The macro has obvious deficiencies, like the "_fail" argument not
being surrounded by  "{ }" (the equivalent of parenthesizing an
expression argument), but even with that trivial fix the syntax is
just too ugly to live, and doesn't match normal C syntax.

And yes, we have other macros that don't have normal C syntax, and
they are ugly too (example: #define CHKINFO(ret) in
drivers/video/fbdev/hgafb.c), but we should have higher standards for
globally visible helpers, and we should have *MUCH* higher standards
for helpers that are supposed to be all about reducing mistakes.

Bad / odd syntax does not reduce mistakes.

If a sane 'guard' model doesn't work for some code, the answer is not
to make an insane guard model. The answer is to not use 'guard' in
code like that.

               Linus

[PATCH 2/3] cleanup: Introduce cond_no_free_ptr()

[Dan Williams]

The no_free_ptr() helper cancels automatic cleanup for cases where
assigning the pointer transfers ownership for freeing it. However, it
gets awkward to use when multiple allocations need to be cancelled in
response to one registration call. For example:

    1/ name = kasprintf(...);
    2/ res = kmalloc(...);
    3/ res->name = name;
    4/ rc = insert_resource(..., res);
    5/ if (rc) return rc;

no_free_ptr() cannot be used for 3 since insert_resource() does not
cleanup on failure. no_free_ptr() could be used at 4, but if
insert_resource() fails, the no_free_ptr() was premature. After 5 is
when it is known that it is safe to free @res and @name. However,
no_free_ptr() is awkward there as well because of __must_check().

The options are:
 * Just open code @res = NULL and @name = NULL, but that is a
   non-idiomatic way to use the cleanup helpers.
 * Introduce a no_free_ptr() variant that drops the __must_check, but
   that defeats the purpose of mandating that the caller understands
   that responsibility for freeing has been handed off.
 * Introduce a new helper that combines a condition check to supersede
   the __must_check of no_free_ptr()

So, per that last option, line 5/ from the example becomes:

    5/ cond_no_free_ptr(rc == 0, return rc, res, name);

...and that handles calling no_free_ptr() while also mandating the
negative condition be handled. It is inspired by scoped_cond_guard()
which also takes a statement for the negative condition case.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/cleanup.h |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 602afb85da34..a6d593a60611 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -77,6 +77,28 @@ const volatile void * __must_check_fn(const volatile void *val)
 
 #define return_ptr(p)	return no_free_ptr(p)
 
+#define __cond_no_free_ptrs(p) ({__auto_type __always_unused __ptr = no_free_ptr(p);})
+#define __cond_no_free_ptrs1(p, ...) __cond_no_free_ptrs(p)
+#define __cond_no_free_ptrs2(p, ...) \
+	__cond_no_free_ptrs(p), __cond_no_free_ptrs1(__VA_ARGS__)
+#define __cond_no_free_ptrs3(p, ...) \
+	__cond_no_free_ptrs(p), __cond_no_free_ptrs2(__VA_ARGS__)
+
+/*
+ * When an object is built up by an amalgamation of multiple allocations
+ * each of those need to be cleaned up on error, but there are occasions
+ * where once the object is registered all of those cleanups can be
+ * cancelled.  cond_no_free_ptr() arranges to call no_free_ptr() on all
+ * its arguments (up to 3) if @condition is true and runs @_fail
+ * otherwise (typically to return and trigger auto-cleanup).
+ */
+#define cond_no_free_ptr(condition, _fail, ...)                           \
+	if (condition) {                                                  \
+		CONCATENATE(__cond_no_free_ptrs, COUNT_ARGS(__VA_ARGS__)) \
+		(__VA_ARGS__);                                            \
+	} else {                                                          \
+		_fail;                                                    \
+	}
 
 /*
  * DEFINE_CLASS(name, type, exit, init, init_args...):

Re: [PATCH 2/3] cleanup: Introduce cond_no_free_ptr()

[Linus Torvalds]

On Tue, 27 Feb 2024 at 08:49, Dan Williams <dan.j.williams@intel.com> wrote:
>
>     5/ cond_no_free_ptr(rc == 0, return rc, res, name);

Ugh. Honestly, this is all too ugly for words.

The whole - and only - point for the cond_guard() is to make mistakes
less likely.

This is not it. This makes mistakes unreadable and undebuggable.

             Linus

[PATCH 3/3] cxl/region: Use cond_guard() in show_targetN()

[Dan Williams]

From: Fabio M. De Francesco <fabio.maria.de.francesco@linux.intel.com>

Use cond_guard() in show_target() to not open code an up_read() in an 'out'
block. If the down_read_interruptible() fails, the statement passed to the
second argument of cond_guard() returns -EINTR.

Cc: Peter Zijlstra <peterz@infradead.org>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Fabio M. De Francesco <fabio.maria.de.francesco@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/region.c |   16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index ce0e2d82bb2b..704102f75c14 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -666,28 +666,20 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
 {
 	struct cxl_region_params *p = &cxlr->params;
 	struct cxl_endpoint_decoder *cxled;
-	int rc;
 
-	rc = down_read_interruptible(&cxl_region_rwsem);
-	if (rc)
-		return rc;
+	cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);
 
 	if (pos >= p->interleave_ways) {
 		dev_dbg(&cxlr->dev, "position %d out of range %d\n", pos,
 			p->interleave_ways);
-		rc = -ENXIO;
-		goto out;
+		return -ENXIO;
 	}
 
 	cxled = p->targets[pos];
 	if (!cxled)
-		rc = sysfs_emit(buf, "\n");
-	else
-		rc = sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
-out:
-	up_read(&cxl_region_rwsem);
+		return sysfs_emit(buf, "\n");
 
-	return rc;
+	return sysfs_emit(buf, "%s\n", dev_name(&cxled->cxld.dev));
 }
 
 static int match_free_decoder(struct device *dev, void *data)

Re: [PATCH 3/3] cxl/region: Use cond_guard() in show_targetN()

[Linus Torvalds]

On Tue, 27 Feb 2024 at 08:49, Dan Williams <dan.j.williams@intel.com> wrote:
>
> -       rc = down_read_interruptible(&cxl_region_rwsem);
> -       if (rc)
> -               return rc;
> +       cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);

Yeah, this is an example of how NOT to do things.

If you can't make the syntax be something clean and sane like

        if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem))
                return -EINTR;

then this code should simply not be converted to guards AT ALL.

Note that we have a perfectly fine way to do conditional lock guarding
by simply using helper functions, which actually makes code MORE
READABLE:

        if (!down_read_interruptible(&cxl_region_rwsem))
                return -EINTR;
        rc = do_locked_function();
        up_read(&cxl_region_rwsem);
        return rc;

and notice how there are no special cases, no multiple unlocks, no
NOTHING. And the syntax is clean.

Honestly, if people are going to use 'guard' to write crap code, we
need to really stop that in its tracks.

There is no upside to making up new interfaces that only generate garbage.

This is final. I'm not willing to even entertain this kind of crap.

                     Linus

Re: [PATCH 3/3] cxl/region: Use cond_guard() in show_targetN()

[Dan Williams]

Linus Torvalds wrote:
> On Tue, 27 Feb 2024 at 08:49, Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > -       rc = down_read_interruptible(&cxl_region_rwsem);
> > -       if (rc)
> > -               return rc;
> > +       cond_guard(rwsem_read_intr, return -EINTR, &cxl_region_rwsem);
> 
> Yeah, this is an example of how NOT to do things.
> 
> If you can't make the syntax be something clean and sane like
> 
>         if (!cond_guard(rwsem_read_intr, &cxl_region_rwsem))
>                 return -EINTR;
> 
> then this code should simply not be converted to guards AT ALL.
> 
> Note that we have a perfectly fine way to do conditional lock guarding
> by simply using helper functions, which actually makes code MORE
> READABLE:
> 
>         if (!down_read_interruptible(&cxl_region_rwsem))
>                 return -EINTR;
>         rc = do_locked_function();
>         up_read(&cxl_region_rwsem);
>         return rc;
> 
> and notice how there are no special cases, no multiple unlocks, no
> NOTHING. And the syntax is clean.

Ok, I took the wrong lessons from scoped_cond_guard(). Consider it
dropped.

> Honestly, if people are going to use 'guard' to write crap code, we
> need to really stop that in its tracks.
> 
> There is no upside to making up new interfaces that only generate garbage.
> 
> This is final. I'm not willing to even entertain this kind of crap.

I will also note that these last 3 statements, nuking the proposal from
space, I find excessive. Yes, on the internet no one can hear you being
subtle, but the "MORE READABLE" and "NOTHING" were pretty darn
unequivocal, especially coming from the person who has absolute final
say on what enters his project.

I have been around a while so I take this as par for the course, and I
appreciate blunt feedback. I have had dance teachers tell me my "dancing
is shit", and sometimes that level of bluntness is needed, but that was
also from somebody I have worked with for years. Fabio has not been
around that long, and nothing about what Fabio did was crap, he carried
through on an idea that I asked him to explore and it did not work out.

So Fabio, keep going, thank you for patiently working through this
investigation and my takeaway is that we have successfully discovered
one way this mission to cleanup usage of goto in the CXL subsystem can
not proceed. Back to the drawing board.

Re: [PATCH 3/3] cxl/region: Use cond_guard() in show_targetN()

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 2053 bytes --]

On Tue, 27 Feb 2024 at 13:42, Dan Williams <dan.j.williams@intel.com> wrote:
>
> I will also note that these last 3 statements, nuking the proposal from
> space, I find excessive. Yes, on the internet no one can hear you being
> subtle, but the "MORE READABLE" and "NOTHING" were pretty darn
> unequivocal, especially coming from the person who has absolute final
> say on what enters his project.

Heh. It's not just " one can hear you being subtle", sometimes it's
also "people don't take hints". It can be hard to tell..

Anyway, it's not that I hate the guard things in general. But I do
think they need to be used carefully, and I do think it's very
important that they have clean interfaces.

The current setup came about after quite long discussions about
getting reasonable syntax, and I'm still a bit worried even about the
current simpler ones.

And by "simpler ones" I don't mean our current scoped_cond_guard()
thing. We have exactly one user of it, and I have considered getting
rid of that one user because I think it's absolutely horrid. I haven't
figured out a better syntax for it.

For the non-scoped version, I actually think there *would* be a better
syntax - putting the error case after the macro (the way we put the
success case after the macro for the scoped one).

In fact, maybe the solution is to make the scoped and non-scoped
versions act very similar: we could do something like this:

        [scoped_]cond_guard(name, args) { success } else { fail };

and that syntax feels much more C-line to me.

So maybe something like the attached (TOTALLY UNTESTED!!) patch for
the scoped version, and then the non-scoped version would have the
same syntax (except it would have to generate that __UNIQUE_ID()
thing, of course).

I haven't thought much about this. But I think this would be more
acceptable to me, and also solve some of the ugliness with the current
pre-existing scoped_cond_guard().

I dunno. PeterZ did the existing stuff, but he's offlined due to
shoulder problems so not likely to chip in.

              Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1999 bytes --]

 include/linux/cleanup.h | 7 +++----
 kernel/ptrace.c         | 5 +++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index c2d09bc4f976..a015ac9517a6 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -142,7 +142,7 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
  *	for conditional locks the loop body is skipped when the lock is not
  *	acquired.
  *
- * scoped_cond_guard (name, fail, args...) { }:
+ * scoped_cond_guard (name, args...) { } [ else { fail } :
  *      similar to scoped_guard(), except it does fail when the lock
  *      acquire fails.
  *
@@ -169,11 +169,10 @@ static inline class_##_name##_t class_##_name##ext##_constructor(_init_args) \
 	for (CLASS(_name, scope)(args),					\
 	     *done = NULL; __guard_ptr(_name)(&scope) && !done; done = (void *)1)
 
-#define scoped_cond_guard(_name, _fail, args...) \
+#define scoped_cond_guard(_name, args...) \
 	for (CLASS(_name, scope)(args), \
 	     *done = NULL; !done; done = (void *)1) \
-		if (!__guard_ptr(_name)(&scope)) _fail; \
-		else
+		if (__guard_ptr(_name)(&scope))
 
 /*
  * Additional helper macros for generating lock guards with types, either for
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 2fabd497d659..f509b21a5711 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -441,8 +441,7 @@ static int ptrace_attach(struct task_struct *task, long request,
 	 * SUID, SGID and LSM creds get determined differently
 	 * under ptrace.
 	 */
-	scoped_cond_guard (mutex_intr, return -ERESTARTNOINTR,
-			   &task->signal->cred_guard_mutex) {
+	scoped_cond_guard (mutex_intr, &task->signal->cred_guard_mutex) {
 
 		scoped_guard (task_lock, task) {
 			retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS);
@@ -466,6 +465,8 @@ static int ptrace_attach(struct task_struct *task, long request,
 
 			ptrace_set_stopped(task);
 		}
+	} else {
+		return -ERESTARTNOINTR;
 	}
 
 	/*

Re: [PATCH 3/3] cxl/region: Use cond_guard() in show_targetN()

[Dan Williams]

Linus Torvalds wrote:
> On Tue, 27 Feb 2024 at 13:42, Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > I will also note that these last 3 statements, nuking the proposal from
> > space, I find excessive. Yes, on the internet no one can hear you being
> > subtle, but the "MORE READABLE" and "NOTHING" were pretty darn
> > unequivocal, especially coming from the person who has absolute final
> > say on what enters his project.
> 
> Heh. It's not just " one can hear you being subtle", sometimes it's
> also "people don't take hints". It can be hard to tell..

I appreciate that. It is difficult to judge what size clue bat to carry
from one thread to the next.

> Anyway, it's not that I hate the guard things in general. But I do
> think they need to be used carefully, and I do think it's very
> important that they have clean interfaces.
> 
> The current setup came about after quite long discussions about
> getting reasonable syntax, and I'm still a bit worried even about the
> current simpler ones.
> 
> And by "simpler ones" I don't mean our current scoped_cond_guard()
> thing. We have exactly one user of it, and I have considered getting
> rid of that one user because I think it's absolutely horrid. I haven't
> figured out a better syntax for it.
> 
> For the non-scoped version, I actually think there *would* be a better
> syntax - putting the error case after the macro (the way we put the
> success case after the macro for the scoped one).
> 
> In fact, maybe the solution is to make the scoped and non-scoped
> versions act very similar: we could do something like this:
> 
>         [scoped_]cond_guard(name, args) { success } else { fail };
> 
> and that syntax feels much more C-line to me.
> 
> So maybe something like the attached (TOTALLY UNTESTED!!) patch for
> the scoped version, and then the non-scoped version would have the
> same syntax (except it would have to generate that __UNIQUE_ID()
> thing, of course).

This would have definitely saved me from thinking that passing a
"return" statement to a macro was an idea worth copying. I like that it
puts the onus on the caller to understand "this is a conditional" you
are responsible for handling the conditions, the macro is only handling
releasing the lock at the end of the scope".

> I haven't thought much about this. But I think this would be more
> acceptable to me, and also solve some of the ugliness with the current
> pre-existing scoped_cond_guard().
> 
> I dunno. PeterZ did the existing stuff, but he's offlined due to
> shoulder problems so not likely to chip in.

Ah, ok, yeah has been quiet on this thread for a while. I will take some
inspiration from this proposal and huddle again with Fabio.

Thanks for the nudge.