[0/3] cleanup: add if_not_cond_guard macro

Message

David Lechner Oct. 1, 2024, 10:30 p.m. UTC

So far, I have not found scoped_cond_guard() to be nice to work with.
We have been using it quite a bit in the IIO subsystem via the
iio_device_claim_direct_scoped() macro.

The main thing I don't like is that scoped_cond_guard() uses a for loop
internally. In the IIO subsystem, we usually try to return as early as
possible, so often we are returning from all paths from withing this
hidden for loop. However, since it is a for loop, the compiler thinks
that it possible to exit the for loop and so we end up having to use
unreachable() after the end of the scope to avoid a compiler warning.
This is illustrated in the ad7380 patch in this series and there are 36
more instance of unreachable() already introduced in the IIO subsystem
because of this.

Also, scoped_cond_guard() is they only macro for conditional guards in
cleanup.h currently. This means that so far, patches adopting this are
generally converting something that wasn't scoped to be scoped. This
results in changing the indentation of a lot of lines of code, which is
just noise in the patches.

To avoid these issues, the natural thing to do would be to have a
non-scoped version of the scoped_cond_guard() macro. There was was a
rejected attempt to do this in [1], where one of the complaints was:

> > -       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.

[1]: https://lore.kernel.org/all/[e-mail]/

I couldn't find a way to make a cond_guard() macro that would work like
exactly as suggested (the problem is that you can't declare a variable
in the condition expression of an if statement in C). So I am proposing
a macro that reads basically the same as the above so it still reads
almost like normal C code even though it hides the if statement a bit.

        if_not_cond_guard(rwsem_read_intr, &cxl_region_rwsem)
                return -EINTR;

The "not" is baked into the macro because in most cases, failing to
obtain the lock is the abnormal condition and generally we want to have
the abnormal path be the indented one.

As example users, I've include a modified version of [2] from the
rejected series and an ADC patch that shows how this avoids the
unreachable() and too much indentation issues in the IIO subsystem.

[2]: https://lore.kernel.org/all/[e-mail]/

---
David Lechner (3):
      cleanup: add conditional guard helper
      iio: adc: ad7380: use if_not_cond_guard for claim direct
      cxl/region: Use cond_guard() in show_targetN()

 drivers/cxl/core/region.c | 18 ++++--------
 drivers/iio/adc/ad7380.c  | 70 +++++++++++++++++++++++------------------------
 include/linux/cleanup.h   | 11 ++++++++
 3 files changed, 51 insertions(+), 48 deletions(-)
---
base-commit: 431c39f6d3edbab14f48dbf37a58ccdc0ac3be1e
change-id: 20241001-cleanup-if_not_cond_guard-0981d867ddf8

Best regards,

Comments

Dan Williams Oct. 2, 2024, 2:13 a.m. UTC | #1

David Lechner wrote:
> So far, I have not found scoped_cond_guard() to be nice to work with.
> We have been using it quite a bit in the IIO subsystem via the
> iio_device_claim_direct_scoped() macro.
> 
> The main thing I don't like is that scoped_cond_guard() uses a for loop
> internally. In the IIO subsystem, we usually try to return as early as
> possible, so often we are returning from all paths from withing this
> hidden for loop. However, since it is a for loop, the compiler thinks
> that it possible to exit the for loop and so we end up having to use
> unreachable() after the end of the scope to avoid a compiler warning.
> This is illustrated in the ad7380 patch in this series and there are 36
> more instance of unreachable() already introduced in the IIO subsystem
> because of this.
> 
> Also, scoped_cond_guard() is they only macro for conditional guards in
> cleanup.h currently. This means that so far, patches adopting this are
> generally converting something that wasn't scoped to be scoped. This
> results in changing the indentation of a lot of lines of code, which is
> just noise in the patches.
> 
> To avoid these issues, the natural thing to do would be to have a
> non-scoped version of the scoped_cond_guard() macro. There was was a
> rejected attempt to do this in [1], where one of the complaints was:
> 
> > > -       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.
> 
> [1]: https://lore.kernel.org/all/[e-mail]/
> 
> I couldn't find a way to make a cond_guard() macro that would work like
> exactly as suggested (the problem is that you can't declare a variable
> in the condition expression of an if statement in C). So I am proposing
> a macro that reads basically the same as the above so it still reads
> almost like normal C code even though it hides the if statement a bit.
> 
>         if_not_cond_guard(rwsem_read_intr, &cxl_region_rwsem)
>                 return -EINTR;
> 
> The "not" is baked into the macro because in most cases, failing to
> obtain the lock is the abnormal condition and generally we want to have
> the abnormal path be the indented one.

I think you could also take the "cond" out of the name because that is
implied by the fact it's an 'if'.

So, calling this "if_not_guard ()", for the series, you can add:

Reviewed-by: Dan Williams <[e-mail]>

...but it's ultimately up to Peter and Linus if they find this "if ()"
rename acceptable. If it is I would suggest the style should be treat it
as an "if ()" statement and add this to .clang-format:

diff --git a/.clang-format b/.clang-format
index 252820d9c80a..ae3511a69896 100644
--- a/.clang-format
+++ b/.clang-format
@@ -63,6 +63,8 @@ DerivePointerAlignment: false
 DisableFormat: false
 ExperimentalAutoDetectBinPacking: false
 FixNamespaceComments: false
+IfMacros:
+  - 'if_not_guard'
 
 # Taken from:
 #   git grep -h '^#define [^[:space:]]*for_each[^[:space:]]*(' include/ tools/ \


Last note, while the iio conversion looks correct to me, I would feel
more comfortable if there was a way to have the compiler catch that
plain "guard(iio_claim_direct)" usage is broken. Perhaps declaring
iio_device_claim_direct_mode() as __must_check achieves that effect?

Jonathan Cameron Oct. 6, 2024, 11:35 a.m. UTC | #2

On Tue, 1 Oct 2024 19:13:01 -0700
Dan Williams <[e-mail]> wrote:

> David Lechner wrote:
> > So far, I have not found scoped_cond_guard() to be nice to work with.
> > We have been using it quite a bit in the IIO subsystem via the
> > iio_device_claim_direct_scoped() macro.
> > 
> > The main thing I don't like is that scoped_cond_guard() uses a for loop
> > internally. In the IIO subsystem, we usually try to return as early as
> > possible, so often we are returning from all paths from withing this
> > hidden for loop. However, since it is a for loop, the compiler thinks
> > that it possible to exit the for loop and so we end up having to use
> > unreachable() after the end of the scope to avoid a compiler warning.
> > This is illustrated in the ad7380 patch in this series and there are 36
> > more instance of unreachable() already introduced in the IIO subsystem
> > because of this.
> > 
> > Also, scoped_cond_guard() is they only macro for conditional guards in
> > cleanup.h currently. This means that so far, patches adopting this are
> > generally converting something that wasn't scoped to be scoped. This
> > results in changing the indentation of a lot of lines of code, which is
> > just noise in the patches.
> > 
> > To avoid these issues, the natural thing to do would be to have a
> > non-scoped version of the scoped_cond_guard() macro. There was was a
> > rejected attempt to do this in [1], where one of the complaints was:
> >   
> > > > -       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.  
> > 
> > [1]: https://lore.kernel.org/all/[e-mail]/
> > 
> > I couldn't find a way to make a cond_guard() macro that would work like
> > exactly as suggested (the problem is that you can't declare a variable
> > in the condition expression of an if statement in C). So I am proposing
> > a macro that reads basically the same as the above so it still reads
> > almost like normal C code even though it hides the if statement a bit.
> > 
> >         if_not_cond_guard(rwsem_read_intr, &cxl_region_rwsem)
> >                 return -EINTR;
> > 
> > The "not" is baked into the macro because in most cases, failing to
> > obtain the lock is the abnormal condition and generally we want to have
> > the abnormal path be the indented one.  
> 
> I think you could also take the "cond" out of the name because that is
> implied by the fact it's an 'if'.
> 
> So, calling this "if_not_guard ()", for the series, you can add:
> 
> Reviewed-by: Dan Williams <[e-mail]>
> 
> ...but it's ultimately up to Peter and Linus if they find this "if ()"
> rename acceptable. 

This is a nice improvement to my eyes anyway and I hope will be fine
with Linus and Peter.  Whilst I like the cond guard stuff for the
simplifications it has brought in the IIO code, it is clunky in
some cases as you've pointed out.

Thanks for driving this forwards.


> If it is I would suggest the style should be treat it
> as an "if ()" statement and add this to .clang-format:
> 
> diff --git a/.clang-format b/.clang-format
> index 252820d9c80a..ae3511a69896 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -63,6 +63,8 @@ DerivePointerAlignment: false
>  DisableFormat: false
>  ExperimentalAutoDetectBinPacking: false
>  FixNamespaceComments: false
> +IfMacros:
> +  - 'if_not_guard'
>  
>  # Taken from:
>  #   git grep -h '^#define [^[:space:]]*for_each[^[:space:]]*(' include/ tools/ \
> 
> 
> Last note, while the iio conversion looks correct to me, I would feel
> more comfortable if there was a way to have the compiler catch that
> plain "guard(iio_claim_direct)" usage is broken. Perhaps declaring
> iio_device_claim_direct_mode() as __must_check achieves that effect?

That would be good to catch. We've not had many missuses but there
have been one or two that have shown up in review.

Jonathan