Received-SPF: pass (google.com: domain of linux-kernel+bounces-110807-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240320020823.337644-1-yosryahmed@google.com>
 <20240320020823.337644-2-yosryahmed@google.com> <CAJD7tkYb-=Ho85e2AJbkfe-FmT6KXpJpUgPRaXQb5-+sY5j4Hg@mail.gmail.com>
 <CAKEwX=PjpJCewYEMp+=g+92=R-Wr2y+7pwJ81xb9-pogohFC4w@mail.gmail.com>
In-Reply-To: <CAKEwX=PjpJCewYEMp+=g+92=R-Wr2y+7pwJ81xb9-pogohFC4w@mail.gmail.com>
From: Yosry Ahmed <yosryahmed@google.com>
Date: Thu, 21 Mar 2024 16:57:27 -0700
Message-ID: <CAJD7tkaC5eTe8Os6f0VsGx9o06YE8zX1r0R0PPMvExRfcGhPgg@mail.gmail.com>
Subject: Re: [PATCH 2/2] mm: zswap: remove nr_zswap_stored atomic
To: Nhat Pham <nphamcs@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>, Johannes Weiner <hannes@cmpxchg.org>, 
	Chengming Zhou <chengming.zhou@linux.dev>, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Mar 21, 2024 at 4:50=E2=80=AFPM Nhat Pham <nphamcs@gmail.com> wrote=
:
>
> On Thu, Mar 21, 2024 at 2:09=E2=80=AFPM Yosry Ahmed <yosryahmed@google.co=
m> wrote:
> >
> > On Tue, Mar 19, 2024 at 7:08=E2=80=AFPM Yosry Ahmed <yosryahmed@google.=
com> wrote:
> > >
> > > zswap_nr_stored is used to maintain the number of stored pages in zsw=
ap
> > > that are not same-filled pages. It is used in zswap_shrinker_count() =
to
> > > scale the number of freeable compressed pages by the compression rati=
o.
> > > That is, to reduce the amount of writeback from zswap with higher
> > > compression ratios as the ROI from IO diminishes.
> > >
> > > However, the need for this counter is questionable due to two reasons=
:
> > > - It is redundant. The value can be inferred from (zswap_stored_pages=
 -
> > >   zswap_same_filled_pages).
>
> Ah, I forgot about this. For context, nr_stored was originally a
> zswap_pool-specific stat, but I think Chengming has pulled it out and
> converted it into a global pool stat in an earlier patch - yet,
> globally, we already have zswap_stored_pages that is (mostly) the same
> counter.

Thanks for the context.

>
> Might as well use existing counters (zswap_stored_pages) then, rather
> than a newly introduced counter. Probably will shave off a couple
> cycles here and there for the atomic increment/decrement :)
>
> > > - When memcgs are enabled, we use memcg_page_state(memcg,
> > >   MEMCG_ZSWAPPED), which includes same-filled pages anyway (i.e.
> > >   equivalent to zswap_stored_pages).
>
> This is fine I suppose. I was aware of this weird inaccuracy. However,
> for the CONFIG_MEMCG case, it was kinda silly to introduce the counter
> for per-cgroup same filled zswap pages, just for this one purpose, so
> I decided to accept the inaccuracy.
>
> > >
> > > Use zswap_stored_pages instead in zswap_shrinker_count() to keep thin=
gs
> > > consistent whether memcgs are enabled or not, and add a comment about
> > > the number of freeable pages possibly being scaled down more than it
> > > should if we have lots of same-filled pages (i.e. inflated compressio=
n
> > > ratio).
> > >
> > > Remove nr_zswap_stored and one atomic operation in the store and free
> > > paths.
> > >
> > > Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> >
> > Any thoughts on this patch? Should I resend it separately?
>
> Might be worth resending it separately, but up to you and Andrew!

I will resend to add some context and include your R-b, thanks.

>
> Reviewed-by: Nhat Pham <nphamcs@gmail.com>