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

[Linus Torvalds]

On Tue, 27 Feb 2024 at 13:42, Dan Williams <dan.j.williams@xxxxxxxxx> 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
 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;
 	}
 
 	/*