Message-ID: <c9c5bea0-7542-4587-9d02-0dc25574c548@redhat.com>
Date: Mon, 4 Nov 2024 18:10:49 +0100
From: David Hildenbrand <david@...hat.com>
To: Johannes Weiner <hannes@...xchg.org>
Cc: Barry Song <21cnbao@...il.com>, akpm@...ux-foundation.org,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 Barry Song <v-songbaohua@...o.com>, Usama Arif <usamaarif642@...il.com>,
 Chengming Zhou <chengming.zhou@...ux.dev>,
 Yosry Ahmed <yosryahmed@...gle.com>, Nhat Pham <nphamcs@...il.com>,
 Hugh Dickins <hughd@...gle.com>, Matthew Wilcox <willy@...radead.org>,
 Shakeel Butt <shakeel.butt@...ux.dev>, Andi Kleen <ak@...ux.intel.com>,
 Baolin Wang <baolin.wang@...ux.alibaba.com>, Chris Li <chrisl@...nel.org>,
 "Huang, Ying" <ying.huang@...el.com>, Kairui Song <kasong@...cent.com>,
 Ryan Roberts <ryan.roberts@....com>
Subject: Re: [PATCH v2] mm: count zeromap read and set for swapout and swapin

On 04.11.24 17:34, Johannes Weiner wrote:
> On Mon, Nov 04, 2024 at 01:42:08PM +0100, David Hildenbrand wrote:
>> On 02.11.24 11:12, Barry Song wrote:
>>> @@ -1599,6 +1599,16 @@ The following nested keys are defined.
>>>    	  pglazyfreed (npn)
>>>    		Amount of reclaimed lazyfree pages
>>>    
>>> +	  swpin_zero
>>> +		Number of pages moved into memory with zero content, meaning no
>>> +		copy exists in the backend swapfile, allowing swap-in to avoid
>>> +		I/O read overhead.
>>> +
>>> +	  swpout_zero
>>> +		Number of pages moved out of memory with zero content, meaning no
>>> +		copy is needed in the backend swapfile, allowing swap-out to avoid
>>> +		I/O write overhead.
>>
>> Hm, can make it a bit clearer that this is a pure optimization and refer
>> to the other counters?
>>
>> swpin_zero
>> 	Portion of "pswpin" pages for which I/O was optimized out
>> 	because the page content was detected to be zero during swapout.
> 
> AFAICS the zeropages currently don't show up in pswpin/pswpout, so
> these are independent counters, not subsets.

Ah. now I understand the problem. The whole "move out of memory" "move 
into memory" here is quite confusing TBH. We're not moving anything, 
we're optimizing out the move completely ... yes, you could call it 
compression (below).

> 
> I'm leaning towards Barry's side on the fixes tag.

I think the documentation when to use the Fixes: tag is pretty clear.

Introducing new counters can hardly be considered a bugfix. Missing to 
adjust some counters that *existing tools* would know/use might be  IMO 
(below).

>  When zswap handled
> the same-filled pages, we would count them in zswpin/out. From a user
> POV, especially one using zswap, the behavior didn't change, but the
> counts giving insight into this (potentially significant) VM activity
> disappeared. This is arguably a regression.
 > >> swpout_zero
>> 	Portion of "pswout" pages for which I/O was optimized out
>> 	because the page content was detected to be zero.
> 
> Are we sure we want to commit to the "zero" in the name here? Until
> very recently, zswap optimized all same-filled pages. It's possible
> somebody might want to bring that back down the line.

Agreed.

> 
> In reference to the above, I'd actually prefer putting them back into
> zswpin/zswpout. Sure, they're not handled by zswap.c proper, but this
> is arguably just an implementation detail; from a user POV this is
> still just (a form of) compression in lieu of IO to the swap backend.
> 
> IMO there is no need for coming up with a separate category. Just add
> them to zswpin/zswpout and remove the CONFIG_ZSWAP guards from them?

Would work for me, although it might be a bit confusing if no zswap is 
configured. Maybe just needs to be documented.

-- 
Cheers,

David / dhildenb

Powered by blists - more mailing lists