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]
Message-ID: <CAJD7tkbPR7ZCR9nTpn3SoF6hD8A7_CuPh+SffHG8Mdo=LSU55w@mail.gmail.com>
Date: Fri, 2 Aug 2024 20:22:04 -0700
From: Yosry Ahmed <yosryahmed@...gle.com>
To: Nhat Pham <nphamcs@...il.com>
Cc: akpm@...ux-foundation.org, hannes@...xchg.org, shakeel.butt@...ux.dev, 
	linux-mm@...ck.org, kernel-team@...a.com, linux-kernel@...r.kernel.org, 
	flintglass@...il.com, chengming.zhou@...ux.dev
Subject: Re: [PATCH v2 2/2] zswap: increment swapin count for non-pivot
 swapped in pages

On Fri, Aug 2, 2024 at 4:46 PM Nhat Pham <nphamcs@...il.com> wrote:
>
> On Thu, Aug 1, 2024 at 1:02 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
> >
> >
> > Hmm, but there is a chance that these pages are not actually needed,
> > in which case we will unnecessarily increase the zswap protection.
> > Does the readahead window self-correct if the pages were not used?
>
> Hmm yeah it's kinda hard to predict if a swapped in page is strictly
> necessary in this context. We don't have this information at the time
> of the read.
>
> That said, I think erring on the side of safety is OK here - my
> understanding that readahead, while predictive in nature, only gets
> progressively more aggressive if we get signals that it's helpful (i.e
> the memory access patterns display sequential behavior).

If the readahead logic is expected to adapt in these situations (and I
think it is), then I think we are fine. Perhaps we should just leave a
comment that we may increase the protection more than we should for
those readahead cases.

>
> I think we also accept this slight inaccuracy (i.e for pages in the
> readahead window that might not necessarily be needed) the in
> workingset refault handling behavior. Could you fact check me,
> Johannes?
>
>
> >
> > > are also incrementing when the pages are read from the zswap pool, which
> > > is inaccurate.
> >
> > I feel like this is the more important part. It should be the focus of
> > the commit log with more details (i.e. why is it wrong to increment
> > the zswap protection in this case).
>
> Yeah this is pretty important too :) Maybe I should make it clearer in
> the patch commit.
>
> >
> > Do we need a Fixes and cc:stable for this one? Maybe it can be moved
> > first to make backports easy.
>
> Hmm.
>
> *Technically*, this is broken in older versions of the shrinker as
> well, but it's more of an optimization than a bug that can crash the
> kernel, so I don't know if it qualifies for a Fixes tag?
>
> Another factor is, under the old scheme, this does not move the needle
> much - at least in my benchmarks. This is because the decaying
> behavior is so aggressive that incrementing the counter in a couple
> places does not matter, when it will be rapidly divided by half later.
> This fix only shows clear improvements when applied on top of the new
> second chance scheme.
>
> I don't have a strong opinion here, but it doesn't seem worth it to
> backport IMHO :)

I thought it's a simple change worth backporting, but if it doesn't
move the needle without the second chance algorithm then it's probably
not worth it.

I would still add the "Fixes" tag because technically the logic is
wrong without this patch, it increases the zswap protection when there
swapins from zswap which doesn't make much sense.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ