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

[Yosry Ahmed <yosryahmed@google.com>]

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.

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

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

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

Thanks!