> -----Original Message----- > From: Johannes Weiner <hannes@xxxxxxxxxxx> > Sent: Wednesday, September 25, 2024 6:54 AM > To: Sridhar, Kanchana P <kanchana.p.sridhar@xxxxxxxxx> > Cc: Yosry Ahmed <yosryahmed@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; > linux-mm@xxxxxxxxx; nphamcs@xxxxxxxxx; chengming.zhou@xxxxxxxxx; > usamaarif642@xxxxxxxxx; shakeel.butt@xxxxxxxxx; ryan.roberts@xxxxxxx; > Huang, Ying <ying.huang@xxxxxxxxx>; 21cnbao@xxxxxxxxx; akpm@linux- > foundation.org; Zou, Nanhai <nanhai.zou@xxxxxxxxx>; Feghali, Wajdi K > <wajdi.k.feghali@xxxxxxxxx>; Gopal, Vinodh <vinodh.gopal@xxxxxxxxx> > Subject: Re: [PATCH v7 5/8] mm: zswap: Compress and store a specific page > in a folio. > > On Wed, Sep 25, 2024 at 01:49:03AM +0000, Sridhar, Kanchana P wrote: > > > From: Yosry Ahmed <yosryahmed@xxxxxxxxxx> > > > I think it's more correct and efficient to update the atomic once > > > after all the pages are successfully compressed and stored. > > > > Actually this would need to co-relate with the limits checking strategy, > > because the atomic is used there and needs to be as accurate as possible. > > For the limit checks, we use the zpool counters, not zswap_stored_pages. Thanks Johannes for your insights and comments. Yes, you are absolutely right. My apologies. > > zswap_stored_pages is used in the zswap shrinker to guesstimate > pressure, so it's likely a good thing to only count entries that are > expected to stay, and not account the ones that might fail just yet. Sure, makes sense. > > > As far as the vmstat 'zswpout', the reason I left it as-is in my patchset > > was to be more indicative of the actual zswpout compute events that > > occurred (for things like getting the compressions count), regardless > > of whether or not the overall mTHP store was successful. If this vmstat > > needs to reflect only successful zswpout events (i.e., represent the zswap > > usage), I can fix it by updating it once only if the mTHP is stored successfully. > > Yeah, that's fine as well. > > I would suggest batching them both at the end of zswap_store(). Ok, will do so in v8. Thanks, Kanchana
