diff mbox series

[1/5] maple_tree: Allow external locks to be configured with their map

Message ID 20240822-b4-regmap-maple-nolock-v1-1-d5e6dbae3396@kernel.org (mailing list archive)
State New
Headers show
Series regmap: Improve lock handling with maple tree | expand

Commit Message

Mark Brown Aug. 22, 2024, 7:13 p.m. UTC
Currently the maple tree code allows external locks to be configured by
passing the lock itself. This is generally helpful and convenient but is
not ideal for situations like the regmap maple tree cache where we support
configurable locking at the regmap level and don't have the lock type when
we are configuring the maple tree. Add a helper that allows us to pass the
dep map directly to help with these situations. Since such code is already
peering at the lockdep internals enough to be looking at the map no stub
is provided.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/linux/maple_tree.h | 3 +++
 1 file changed, 3 insertions(+)

Comments

Matthew Wilcox Aug. 22, 2024, 7:21 p.m. UTC | #1
On Thu, Aug 22, 2024 at 08:13:35PM +0100, Mark Brown wrote:
> Currently the maple tree code allows external locks to be configured by
> passing the lock itself. This is generally helpful and convenient but is

No, it's a really bad idea.  Stop doing it.  Use the internal lock.
It's a temporary hack we put in and I'm really regretting allowing it.

> not ideal for situations like the regmap maple tree cache where we support
> configurable locking at the regmap level and don't have the lock type when
> we are configuring the maple tree. Add a helper that allows us to pass the
> dep map directly to help with these situations. Since such code is already
> peering at the lockdep internals enough to be looking at the map no stub
> is provided.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  include/linux/maple_tree.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/maple_tree.h b/include/linux/maple_tree.h
> index a53ad4dabd7e..bdc6b133abdc 100644
> --- a/include/linux/maple_tree.h
> +++ b/include/linux/maple_tree.h
> @@ -193,6 +193,9 @@ typedef struct lockdep_map *lockdep_map_p;
>  #define mt_set_external_lock(mt, lock)					\
>  	(mt)->ma_external_lock = &(lock)->dep_map
>  
> +#define mt_set_external_lock_dep_map(mt, dep_map)			\
> +	(mt)->ma_external_lock = dep_map
> +
>  #define mt_on_stack(mt)			(mt).ma_external_lock = NULL
>  #else
>  typedef struct { /* nothing */ } lockdep_map_p;
> 
> -- 
> 2.39.2
> 
> 
> -- 
> maple-tree mailing list
> maple-tree@lists.infradead.org
> https://lists.infradead.org/mailman/listinfo/maple-tree
Mark Brown Aug. 22, 2024, 7:48 p.m. UTC | #2
On Thu, Aug 22, 2024 at 08:21:40PM +0100, Matthew Wilcox wrote:
> On Thu, Aug 22, 2024 at 08:13:35PM +0100, Mark Brown wrote:

> > Currently the maple tree code allows external locks to be configured by
> > passing the lock itself. This is generally helpful and convenient but is

> No, it's a really bad idea.  Stop doing it.  Use the internal lock.
> It's a temporary hack we put in and I'm really regretting allowing it.

I mean, we do use the internal lock here since otherwise lockdep moans
but it's pure overhead which just complicates the code.  It's only ever
taken within another lock, meaning it winds up protecting nothing for
these maple trees.  We can't go the other way round and use the maple
tree lock as the regmap lock since apart from anything else it's a spin
lock and we need to use a mutex most of the time to support busses that
sleep during I/O.
Matthew Wilcox Aug. 22, 2024, 7:55 p.m. UTC | #3
On Thu, Aug 22, 2024 at 08:48:56PM +0100, Mark Brown wrote:
> On Thu, Aug 22, 2024 at 08:21:40PM +0100, Matthew Wilcox wrote:
> > On Thu, Aug 22, 2024 at 08:13:35PM +0100, Mark Brown wrote:
> 
> > > Currently the maple tree code allows external locks to be configured by
> > > passing the lock itself. This is generally helpful and convenient but is
> 
> > No, it's a really bad idea.  Stop doing it.  Use the internal lock.
> > It's a temporary hack we put in and I'm really regretting allowing it.
> 
> I mean, we do use the internal lock here since otherwise lockdep moans
> but it's pure overhead which just complicates the code.  It's only ever
> taken within another lock, meaning it winds up protecting nothing for
> these maple trees.  We can't go the other way round and use the maple
> tree lock as the regmap lock since apart from anything else it's a spin
> lock and we need to use a mutex most of the time to support busses that
> sleep during I/O.

When it's an uncontended spinlock, there's really no overhead.  I wish I'd
been firmer on that point earlier and prohibited the external lock hack.

The point is that the lock protects the tree.  If we are ever going to
be able to defragment slabs (and I believe this is an ability that Linux
must gain), we must be able to go from the object (the maple node) to
a lock that will let us reallocate the node.  If there's some external
lock that protects the tree, we can't possibly do that.
Mark Brown Aug. 22, 2024, 8:45 p.m. UTC | #4
On Thu, Aug 22, 2024 at 08:55:20PM +0100, Matthew Wilcox wrote:
> On Thu, Aug 22, 2024 at 08:48:56PM +0100, Mark Brown wrote:

> > I mean, we do use the internal lock here since otherwise lockdep moans
> > but it's pure overhead which just complicates the code.  It's only ever

> When it's an uncontended spinlock, there's really no overhead.  I wish I'd
> been firmer on that point earlier and prohibited the external lock hack.

> The point is that the lock protects the tree.  If we are ever going to
> be able to defragment slabs (and I believe this is an ability that Linux
> must gain), we must be able to go from the object (the maple node) to
> a lock that will let us reallocate the node.  If there's some external
> lock that protects the tree, we can't possibly do that.

If the external lock guarantees that nothing can possibly be contending
access to the tree (including the read side) I don't see any issue
there?
diff mbox series

Patch

diff --git a/include/linux/maple_tree.h b/include/linux/maple_tree.h
index a53ad4dabd7e..bdc6b133abdc 100644
--- a/include/linux/maple_tree.h
+++ b/include/linux/maple_tree.h
@@ -193,6 +193,9 @@  typedef struct lockdep_map *lockdep_map_p;
 #define mt_set_external_lock(mt, lock)					\
 	(mt)->ma_external_lock = &(lock)->dep_map
 
+#define mt_set_external_lock_dep_map(mt, dep_map)			\
+	(mt)->ma_external_lock = dep_map
+
 #define mt_on_stack(mt)			(mt).ma_external_lock = NULL
 #else
 typedef struct { /* nothing */ } lockdep_map_p;