RE: [PATCH v6 2/3] mm: zswap: zswap_store() extended to handle mTHP folios.

["Sridhar, Kanchana P" <kanchana.p.sridhar@intel.com>]

> -----Original Message-----
> From: Yosry Ahmed <yosryahmed@google.com>
> Sent: Thursday, August 29, 2024 4:06 PM
> To: Sridhar, Kanchana P <kanchana.p.sridhar@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-mm@kvack.org;
> hannes@cmpxchg.org; nphamcs@gmail.com; chengming.zhou@linux.dev;
> usamaarif642@gmail.com; ryan.roberts@arm.com; Huang, Ying
> <ying.huang@intel.com>; 21cnbao@gmail.com; akpm@linux-foundation.org;
> Zou, Nanhai <nanhai.zou@intel.com>; Feghali, Wajdi K
> <wajdi.k.feghali@intel.com>; Gopal, Vinodh <vinodh.gopal@intel.com>
> Subject: Re: [PATCH v6 2/3] mm: zswap: zswap_store() extended to handle
> mTHP folios.
> 
> On Thu, Aug 29, 2024 at 2:27 PM Kanchana P Sridhar
> <kanchana.p.sridhar@intel.com> wrote:
> >
> 
> I think "mm: zswap: support mTHP swapout in zswap_store()" is a better
> subject. We usually use imperative tone for commit logs as much as
> possible.

Sure, this is a much better subject, thanks! I will make this change in v7.

> 
> > zswap_store() will now process and store mTHP and PMD-size THP folios.
> >
> > A new config variable CONFIG_ZSWAP_STORE_THP_DEFAULT_ON (off by
> default)
> > will enable/disable zswap storing of (m)THP.
> >
> > This change reuses and adapts the functionality in Ryan Roberts' RFC
> > patch [1]:
> >
> >   "[RFC,v1] mm: zswap: Store large folios without splitting"
> >
> >   [1] https://lore.kernel.org/linux-mm/20231019110543.3284654-1-
> ryan.roberts@arm.com/T/#u
> >
> > This patch provides a sequential implementation of storing an mTHP in
> > zswap_store() by iterating through each page in the folio to compress
> > and store it in the zswap zpool.
> >
> > Towards this goal, zswap_compress() is modified to take a page instead
> > of a folio as input.
> >
> > Each page's swap offset is stored as a separate zswap entry.
> >
> > If an error is encountered during the store of any page in the mTHP,
> > all previous pages/entries stored will be invalidated. Thus, an mTHP
> > is either entirely stored in ZSWAP, or entirely not stored in ZSWAP.
> >
> > This forms the basis for building batching of pages during zswap store
> > of large folios, by compressing batches of up to say, 8 pages in an
> > mTHP in parallel in hardware, with the Intel In-Memory Analytics
> > Accelerator (Intel IAA).
> >
> > Also, addressed some of the RFC comments from the discussion in [1].
> >
> > Made a minor edit in the comments for "struct zswap_entry" to delete
> > the comments related to "value" since same-filled page handling has
> > been removed from zswap.
> 
> This commit log is not ordered clearly. Please start by describing
> what we are doing, which is basically making zswap_store() support
> large folios by compressing them page by page. Then mention important
> implementation details and the tunabel and config options added at the
> end. After that, refer to the RFC that this is based on.

Thanks for these comments. Sure, I will incorporate in v7.

> 
> >
> > Co-developed-by: Ryan Roberts
> > Signed-off-by:
> 
> This is probably supposed to be "Signed-off-by: Ryan Roberts". I am
> not sure what the policy is for reusing patches sent earlier on the
> mailing list. Did you talk to Ryan about this?

You're right, this is intended to be "Signed-off-by: Ryan Roberts" once
Ryan has had a chance to review and indicate approval of attribution
as co-author.

I have been following the documentation guidelines for submitting
patches, as pertaining to co-development. Ryan is in the recipients list
and I am hoping he can indicate his approval for the reuse of his original
RFC.

Ryan, I would greatly appreciate your inputs on the reuse of your RFC,
and also any code review comments for improving the patchset!

> 
> > Signed-off-by: Kanchana P Sridhar <kanchana.p.sridhar@intel.com>
> 
> The diff is hard to follow because there is a lot of refactoring mixed
> in with the functional changes. Could you please break this down into
> purely refactoring patches doing the groundwork, and then the actual
> functional change patch(es) on top of them?

Sure, I will do this and submit a v7. Appreciate your comments!

Thanks,
Kanchana

> 
> Thanks!