linux-kernel - Re: [PATCH] mm: zswap: increase reject_compress_poor but not reject_compress

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Fri, 16 Feb 2024 20:01:51 +0000
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Barry Song <21cnbao@...il.com>
Cc: Chengming Zhou <zhouchengming@...edance.com>, akpm@...ux-foundation.org, 
	hannes@...xchg.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	Barry Song <v-songbaohua@...o.com>, Nhat Pham <nphamcs@...il.com>, 
	Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCH] mm: zswap: increase reject_compress_poor but not
 reject_compress_fail if compression returns ENOSPC

> > >> 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