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

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

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
  

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.
  

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
  

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.