On Fri, Mar 5, 2021 at 8:25 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote: > > On Fri, Mar 05, 2021 at 12:06:31AM -0800, Hugh Dickins wrote: > > On Wed, 3 Mar 2021, Shakeel Butt wrote: > > > > > Currently the kernel adds the page, allocated for swapin, to the > > > swapcache before charging the page. This is fine but now we want a > > > per-memcg swapcache stat which is essential for folks who wants to > > > transparently migrate from cgroup v1's memsw to cgroup v2's memory and > > > swap counters. In addition charging a page before exposing it to other > > > parts of the kernel is a step in the right direction. > > > > > > To correctly maintain the per-memcg swapcache stat, this patch has > > > adopted to charge the page before adding it to swapcache. One > > > challenge in this option is the failure case of add_to_swap_cache() on > > > which we need to undo the mem_cgroup_charge(). Specifically undoing > > > mem_cgroup_uncharge_swap() is not simple. > > > > > > To resolve the issue, this patch introduces transaction like interface > > > to charge a page for swapin. The function mem_cgroup_charge_swapin_page() > > > initiates the charging of the page and mem_cgroup_finish_swapin_page() > > > completes the charging process. So, the kernel starts the charging > > > process of the page for swapin with mem_cgroup_charge_swapin_page(), > > > adds the page to the swapcache and on success completes the charging > > > process with mem_cgroup_finish_swapin_page(). > > > > > > Signed-off-by: Shakeel Butt <shakeelb@xxxxxxxxxx> > > > > Quite apart from helping with the stat you want, what you've ended > > up with here is a nice cleanup in several different ways (and I'm > > glad Johannes talked you out of __GFP_NOFAIL: much better like this). > > I'll say > > > > Acked-by: Hugh Dickins <hughd@xxxxxxxxxx> > > > > but I am quite unhappy with the name mem_cgroup_finish_swapin_page(): > > it doesn't finish the swapin, it doesn't finish the page, and I'm > > not persuaded by your paragraph above that there's any "transaction" > > here (if there were, I'd suggest "commit" instead of "finish"'; and > > I'd get worried by the css_put before it's called - but no, that's > > fine, it's independent). > > > > How about complementing mem_cgroup_charge_swapin_page() with > > mem_cgroup_uncharge_swapin_swap()? I think that describes well > > what it does, at least in the do_memsw_account() case, and I hope > > we can overlook that it does nothing at all in the other case. > > Yes, that's better. The sequence is still somewhat mysterious for > people not overly familiar with memcg swap internals, but it's much > clearer for people who are. > > I mildly prefer swapping the swapin bit: > > mem_cgroup_swapin_charge_page() > mem_cgroup_swapin_uncharge_swap() > > But either way works for me. > I will do a coin toss to select one. > > And it really doesn't need a page argument: both places it's called > > have just allocated an order-0 page, there's no chance of a THP here; > > but you might have some idea of future expansion, or matching > > put_swap_page() - I won't object if you prefer to pass in the page. > > Agreed. Will remove the page parameter. BTW I will keep mem_cgroup_disabled() check in the uncharge swap function as I am thinking of removing "swapaccount=0" functionality.
