On Mon, Nov 20, 2023 at 4:57 PM Huang, Ying <ying.huang@xxxxxxxxx> wrote: > > 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. Ah yes that's a good point. Since most of these callbacks don't actually access sis, but use the swap entry value itself, I am guessing the reason we need to hold the lock for all these callbacks is to prevent swapoff and swapon reusing the same swap entry on a different swap device, right?
