Re: [PATCH v6 0/4] mm/zsmalloc: reduce lock contention in zs_free()

[Sergey Senozhatsky]

Apologies for the delay.

On (26/06/27 21:36), Andrew Morton wrote:
[..]
> > Raspberry Pi 4B (4-core ARM64 Cortex-A72):
> > 
> >   mode        Base       Patched     Speedup
> >   single      59.0ms     56.0ms      1.05x
> >   multi 2p    94.6ms     66.7ms      1.42x
> >   multi 4p    202.9ms    110.6ms     1.83x
> > 
> > x86 (20-core Intel i7-12700, 16 concurrent processes):
> > 
> >   mode        Base       Patched     Speedup
> >   single      11.7ms     9.8ms       1.19x
> >   multi 2p    24.1ms     17.2ms      1.40x
> >   multi 4p    63.0ms     45.3ms      1.39x
> 
> Well that's a nice result.

Indeed.

> Sashiko AI review said ....  nothing.  I don't recall seeing that
> before ;)

grep-ing the code, I wonder if we maybe want to tweak some comments.
Namely, zs_page_migrate() says:

	The pool migrate_lock protects the race between zpage migration
	and zs_free

which is not entirely true anymore, as zs_free() has a lockless path
that doesn't take pool look (when ZS_OBJ_CLASS_BITS permit).

Likewise, __zs_compact() says the following about pool migrate_lock:

	protect the race between zpage migration and zs_free

None of these are critical, very minor.

Reviewed-by: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
Tested-by: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>




Unrelated to the series, no need to do anything.  Just because I'm
looking at zs_page_migrate() at the moment, would something like
this make sense:

---

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 3d566a3ee96f..a6ca93ea1eb2 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1875,6 +1875,18 @@ static int zs_page_migrate(struct page *newpage, struct page *page,
 	 * the class lock protects zpage alloc/free in the zspage.
 	 */
 	spin_lock(&class->lock);
+
+	/*
+	 * Migrating an empty zspage is probably pointless as it's likely
+	 * scheduled for a destruction anyway.
+	 */
+	if (!get_zspage_inuse(zspage)) {
+		spin_unlock(&class->lock);
+		write_unlock(&pool->lock);
+		/* Same jusitification as below - we cannot return -EAGAIN. */
+		return -EBUSY;
+	}
+
 	/* the zspage write_lock protects zpage access via zs_obj_read/write() */
 	if (!zspage_write_trylock(zspage)) {
 		spin_unlock(&class->lock);