[PATCH] zsmalloc: return -EAGAIN for zspage migration lock contention

[PATCH] zsmalloc: return -EAGAIN for zspage migration lock contention

[Hui Zhu]

From: teawater <zhuhui@kylinos.cn>

movable_operations::migrate_page() should return -EAGAIN for temporary
migration failures so the migration core can retry. Other negative
errors are treated as permanent failures.

zs_page_migrate() currently returns -EINVAL when zspage_write_trylock()
fails. That path reflects transient lock contention, not invalid input.
Returning -EINVAL misclassifies the failure and can reduce migration
success under contention.

Return -EAGAIN in this path.

Fixes: e27af3f9360e ("zsmalloc: sleepable zspage reader-lock")
Signed-off-by: teawater <zhuhui@kylinos.cn>
---
 mm/zsmalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 2c1430bf8d57..1d26eda7f50c 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1727,7 +1727,7 @@ static int zs_page_migrate(struct page *newpage, struct page *page,
 	if (!zspage_write_trylock(zspage)) {
 		spin_unlock(&class->lock);
 		write_unlock(&pool->lock);
-		return -EINVAL;
+		return -EAGAIN;
 	}
 
 	/* We're committed, tell the world that this is a Zsmalloc page. */
-- 
2.43.0

Re: [PATCH] zsmalloc: return -EAGAIN for zspage migration lock contention

[Sergey Senozhatsky]

On (26/03/18 15:31), Hui Zhu wrote:
> movable_operations::migrate_page() should return -EAGAIN for temporary
> migration failures so the migration core can retry. Other negative
> errors are treated as permanent failures.
> 
> zs_page_migrate() currently returns -EINVAL when zspage_write_trylock()
> fails. That path reflects transient lock contention, not invalid input.
> Returning -EINVAL misclassifies the failure and can reduce migration
> success under contention.

The zspage's reader-lock owner can be preempted for unknown duration,
so I don't know if returning -EAGAIN is safe.  On one hand, migration
attempts are limited by NR_MAX_MIGRATE_PAGES_RETRY, on the other hand,
those attempts are performed basically immediately (?), I'm not sure
if we have good chances for preempted reader-lock owner to get scheduled
and release the lock in time.

Re: [PATCH] zsmalloc: return -EAGAIN for zspage migration lock contention

[Sergey Senozhatsky]

On (26/03/18 19:21), Sergey Senozhatsky wrote:
> On (26/03/18 15:31), Hui Zhu wrote:
> > movable_operations::migrate_page() should return -EAGAIN for temporary
> > migration failures so the migration core can retry. Other negative
> > errors are treated as permanent failures.
> > 
> > zs_page_migrate() currently returns -EINVAL when zspage_write_trylock()
> > fails. That path reflects transient lock contention, not invalid input.
> > Returning -EINVAL misclassifies the failure and can reduce migration
> > success under contention.
> 
> The zspage's reader-lock owner can be preempted for unknown duration,
> so I don't know if returning -EAGAIN is safe.  On one hand, migration
> attempts are limited by NR_MAX_MIGRATE_PAGES_RETRY, on the other hand,
> those attempts are performed basically immediately (?), I'm not sure
> if we have good chances for preempted reader-lock owner to get scheduled
> and release the lock in time.

.. or reader-lock owner can simply perform decompression with
a very slow algorithm (e.g. deflate) which can taks significantly
more time than NR_MAX_MIGRATE_PAGES_RETRY migration atteempts.

So I agree that -EINVAL is not the best error code to return here.
Not sure if -EAGAIN is the best one either.  Technically, -EBUSY can
be a candidate (this is not exactly what migration code expects and
-EBUSY is not any better than -EINVAL as it's still going to be a
terminal error).