diff mbox series

regcache: Push async I/O request down into the rbtree cache

Message ID 20230719-regcache-async-rbtree-v1-1-b03d30cf1daf@kernel.org
State New
Headers
Series regcache: Push async I/O request down into the rbtree cache |

Commit Message

Mark Brown July 18, 2023, 11:30 p.m. UTC 
  Currently the regcache core unconditionally enables async I/O for all cache
types, causing problems for the maple tree cache which dynamically allocates
the buffers used to write registers to the device since async requires the
buffers to be kept around until the I/O has been completed.

This use of async I/O is mainly for the rbtree cache which stores data in
a format directly usable for regmap_raw_write(), though there is a special
case for single register writes which would also have allowed it to be used
with the flat cache. It is a bit of a landmine for other caches since it
implicitly converts sync operations to async, and with modern hardware it
is not clear that async I/O is actually a performance win as shown by the
performance work David Jander did with SPI. In multi core systems the cost
of managing concurrency ends up swamping the performance benefit and almost
all modern systems are multi core.

Address this by pushing the enablement of async I/O down into the rbtree
cache where it is actively used, avoiding surprises for other cache
implementations.

Reported-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Fixes: bfa0b38c1483 ("regmap: maple: Implement block sync for the maple tree cache")
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regcache-rbtree.c | 4 ++++
 drivers/base/regmap/regcache.c        | 3 ---
 2 files changed, 4 insertions(+), 3 deletions(-)


---
base-commit: fdf0eaf11452d72945af31804e2a1048ee1b574c
change-id: 20230718-regcache-async-rbtree-dcce23303176

Best regards,

Comments

Charles Keepax July 19, 2023, 8:40 a.m. UTC | #1 
On Wed, Jul 19, 2023 at 12:30:40AM +0100, Mark Brown wrote:
> Currently the regcache core unconditionally enables async I/O for all cache
> types, causing problems for the maple tree cache which dynamically allocates
> the buffers used to write registers to the device since async requires the
> buffers to be kept around until the I/O has been completed.
> 
> This use of async I/O is mainly for the rbtree cache which stores data in
> a format directly usable for regmap_raw_write(), though there is a special
> case for single register writes which would also have allowed it to be used
> with the flat cache. It is a bit of a landmine for other caches since it
> implicitly converts sync operations to async, and with modern hardware it
> is not clear that async I/O is actually a performance win as shown by the
> performance work David Jander did with SPI. In multi core systems the cost
> of managing concurrency ends up swamping the performance benefit and almost
> all modern systems are multi core.
> 
> Address this by pushing the enablement of async I/O down into the rbtree
> cache where it is actively used, avoiding surprises for other cache
> implementations.
> 
> Reported-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> Fixes: bfa0b38c1483 ("regmap: maple: Implement block sync for the maple tree cache")
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Tested-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles
Mark Brown July 21, 2023, 5:41 p.m. UTC | #2 
On Wed, 19 Jul 2023 00:30:40 +0100, Mark Brown wrote:
> Currently the regcache core unconditionally enables async I/O for all cache
> types, causing problems for the maple tree cache which dynamically allocates
> the buffers used to write registers to the device since async requires the
> buffers to be kept around until the I/O has been completed.
> 
> This use of async I/O is mainly for the rbtree cache which stores data in
> a format directly usable for regmap_raw_write(), though there is a special
> case for single register writes which would also have allowed it to be used
> with the flat cache. It is a bit of a landmine for other caches since it
> implicitly converts sync operations to async, and with modern hardware it
> is not clear that async I/O is actually a performance win as shown by the
> performance work David Jander did with SPI. In multi core systems the cost
> of managing concurrency ends up swamping the performance benefit and almost
> all modern systems are multi core.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Thanks!

[1/1] regcache: Push async I/O request down into the rbtree cache
      commit: b460a52257b1f5299ca70b7d1bb32442d3ce7bf6

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark
diff mbox series

Patch

diff --git a/drivers/base/regmap/regcache-rbtree.c b/drivers/base/regmap/regcache-rbtree.c
index fabf87058d80..584bcc55f56e 100644
--- a/drivers/base/regmap/regcache-rbtree.c
+++ b/drivers/base/regmap/regcache-rbtree.c
@@ -471,6 +471,8 @@  static int regcache_rbtree_sync(struct regmap *map, unsigned int min,
 	unsigned int start, end;
 	int ret;
 
+	map->async = true;
+
 	rbtree_ctx = map->cache;
 	for (node = rb_first(&rbtree_ctx->root); node; node = rb_next(node)) {
 		rbnode = rb_entry(node, struct regcache_rbtree_node, node);
@@ -499,6 +501,8 @@  static int regcache_rbtree_sync(struct regmap *map, unsigned int min,
 			return ret;
 	}
 
+	map->async = false;
+
 	return regmap_async_complete(map);
 }
 
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 28bc3ae9458a..7d3e47436056 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -368,8 +368,6 @@  int regcache_sync(struct regmap *map)
 	if (!map->cache_dirty)
 		goto out;
 
-	map->async = true;
-
 	/* Apply any patch first */
 	map->cache_bypass = true;
 	for (i = 0; i < map->patch_regs; i++) {
@@ -392,7 +390,6 @@  int regcache_sync(struct regmap *map)
 
 out:
 	/* Restore the bypass state */
-	map->async = false;
 	map->cache_bypass = bypass;
 	map->no_sync_defaults = false;
 	map->unlock(map->lock_arg);