Received-SPF: pass (google.com: domain of linux-kernel+bounces-69272-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 16 Feb 2024 20:01:51 +0000
In-Reply-To: <CAGsJ_4zSDP_A32jUPrYPwZ=uwU4o0a6x-GW2HO3vu8yUh0qYjA@mail.gmail.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240216030539.110404-1-21cnbao@gmail.com> <Zc8bjZFZAZneObQG@google.com>
 <b717ae0c-ed4f-4ebc-90cf-51a8da0a6583@bytedance.com> <CAGsJ_4zSDP_A32jUPrYPwZ=uwU4o0a6x-GW2HO3vu8yUh0qYjA@mail.gmail.com>
Message-ID: <Zc-_L70_b6Dw1BQa@google.com>
Subject: Re: [PATCH] mm: zswap: increase reject_compress_poor but not
 reject_compress_fail if compression returns ENOSPC
From: Yosry Ahmed <yosryahmed@google.com>
To: Barry Song <21cnbao@gmail.com>
Cc: Chengming Zhou <zhouchengming@bytedance.com>, akpm@linux-foundation.org, 
	hannes@cmpxchg.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, 
	Barry Song <v-songbaohua@oppo.com>, Nhat Pham <nphamcs@gmail.com>, 
	Sergey Senozhatsky <senozhatsky@chromium.org>
Content-Type: text/plain; charset="us-ascii"

> > >> diff --git a/mm/zswap.c b/mm/zswap.c
> > >> index 6319d2281020..9a21dbe8c056 100644
> > >> --- a/mm/zswap.c
> > >> +++ b/mm/zswap.c
> > >> @@ -1627,7 +1627,10 @@ bool zswap_store(struct folio *folio)
> > >>      dlen = acomp_ctx->req->dlen;
> > >>
> > >>      if (ret) {
> > >> -            zswap_reject_compress_fail++;
> > >> +            if (ret == -ENOSPC)
> > >> +                    zswap_reject_compress_poor++;
> > >> +            else
> > >> +                    zswap_reject_compress_fail++;
> > >
> > > With this diff, we have four locations in zswap_store() where we
> > > increment zswap_reject_compress_{poor/fail}.
> > >
> > > How about the following instead?A
> > >
> > > diff --git a/mm/zswap.c b/mm/zswap.c
> > > index 62fe307521c93..3a7e8ba7f6116 100644
> > > --- a/mm/zswap.c
> > > +++ b/mm/zswap.c
> > > @@ -1059,24 +1059,16 @@ static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
> > >        */
> > >       ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
> > >       dlen = acomp_ctx->req->dlen;
> > > -     if (ret) {
> > > -             zswap_reject_compress_fail++;
> > > +     if (ret)
> > >               goto unlock;
> > > -     }
> > >
> > >       zpool = zswap_find_zpool(entry);
> > >       gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
> > >       if (zpool_malloc_support_movable(zpool))
> > >               gfp |= __GFP_HIGHMEM | __GFP_MOVABLE;
> > >       ret = zpool_malloc(zpool, dlen, gfp, &handle);
> > > -     if (ret == -ENOSPC) {
> > > -             zswap_reject_compress_poor++;
> > > -             goto unlock;
> > > -     }
> > > -     if (ret) {
> > > -             zswap_reject_alloc_fail++;
> > > +     if (ret)
> > >               goto unlock;
> > > -     }
> > >
> > >       buf = zpool_map_handle(zpool, handle, ZPOOL_MM_WO);
> > >       memcpy(buf, dst, dlen);
> > > @@ -1086,6 +1078,10 @@ static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
> > >       entry->length = dlen;
> > >
> > >  unlock:
> > > +     if (ret == -ENOSPC)
> > > +             zswap_reject_compress_poor++;
> > > +     else if (ret)
> > > +             zswap_reject_alloc_fail++;
> >
> > Here have two cases: zswap_reject_compress_fail, zswap_reject_alloc_fail.

Ah brain fart, sorry.

> 
> Is it safe to differentiate these two cases by checking ret == -ENOMEM ?
> otherwise, it seems the original patch still makes more sense?

I don't think it is in all cases, some allocators return other error
codes. It seems unlikely that we'll get any of them, but it can be
missed in the future. How about we use different return codes to
differentiate failures, and still centralize the counters handling.

Something like the following (ideally that one is not a brain fart):

diff --git a/mm/zswap.c b/mm/zswap.c
index 62fe307521c93..20ba25b7601a7 100644
--- a/mm/zswap.c
+++ b/mm/zswap.c
@@ -1021,12 +1021,12 @@ static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
 {
 	struct crypto_acomp_ctx *acomp_ctx;
 	struct scatterlist input, output;
+	int comp_ret = 0, alloc_ret = 0;
 	unsigned int dlen = PAGE_SIZE;
 	unsigned long handle;
 	struct zpool *zpool;
 	char *buf;
 	gfp_t gfp;
-	int ret;
 	u8 *dst;

 	acomp_ctx = raw_cpu_ptr(entry->pool->acomp_ctx);
@@ -1057,26 +1057,18 @@ static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
 	 * but in different threads running on different cpu, we have different
 	 * acomp instance, so multiple threads can do (de)compression in parallel.
 	 */
-	ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
+	comp_ret = crypto_wait_req(crypto_acomp_compress(acomp_ctx->req), &acomp_ctx->wait);
 	dlen = acomp_ctx->req->dlen;
-	if (ret) {
-		zswap_reject_compress_fail++;
+	if (comp_ret)
 		goto unlock;
-	}

 	zpool = zswap_find_zpool(entry);
 	gfp = __GFP_NORETRY | __GFP_NOWARN | __GFP_KSWAPD_RECLAIM;
 	if (zpool_malloc_support_movable(zpool))
 		gfp |= __GFP_HIGHMEM | __GFP_MOVABLE;
-	ret = zpool_malloc(zpool, dlen, gfp, &handle);
-	if (ret == -ENOSPC) {
-		zswap_reject_compress_poor++;
-		goto unlock;
-	}
-	if (ret) {
-		zswap_reject_alloc_fail++;
+	alloc_ret = zpool_malloc(zpool, dlen, gfp, &handle);
+	if (alloc_ret)
 		goto unlock;
-	}

 	buf = zpool_map_handle(zpool, handle, ZPOOL_MM_WO);
 	memcpy(buf, dst, dlen);
@@ -1086,6 +1078,13 @@ static bool zswap_compress(struct folio *folio, struct zswap_entry *entry)
 	entry->length = dlen;

 unlock:
+	if (comp_ret == -ENOSPC || alloc_ret == -ENOSPC)
+		zswap_reject_compress_poor++;
+	else if (comp_ret)
+		zswap_reject_compress_fail++;
+	else if (alloc_ret)
+		zswap_reject_alloc_fail++;
+
 	mutex_unlock(&acomp_ctx->mutex);
 	return ret == 0;
 }

> 
> >
> > >       mutex_unlock(&acomp_ctx->mutex);
> > >       return ret == 0;
> > >  }
> > >
> > >>              goto put_dstmem;
> > >>      }
> > >>
> > >> --
> > >> 2.34.1
> > >>
> 
> Thanks
> Barry