Re: [PATCH 05/18] mm: memcontrol: convert page cache to a new mem_cgroup_charge() API
From: Hugh Dickins <hidden>
Date: 2020-05-11 18:44:39
Also in:
linux-mm, lkml
On Mon, 11 May 2020, Johannes Weiner wrote:
Since commit b56a2d8af914 ("mm: rid swapoff of quadratic complexity"),
shmem_unuse_inode() doesn't have its own copy anymore - it uses
shmem_swapin_page().
However, that commit appears to have made shmem's private call to
delete_from_swap_cache() obsolete as well. Whereas before this change
we fully relied on shmem_unuse() to find and clear a shmem swap entry
and its swapcache page, we now only need it to clean out shmem's
private state in the inode, as it's followed by a loop over all
remaining swap slots, calling try_to_free_swap() on stragglers.Great, you've looked deeper into the current situation than I had.
Unless I missed something, it's still merely an optimization, and we can delete it for simplicity:
Yes, nice ---s, simpler code, and a good idea to separate it out as a precursor: thanks, Hannes.
---
From fc9dcaf68c8b54baf365cd670fb5780c7f0d243f Mon Sep 17 00:00:00 2001
From: Johannes Weiner <redacted>
Date: Mon, 11 May 2020 12:59:08 -0400
Subject: [PATCH] mm: shmem: remove rare optimization when swapin races with
hole punching
Commit 215c02bc33bb ("tmpfs: fix shmem_getpage_gfp() VM_BUG_ON")
recognized that hole punching can race with swapin and removed the
BUG_ON() for a truncated entry from the swapin path.
The patch also added a swapcache deletion to optimize this rare case:
Since swapin has the page locked, and free_swap_and_cache() merely
trylocks, this situation can leave the page stranded in
swapcache. Usually, page reclaim picks up stale swapcache pages, and
the race can happen at any other time when the page is locked. (The
same happens for non-shmem swapin racing with page table zapping.) The
thinking here was: we already observed the race and we have the page
locked, we may as well do the cleanup instead of waiting for reclaim.
However, this optimization complicates the next patch which moves the
cgroup charging code around. As this is just a minor speedup for a
race condition that is so rare that it required a fuzzer to trigger
the original BUG_ON(), it's no longer worth the complications.
Suggested-by: Hugh Dickins <redacted>
Signed-off-by: Johannes Weiner <redacted>Acked-by: Hugh Dickins <redacted> (if one is allowed to suggest and to ack)
quoted hunk ↗ jump to hunk
--- mm/shmem.c | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-)diff --git a/mm/shmem.c b/mm/shmem.c index d505b6cce4ab..729bbb3513cd 100644 --- a/mm/shmem.c +++ b/mm/shmem.c@@ -1665,27 +1665,16 @@ static int shmem_swapin_page(struct inode *inode, pgoff_t index, } error = mem_cgroup_try_charge_delay(page, charge_mm, gfp, &memcg); - if (!error) { - error = shmem_add_to_page_cache(page, mapping, index, - swp_to_radix_entry(swap), gfp); - /* - * We already confirmed swap under page lock, and make - * no memory allocation here, so usually no possibility - * of error; but free_swap_and_cache() only trylocks a - * page, so it is just possible that the entry has been - * truncated or holepunched since swap was confirmed. - * shmem_undo_range() will have done some of the - * unaccounting, now delete_from_swap_cache() will do - * the rest. - */ - if (error) { - mem_cgroup_cancel_charge(page, memcg); - delete_from_swap_cache(page); - } - } if (error) goto failed; + error = shmem_add_to_page_cache(page, mapping, index, + swp_to_radix_entry(swap), gfp); + if (error) { + mem_cgroup_cancel_charge(page, memcg); + goto failed; + } + mem_cgroup_commit_charge(page, memcg, true); spin_lock_irq(&info->lock);-- 2.26.2