linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hugh Dickins <hughd@google.com>
To: Yang Shi <shy828301@gmail.com>
Cc: Hugh Dickins <hughd@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Shakeel Butt <shakeelb@google.com>,
	"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
	Miaohe Lin <linmiaohe@huawei.com>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Michal Hocko <mhocko@suse.com>, Rik van Riel <riel@surriel.com>,
	Christoph Hellwig <hch@infradead.org>,
	Matthew Wilcox <willy@infradead.org>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Alexey Gladkov <legion@kernel.org>,
	Chris Wilson <chris@chris-wilson.co.uk>,
	Matthew Auld <matthew.auld@intel.com>,
	Linux FS-devel Mailing List <linux-fsdevel@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-api@vger.kernel.org, Linux MM <linux-mm@kvack.org>
Subject: Re: [PATCH 06/16] huge tmpfs: shmem_is_huge(vma, inode, index)
Date: Sat, 31 Jul 2021 22:22:41 -0700 (PDT)	[thread overview]
Message-ID: <e7374d7e-4773-aba1-763-8fa2c953f917@google.com> (raw)
In-Reply-To: <CAHbLzkoKZ9OdUfP5DX81CKOJWrRZ0GANrmenNeKWNmSOgUh0bQ@mail.gmail.com>

On Fri, 30 Jul 2021, Yang Shi wrote:
> On Fri, Jul 30, 2021 at 12:42 AM Hugh Dickins <hughd@google.com> wrote:
> >
> > Extend shmem_huge_enabled(vma) to shmem_is_huge(vma, inode, index), so
> > that a consistent set of checks can be applied, even when the inode is
> > accessed through read/write syscalls (with NULL vma) instead of mmaps
> > (the index argument is seldom of interest, but required by mount option
> > "huge=within_size").  Clean up and rearrange the checks a little.
> >
> > This then replaces the checks which shmem_fault() and shmem_getpage_gfp()
> > were making, and eliminates the SGP_HUGE and SGP_NOHUGE modes: while it's
> > still true that khugepaged's collapse_file() at that point wants a small
> > page, the race that might allocate it a huge page is too unlikely to be
> > worth optimizing against (we are there *because* there was at least one
> > small page in the way), and handled by a later PageTransCompound check.
> 
> Yes, it seems too unlikely. But if it happens the PageTransCompound
> check may be not good enough since the page allocated by
> shmem_getpage() may be charged to wrong memcg (root memcg). And it
> won't be replaced by a newly allocated huge page so the wrong charge
> can't be undone.

Good point on the memcg charge: I hadn't thought of that.  Of course
it's not specific to SGP_CACHE versus SGP_NOHUGE (this patch), but I
admit that a huge mischarge is hugely worse than a small mischarge.

We could fix it by making shmem_getpage_gfp() non-static, and pointing
to the vma (hence its mm, hence its memcg) here, couldn't we?  Easily
done, but I don't really want to make shmem_getpage_gfp() public just
for this, for two reasons.

One is that the huge race it just so unlikely; and a mischarge to root
is not the end of the world, so long as it's not reproducible.  It can
only happen on the very first page of the huge extent, and the prior
"Stop if extent has been truncated" check makes sure there was one
entry in the extent at that point: so the race with hole-punch can only
occur after we xas_unlock_irq(&xas) immediately before shmem_getpage()
looks up the page in the tree (and I say hole-punch not truncate,
because shmem_getpage()'s i_size check will reject when truncated).
I don't doubt that it could happen, but stand by not optimizing against.

Other reason is that doing shmem_getpage() (or shmem_getpage_gfp())
there is unhealthy for unrelated reasons, that I cannot afford to get
into sending patches for at this time: but some of our users found the
worst-case latencies in collapse_file() intolerable - shmem_getpage()
may be reading in from swap, while the locked head of the huge page
being built is in the page cache keeping other users waiting.  So,
I'd say there's something worse than memcg in that shmem_getpage(),
but fixing that cannot be a part of this series.

> 
> And, another question is it seems the newly allocated huge page will
> just be uncharged instead of being freed until
> "khugepaged_pages_to_scan" pages are scanned. The
> khugepaged_prealloc_page() is called to free the allocated huge page
> before each call to khugepaged_scan_mm_slot(). But
> khugepaged_scan_file() -> collapse_fille() -> khugepaged_alloc_page()
> may be called multiple times in the loop in khugepaged_scan_mm_slot(),
> so khugepaged_alloc_page() may see that page to trigger VM_BUG IIUC.
> 
> The code is quite convoluted, I'm not sure whether I miss something or
> not. And this problem seems very hard to trigger in real life
> workload.

Just to clarify, those two paragraphs are not about this patch, but about
what happens to mm/khugepaged.c's newly allocated huge page, when collapse
fails for any reason.

Yes, the code is convoluted: that's because it takes very different paths
when CONFIG_NUMA=y (when it cannot predict which node to allocate from)
and when not NUMA (when it can allocate the huge page at a good unlocked
moment, and carry it forward from one attempt to the next).

I don't like it at all, the two paths are confusing: sometimes I wonder
whether we should just remove the !CONFIG_NUMA path entirely; and other
times I wonder in the other direction, whether the CONFIG_NUMA=y path
ought to go the other way when it finds nr_node_ids is 1.  Undecided.

I'm confident that if you work through the two cases (thinking about
only one of them at once!), you'll find that the failure paths (not
to mention the successful paths) do actually work correctly without
leaking (well, maybe the !NUMA path can hold on to one huge page
indefinitely, I forget, but I wouldn't count that as leaking).

Collapse failure is not uncommon and leaking huge pages gets noticed.

Hugh

  reply	other threads:[~2021-08-01  5:22 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-30  7:22 [PATCH 00/16] tmpfs: HUGEPAGE and MEM_LOCK fcntls and memfds Hugh Dickins
2021-07-30  7:25 ` [PATCH 01/16] huge tmpfs: fix fallocate(vanilla) advance over huge pages Hugh Dickins
2021-07-30 21:36   ` Yang Shi
2021-08-01  3:38     ` Hugh Dickins
2021-08-02 20:36       ` Yang Shi
2021-07-30  7:28 ` [PATCH 02/16] huge tmpfs: fix split_huge_page() after FALLOC_FL_KEEP_SIZE Hugh Dickins
2021-07-30 23:48   ` Yang Shi
2021-07-30  7:30 ` [PATCH 03/16] huge tmpfs: remove shrinklist addition from shmem_setattr() Hugh Dickins
2021-07-30 21:50   ` Yang Shi
2021-07-30  7:36 ` [PATCH 04/16] huge tmpfs: revert shmem's use of transhuge_vma_enabled() Hugh Dickins
2021-07-30 21:56   ` Yang Shi
2021-08-01  4:01     ` Hugh Dickins
2021-08-02 20:39       ` Yang Shi
2021-07-30  7:39 ` [PATCH 05/16] huge tmpfs: move shmem_huge_enabled() upwards Hugh Dickins
2021-07-30 21:57   ` Yang Shi
2021-07-30  7:42 ` [PATCH 06/16] huge tmpfs: shmem_is_huge(vma, inode, index) Hugh Dickins
2021-07-30 23:34   ` Yang Shi
2021-08-01  5:22     ` Hugh Dickins [this message]
2021-08-01  5:37       ` Hugh Dickins
2021-08-02 21:14       ` Yang Shi
2021-08-04  8:28         ` Hugh Dickins
2021-08-04 19:01           ` Yang Shi
2021-08-06  5:21             ` Hugh Dickins
2021-08-06 17:41               ` Yang Shi
2021-08-05 23:04         ` Yang Shi
2021-08-06  5:43           ` Hugh Dickins
2021-08-06 17:57             ` Yang Shi
2021-08-12 18:19               ` Yang Shi
2021-07-30  7:45 ` [PATCH 07/16] memfd: memfd_create(name, MFD_HUGEPAGE) for shmem huge pages Hugh Dickins
2021-08-04 14:03   ` Kirill A. Shutemov
2021-08-06  3:33     ` Hugh Dickins
2021-07-30  7:48 ` [PATCH 08/16] huge tmpfs: fcntl(fd, F_HUGEPAGE) and fcntl(fd, F_NOHUGEPAGE) Hugh Dickins
2021-08-04 14:08   ` Kirill A. Shutemov
2021-08-06  4:34     ` Hugh Dickins
2021-07-30  7:51 ` [PATCH 09/16] huge tmpfs: decide stat.st_blksize by shmem_is_huge() Hugh Dickins
2021-07-30 23:40   ` Yang Shi
2021-07-30  7:55 ` [PATCH 10/16] tmpfs: fcntl(fd, F_MEM_LOCK) to memlock a tmpfs file Hugh Dickins
2021-08-03  1:38   ` Matthew Wilcox
2021-08-04  9:15     ` Hugh Dickins
2021-07-30  7:57 ` [PATCH 11/16] tmpfs: fcntl(fd, F_MEM_LOCKED) to test if memlocked Hugh Dickins
2021-07-30  8:00 ` [PATCH 12/16] tmpfs: refuse memlock when fallocated beyond i_size Hugh Dickins
2021-07-30  8:03 ` [PATCH 13/16] mm: bool user_shm_lock(loff_t size, struct ucounts *) Hugh Dickins
2021-07-30  8:06 ` [PATCH 14/16] mm: user_shm_lock(,,getuc) and user_shm_unlock(,,putuc) Hugh Dickins
2021-07-30  8:09 ` [PATCH 15/16] tmpfs: permit changing size of memlocked file Hugh Dickins
2021-07-30  8:13 ` [PATCH 16/16] memfd: memfd_create(name, MFD_MEM_LOCK) for memlocked shmem Hugh Dickins

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e7374d7e-4773-aba1-763-8fa2c953f917@google.com \
    --to=hughd@google.com \
    --cc=akpm@linux-foundation.org \
    --cc=chris@chris-wilson.co.uk \
    --cc=ebiederm@xmission.com \
    --cc=hch@infradead.org \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=legion@kernel.org \
    --cc=linmiaohe@huawei.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=matthew.auld@intel.com \
    --cc=mhocko@suse.com \
    --cc=mike.kravetz@oracle.com \
    --cc=riel@surriel.com \
    --cc=shakeelb@google.com \
    --cc=shy828301@gmail.com \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH 06/16] huge tmpfs: shmem_is_huge(vma, inode, index)' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).