Yosry Ahmed <yosryahmed@xxxxxxxxxx> writes: > On Sun, Nov 19, 2023 at 7:20 PM Huang, Ying <ying.huang@xxxxxxxxx> wrote: >> >> Chris Li <chriscli@xxxxxxxxxx> writes: >> >> > On Thu, Nov 16, 2023 at 12:19 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote: >> >> >> >> Not bypassing the swap slot cache, just make the callbacks to >> >> invalidate the zswap entry, do memg uncharging, etc when the slot is >> >> no longer used and is entering the swap slot cache (i.e. when >> >> free_swap_slot() is called), instead of when draining the swap slot >> >> cache (i.e. when swap_range_free() is called). For all parts of MM >> >> outside of swap, the swap entry is freed when free_swap_slot() is >> >> called. We don't free it immediately because of caching, but this >> >> should be transparent to other parts of MM (e.g. zswap, memcg, etc). >> > >> > That will cancel the batching effect on the swap slot free, making the >> > common case for swapping faults take longer to complete, righ? >> > If I recall correctly, the uncharge is the expensive part of the swap >> > slot free operation. >> > I just want to figure out what we are trading off against. This is not >> > one side wins all situations. >> >> Per my understanding, we don't batch memcg uncharging in >> swap_entry_free() now. Although it's possible and may improve >> performance. > > Yes. It actually causes a long tail in swapin fault latency as Chris > discovered in our prod. I am wondering if doing the memcg uncharging > outside the slots cache will actually amortize the cost instead. > > Regardless of memcg charging, which is more complicated, I think we > should at least move the call to zswap_invalidate() before the slots > cache. I would prefer that we move everything non-swapfile specific > outside the slots cache layer (zswap_invalidate(), > arch_swap_invalidate_page(), clear_shadow_from_swap_cache(), > mem_cgroup_uncharge_swap(), ..). However, if some of those are > controversial, we can move some of them for now. That makes sense for me. > When draining free swap slots from the cache, swap_range_free() is > called with nr_entries == 1 anyway, so I can't see how any batching is > going on. If anything it should help amortize the cost. In swapcache_free_entries(), the sis->lock will be held to free multiple swap slots via swap_info_get_cont() if possible. This can reduce sis->lock contention. -- Best Regards, Huang, Ying
