On Sun, Sep 10, 2023 at 10:15:59PM -0400, Waiman Long wrote:
>
> On 9/10/23 20:55, Dave Chinner wrote:
> > 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,
>
> That may be a possible compromise. Actually, I am not against having them
> defined even outside the DEBUG_RWSEMS. We already have mutex_is_locked()
> defined and used in a lot of places. So this is just providing the rwsem
> equivalents.
So, once again, we have mixed messages from the lock maintainers.
One says "no, it might get abused", another says "I'm fine with
that", and now we have a maintainer disagreement stalemate.
This is dysfunctional.
Peter, Waiman, please make a decision one way or the other about
allowing rwsems ito support native write lock checking. In the
absence of an actual yes/no decision, do we assume that the
maintainers don't actually care about it and we should just
submit it straight to Linus?
-Dave.
--
Dave Chinner
[email protected]