linux-kernel - Re: [External] Re: [PATCH] mm:zswap: fix zswap entry reclamation failure in two scenarios

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 15 Nov 2023 20:12:01 +0800
From:   贺中坤 <hezhongkun.hzk@...edance.com>
To:     Nhat Pham <nphamcs@...il.com>
Cc:     akpm@...ux-foundation.org, hannes@...xchg.org,
        yosryahmed@...gle.com, sjenning@...hat.com, ddstreet@...e.org,
        vitaly.wool@...sulko.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [External] Re: [PATCH] mm:zswap: fix zswap entry reclamation
 failure in two scenarios

>
> Ah my bad, I should have been clearer.
>
> I was looking at the zswap shrinker patch series (specifically the
> cgroup-aware LRU patch), which moves the counter update out of
> zswap_writeback_entry. If we apply that patch on top of that series, we will
> screw up the counter. Should be easily fixable anyway though.

Got it.

>
> Ah I think I understand the point of the patch a bit better now.
>
> Essentially, we're invalidating these entries, which does reclaim the
> memory used for these compressed objects, but there is no IO involved.
> Writeback-less shrinking, if you will.
>
> This will still screw up one of the heuristics I'm using for the zswap
> shrinker a bit, but that should be easily fixable with some plumbing.
> Same goes for the writeback counter - but depending on the order in
> which Andrew apply the patches, you might have to resolve the conflicts
> there :)

OK,  I will fix it.

>
> Other than this objection, I think this optimization makes sense to me:
>
> In the first case, we already freed the swap entry. Might as well also
> dropped the zswap entry.
>
> In the second case, we already have another copy in memory, so
> dropping the compressed copy to make space for warmer objects
> coming into zswap makes sense to me. Might be worth doing a
> micro-benchmark to verify this intuition, but I agree that it's more
> important to maintain the LRU ordering than any CPU saving from
> skipping re-compression.
>
> I would suggest that you should expand on this on the commit log
> to make clearer the motivation behind this optimization, if you were
> to re-submit this patch for some reason (for e.g to resolve the
> aforementioned conflicts with the zswap shrinker series).
>
> But otherwise, LGTM!
>
> Feel free to add the following tag:
> Reviewed-by: Nhat Pham <nphamcs@...il.com>

Thanks, there are still some commits from Yosry,
after that, I will send it again.