On Mon, Sep 11, 2023 at 12:17:18AM +0100, Matthew Wilcox wrote:
> On Mon, Sep 11, 2023 at 08:56:45AM +1000, Dave Chinner wrote:
> > On Fri, Sep 08, 2023 at 12:44:34PM +0200, Peter Zijlstra wrote:
> > > Agreed, and this is fine. However there's been some very creative
> > > 'use' of the _is_locked() class of functions in the past that did not
> > > follow 'common' sense.
> > >
> > > If all usage was: I should be holding this, lets check. I probably
> > > wouldn't have this bad feeling about things.
> >
> > So your argument against such an interface is essentially "we can't
> > have nice things because someone might abuse them"?
>
> Some people are very creative ...
Sure, but that's no reason to stop anyone else from making progress.
> I was thinking about how to handle this better. We could have
>
> static inline void rwsem_assert_locked(const struct rw_semaphore *sem)
> {
> BUG_ON(atomic_long_read(&sem->count) == 0);
> }
>
> static inline void rwsem_assert_write_locked(const struct rw_semaphore *sem)
> {
> BUG_ON((atomic_long_read(&sem->count) & 1) != 1);
> }
We already have CONFIG_DEBUG_RWSEMS, so we can put these
introspection interfaces inside debug code, and make any attempt to
use them outside of debug builds break the build. e.g:
#if DEBUG_RWSEMS
/*
* rwsem locked checks can only be used by conditionally compiled
* subsystem debug code. It is not valid to use them in normal
* production code.
*/
static inline bool rwsem_is_write_locked()
{
....
}
static inline bool rwsem_is_locked()
{
....
}
#else /* !DEBUG_RWSEMS */
#define rwsem_is_write_locked() BUILD_BUG()
#define rwsem_is_locked() BUILD_BUG()
#endif /* DEBUG_RWSEMS */
And now we simply add a single line to subsystem Kconfig debug
options to turn on rwsem introspection for their debug checks like
so:
config XFS_DEBUG
bool "XFS Debugging support"
depends on XFS_FS
+ select RWSEM_DEBUG
help
Say Y here to get an XFS build with many debugging features,
including ASSERT checks, function wrappers around macros,
> but then we'd also need to change how XFS currently uses the ASSERT()
> macro to be ASSERT_LOCKED(ip, flags), and I understand this code is also
> used in userspace, so it'd involve changing that shim, and we're getting
> way past the amount of code I'm comfortable changing, and way past the
> amount of time I should be spending on this.
>
> And then there'd be the inevitable bikeshedding about "don't use BUG_ON"
> and it's probably just for the best if I walk away at this point,
> becoming the third person to fail to remove the mrlock.
Yeah, which further points out how ridiculous the situation is. This
is useful debug code and it can *obviously* and *easily* be
constrained to debug environments.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
