Message-ID: <20241006123513.42b7d4ec@jic23-huawei>
Date: Sun, 6 Oct 2024 12:35:13 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Dan Williams <dan.j.williams@...el.com>
Cc: David Lechner <dlechner@...libre.com>, Peter Zijlstra
 <peterz@...radead.org>, "Linus Torvalds" <torvalds@...ux-foundation.org>,
 Nuno Sá <nuno.sa@...log.com>, Michael Hennerich
 <michael.hennerich@...log.com>, <linux-kernel@...r.kernel.org>,
 <linux-iio@...r.kernel.org>, <linux-cxl@...r.kernel.org>, Ira Weiny
 <ira.weiny@...el.com>, "Fabio M. De Francesco"
 <fabio.maria.de.francesco@...ux.intel.com>
Subject: Re: [PATCH 0/3] cleanup: add if_not_cond_guard macro

On Tue, 1 Oct 2024 19:13:01 -0700
Dan Williams <dan.j.williams@...el.com> 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/170905252721.2268463.6714121678946763402.stgit@dwillia2-xfh.jf.intel.com/
> > 
> > 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 <dan.j.williams@...el.com>
> 
> ...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

Powered by blists - more mailing lists