Re: [External] Re: [PATCH] mm: zswap: fix the lack of page lru flag in zswap_writeback_entry

[Nhat Pham]

On Sun, Jan 7, 2024 at 1:29 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
>
> On Fri, Jan 5, 2024 at 6:10 AM Zhongkun He <hezhongkun.hzk@xxxxxxxxxxxxx> wrote:
> >
> > > > There is another option here, which is not to move the page to the
> > > > tail of the inactive
> > > > list after end_writeback and delete the following code in
> > > > zswap_writeback_entry(),
> > > > which did not work properly. But the pages will not be released first.
> > > >
> > > > /* move it to the tail of the inactive list after end_writeback */
> > > > SetPageReclaim(page);
>
>
> Ok, so I took a look at the patch that originally introduced this
> piece of logic:
>
> https://github.com/torvalds/linux/commit/b349acc76b7f65400b85abd09a5379ddd6fa5a97
>
> Looks like it's not for the sake of correctness, but only as a
> best-effort optimization (reducing page scanning). If it doesn't bring
> any benefit (i.e due to the newly allocated page still on the cpu
> batch), then we can consider removing it. After all, if you're right
> and it's not really doing anything here - why bother. Perhaps we can
> replace this with some other mechanism to avoid it being scanned for
> reclaim.

For instance, we can grab the local lock, look for the folio in the
add batch and take the folio off it, then add it to the rotate batch
instead? Not sure if this is doable within folio_rotate_reclaimable(),
or you'll have to manually perform this yourself (and remove the
PG_reclaim flag set here so that folio_end_writeback() doesn't try to
handle it).

There is still some overhead with this, but at least we don't have to
*drain everything* (which looks like what's lru_add_drain() ->
lru_add_drain_cpu() is doing). The latter sounds expensive and
unnecessary, whereas this is just one element addition and one element
removal - and if IIUC the size of the per-cpu add batch is capped at
15, so lookup + removal (if possible) shouldn't be too expensive?

Just throwing ideas out there :)

>
> I would cc Weijie as well, as he is the original author of this.