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.
