shmem_fallocate() goes to a lot of trouble to leave its newly allocated
pages !Uptodate, partly to identify and undo them on failure, partly to
leave the overhead of clearing them until later. But the huge page case
did not skip to the end of the extent, walked through the tail pages one
by one, and appeared to work just fine: but in doing so, cleared and
Uptodated the huge page, so there was no way to undo it on failure.
Now advance immediately to the end of the huge extent, with a comment on
why this is more than just an optimization. But although this speeds up
huge tmpfs fallocation, it does leave the clearing until first use, and
some users may have come to appreciate slow fallocate but fast first use:
if they complain, then we can consider adding a pass to clear at the end.
Fixes: 800d8c63b2e9 ("shmem: add huge pages support")
Signed-off-by: Hugh Dickins <hughd@google.com>
---
mm/shmem.c | 19 ++++++++++++++++---
1 file changed, 16 insertions(+), 3 deletions(-)
A successful shmem_fallocate() guarantees that the extent has been
reserved, even beyond i_size when the FALLOC_FL_KEEP_SIZE flag was used.
But that guarantee is broken by shmem_unused_huge_shrink()'s attempts to
split huge pages and free their excess beyond i_size; and by other uses
of split_huge_page() near i_size.
It's sad to add a shmem inode field just for this, but I did not find a
better way to keep the guarantee. A flag to say KEEP_SIZE has been used
would be cheaper, but I'm averse to unclearable flags. The fallocend
field is not perfect either (many disjoint ranges might be fallocated),
but good enough; and gains another use later on.
Fixes: 779750d20b93 ("shmem: split huge pages beyond i_size under memory pressure")
Signed-off-by: Hugh Dickins <hughd@google.com>
---
include/linux/shmem_fs.h | 13 +++++++++++++
mm/huge_memory.c | 6 ++++--
mm/shmem.c | 15 ++++++++++++++-
3 files changed, 31 insertions(+), 3 deletions(-)
@@ -2454,11 +2454,11 @@ static void __split_huge_page(struct page *page, struct list_head *list,for(i=nr-1;i>=1;i--){__split_huge_page_tail(head,i,lruvec,list);-/* Some pages can be beyond i_size: drop them from page cache */+/* Some pages can be beyond EOF: drop them from page cache */if(head[i].index>=end){ClearPageDirty(head+i);__delete_from_page_cache(head+i,NULL);-if(IS_ENABLED(CONFIG_SHMEM)&&PageSwapBacked(head))+if(shmem_mapping(head->mapping))shmem_uncharge(head->mapping->host,1);put_page(head+i);}elseif(!PageAnon(page)){
@@ -905,6 +905,9 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,if(lend==-1)end=-1;/* unsigned, so actually very big */+if(info->fallocend>start&&info->fallocend<=end&&!unfalloc)+info->fallocend=start;+pagevec_init(&pvec);index=start;while(index<end&&find_lock_entries(mapping,index,end-1,
@@ -2667,7 +2670,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,structshmem_sb_info*sbinfo=SHMEM_SB(inode->i_sb);structshmem_inode_info*info=SHMEM_I(inode);structshmem_fallocshmem_falloc;-pgoff_tstart,index,end;+pgoff_tstart,index,end,undo_fallocend;interror;if(mode&~(FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE))
@@ -2736,6 +2739,15 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,inode->i_private=&shmem_falloc;spin_unlock(&inode->i_lock);+/*+*info->fallocendisonlyrelevantwhenhugepagesmightbe+*involved:topreventsplit_huge_page()freeingfallocated+*pageswhenFALLOC_FL_KEEP_SIZEcommittedbeyondi_size.+*/+undo_fallocend=info->fallocend;+if(info->fallocend<end)+info->fallocend=end;+for(index=start;index<end;){structpage*page;
@@ -2750,6 +2762,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,elseerror=shmem_getpage(inode,index,&page,SGP_FALLOC);if(error){+info->fallocend=undo_fallocend;/* Remove the !PageUptodate pages we added */if(index>start){shmem_undo_range(inode,
There's a block of code in shmem_setattr() to add the inode to
shmem_unused_huge_shrink()'s shrinklist when lowering i_size: it dates
from before 5.7 changed truncation to do split_huge_page() for itself,
and should have been removed at that time.
I am over-stating that: split_huge_page() can fail (notably if there's
an extra reference to the page at that time), so there might be value in
retrying. But there were already retries as truncation worked through
the tails, and this addition risks repeating unsuccessful retries
indefinitely: I'd rather remove it now, and work on reducing the
chance of split_huge_page() failures separately, if we need to.
Fixes: 71725ed10c40 ("mm: huge tmpfs: try to split_huge_page() when punching hole")
Signed-off-by: Hugh Dickins <hughd@google.com>
---
mm/shmem.c | 19 -------------------
1 file changed, 19 deletions(-)
5.14 commit e6be37b2e7bd ("mm/huge_memory.c: add missing read-only THP
checking in transparent_hugepage_enabled()") added transhuge_vma_enabled()
as a wrapper for two very different checks: shmem_huge_enabled() prefers
to show those two checks explicitly, as before.
Signed-off-by: Hugh Dickins <hughd@google.com>
---
mm/shmem.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
shmem_huge_enabled() is about to be enhanced into shmem_is_huge(),
so that it can be used more widely throughout: before making functional
changes, shift it to its final position (to avoid forward declaration).
Signed-off-by: Hugh Dickins <hughd@google.com>
---
mm/shmem.c | 72 ++++++++++++++++++++++++++----------------------------
1 file changed, 35 insertions(+), 37 deletions(-)
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.
Replace a couple of 0s by explicit SHMEM_HUGE_NEVERs; and replace the
obscure !shmem_mapping() symlink check by explicit S_ISLNK() - nothing
else needs that symlink check, so leave it there in shmem_getpage_gfp().
Signed-off-by: Hugh Dickins <hughd@google.com>
---
include/linux/shmem_fs.h | 9 +++--
mm/khugepaged.c | 2 +-
mm/shmem.c | 84 ++++++++++++----------------------------
3 files changed, 32 insertions(+), 63 deletions(-)
@@ -95,8 +100,6 @@ extern unsigned long shmem_partial_swap_usage(struct address_space *mapping,enumsgp_type{SGP_READ,/* don't exceed i_size, don't allocate page */SGP_CACHE,/* don't exceed i_size, may allocate page */-SGP_NOHUGE,/* like SGP_CACHE, but no huge pages */-SGP_HUGE,/* like SGP_CACHE, huge pages preferred */SGP_WRITE,/* may exceed i_size, may allocate !Uptodate page */SGP_FALLOC,/* like SGP_WRITE, but make existing page Uptodate */};
@@ -3961,7 +3927,7 @@ int __init shmem_init(void)if(has_transparent_hugepage()&&shmem_huge>SHMEM_HUGE_DENY)SHMEM_SB(shm_mnt->mnt_sb)->huge=shmem_huge;else-shmem_huge=0;/* just in case it was patched */+shmem_huge=SHMEM_HUGE_NEVER;/* just in case it was patched */#endifreturn0;
Commit 749df87bd7be ("mm/shmem: add hugetlbfs support to memfd_create()")
in 4.14 added the MFD_HUGETLB flag to memfd_create(), to use hugetlbfs
pages instead of tmpfs pages: now add the MFD_HUGEPAGE flag, to use tmpfs
Transparent Huge Pages when they can be allocated (flag named to follow
the precedent of madvise's MADV_HUGEPAGE for THPs).
/sys/kernel/mm/transparent_hugepage/shmem_enabled "always" or "force"
already made this possible: but that is much too blunt an instrument,
affecting all the very different kinds of files on the internal shmem
mount, and was intended just for ease of testing hugepage loads.
MFD_HUGEPAGE is implemented internally by VM_HUGEPAGE in the shmem inode
flags: do not permit a PR_SET_THP_DISABLE (MMF_DISABLE_THP) task to set
this flag, and do not set it if THPs are not allowed at all; but let the
memfd_create() succeed even in those cases - the caller wants to create a
memfd, just hinting how it's best allocated if huge pages are available.
shmem_is_huge() (at allocation time or khugepaged time) applies its
SHMEM_HUGE_DENY and vma VM_NOHUGEPAGE and vm_mm MMF_DISABLE_THP checks
first, and only then allows the memfd's MFD_HUGEPAGE to take effect.
Signed-off-by: Hugh Dickins <hughd@google.com>
---
include/uapi/linux/memfd.h | 3 ++-
mm/memfd.c | 24 ++++++++++++++++++------
mm/shmem.c | 33 +++++++++++++++++++++++++++++++--
3 files changed, 51 insertions(+), 9 deletions(-)
Add support for fcntl(fd, F_HUGEPAGE) and fcntl(fd, F_NOHUGEPAGE), to
select hugeness per file: useful to override the default hugeness of the
shmem mount, when occasionally needing to store a hugepage file in a
smallpage mount or vice versa.
These fcntls just specify whether or not to try for huge pages when
allocating to the object later: F_HUGEPAGE does not touch small pages
already allocated (though khugepaged may do so when the file is mapped
afterwards), F_NOHUGEPAGE does not split huge pages already allocated.
Why fcntl? Because it's already in use (for sealing) on memfds; and I'm
anxious to keep this simple, just applying it to whole files: fallocate,
madvise and posix_fadvise each involve a range, which would need a new
kind of tree attached to the inode for proper support. Any application
needing range support should be able to provide that from userspace, by
issuing the respective fcntl prior to instantiating each range.
Do not allow it when the file is open read-only (EBADF). Do not permit
a PR_SET_THP_DISABLE (MMF_DISABLE_THP) task to interfere with the flags,
and do not let VM_HUGEPAGE be set if THPs are not allowed at all (EPERM).
Note that transparent_hugepage_allowed(), used to validate F_HUGEPAGE,
accepts (anon) transparent_hugepage_flags in addition to mount option.
This is to overcome the limitation of the "huge=advise" option, which
applies hugepage alignment (reducing ASLR) to all mappings, because
madvise(address,len,MADV_HUGEPAGE) needs address before it can be used.
So mount option "huge=never" gives a default which can be overridden by
fcntl(fd, F_HUGEPAGE) when /sys/kernel/mm/transparent_hugepage/enabled
is not "never" too. (We could instead add a "huge=fcntl" mount option
between "never" and "advise", but I lack the enthusiasm for that.)
Signed-off-by: Hugh Dickins <hughd@google.com>
---
fs/fcntl.c | 5 +++
include/linux/shmem_fs.h | 8 +++++
include/uapi/linux/fcntl.h | 9 +++++
mm/shmem.c | 70 ++++++++++++++++++++++++++++++++++----
4 files changed, 85 insertions(+), 7 deletions(-)
@@ -434,6 +435,10 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,caseF_SET_FILE_RW_HINT:err=fcntl_rw_hint(filp,cmd,arg);break;+caseF_HUGEPAGE:+caseF_NOHUGEPAGE:+err=shmem_fcntl(filp,cmd,arg);+break;default:break;}
4.18 commit 89fdcd262fd4 ("mm: shmem: make stat.st_blksize return huge
page size if THP is on") added is_huge_enabled() to decide st_blksize:
now that hugeness can be defined per file, that too needs to be replaced
by shmem_is_huge().
Unless they have been fcntl'ed F_HUGEPAGE, this does give a different
answer (No) for small files on a "huge=within_size" mount: but that can
be considered a minor bugfix. And a different answer (No) for unfcntl'ed
files on a "huge=advise" mount: I'm reluctant to complicate it, just to
reproduce the same debatable answer as before.
Signed-off-by: Hugh Dickins <hughd@google.com>
---
mm/shmem.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
From: Shakeel Butt <redacted>
A new uapi to lock the files on tmpfs in memory, to protect against swap
without mapping the files. This commit introduces two new commands to
fcntl and shmem: F_MEM_LOCK and F_MEM_UNLOCK. The locking will be
charged against RLIMIT_MEMLOCK of uid in namespace of the caller.
This feature is implemented by mostly re-using the shmctl's SHM_LOCK
mechanism (System V IPC shared memory). This api follows the design
choices of shmctl's SHM_LOCK and also of mlock2 syscall where pages
on swap are not populated on the syscall. The pages will be brought
to memory on first access.
As with System V shared memory, these pages are counted as Unevictable
in /proc/meminfo (when they are allocated, or when page reclaim finds
any allocated earlier), but they are not counted as Mlocked there.
For simplicity the locked files are forbidden to grow or shrink
to keep the user accounting simple. This design decision will be
revisited once such use-case arises.
The permissions to lock and unlock differs slightly from other similar
interfaces. Anyone having CAP_IPC_LOCK or remaining rlimit can lock
the file, but the unlocker has to have either CAP_IPC_LOCK or it
should be the locker itself.
This commit does not make the locked status of a tmpfs file visible.
We can add an F_MEM_LOCKED fcntl later, to query that status if
required; but it's not yet clear how best to make it visible.
Signed-off-by: Shakeel Butt <redacted>
Signed-off-by: Hugh Dickins <hughd@google.com>
---
fs/fcntl.c | 2 ++
include/linux/shmem_fs.h | 1 +
include/uapi/linux/fcntl.h | 7 +++++
mm/shmem.c | 59 ++++++++++++++++++++++++++++++++++++--
4 files changed, 66 insertions(+), 3 deletions(-)
@@ -437,6 +437,8 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,break;caseF_HUGEPAGE:caseF_NOHUGEPAGE:+caseF_MEM_LOCK:+caseF_MEM_UNLOCK:err=shmem_fcntl(filp,cmd,arg);break;default:
@@ -24,6 +24,7 @@ struct shmem_inode_info {structshared_policypolicy;/* NUMA memory alloc policy */structsimple_xattrsxattrs;/* list of xattrs */atomic_tstop_eviction;/* hold when working on inode */+structucounts*mlock_ucounts;/* user memlocked tmpfs file */structinodevfs_inode;};
@@ -2715,6 +2760,10 @@ long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)caseF_NOHUGEPAGE:error=shmem_huge_fcntl(file,cmd);break;+caseF_MEM_LOCK:+caseF_MEM_UNLOCK:+error=shmem_memlock_fcntl(file,cmd);+break;}returnerror;
@@ -2778,6 +2827,10 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,error=-EPERM;gotoout;}+if(info->mlock_ucounts&&offset+len>inode->i_size){+error=-EPERM;+gotoout;+}start=offset>>PAGE_SHIFT;end=(offset+len+PAGE_SIZE-1)>>PAGE_SHIFT;
Though we have not yet found a compelling need to make the locked status
of a tmpfs file visible, and offer no tool to show it, the kernel ought
to be able to support such a tool: add the F_MEM_LOCKED fcntl, returning
-1 on failure (not tmpfs), 0 when not F_MEM_LOCKED, 1 when F_MEM_LOCKED.
Signed-off-by: Hugh Dickins <hughd@google.com>
---
fs/fcntl.c | 1 +
include/uapi/linux/fcntl.h | 1 +
mm/shmem.c | 4 ++++
3 files changed, 6 insertions(+)
@@ -439,6 +439,7 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg,caseF_NOHUGEPAGE:caseF_MEM_LOCK:caseF_MEM_UNLOCK:+caseF_MEM_LOCKED:err=shmem_fcntl(filp,cmd,arg);break;default:
@@ -2299,6 +2299,9 @@ static int shmem_memlock_fcntl(struct file *file, unsigned int cmd)boolcleanup_mapping=false;intretval=0;+if(cmd==F_MEM_LOCKED)+return!!info->mlock_ucounts;+inode_lock(inode);if(cmd==F_MEM_LOCK){if(!info->mlock_ucounts){
@@ -2762,6 +2765,7 @@ long shmem_fcntl(struct file *file, unsigned int cmd, unsigned long arg)break;caseF_MEM_LOCK:caseF_MEM_UNLOCK:+caseF_MEM_LOCKED:error=shmem_memlock_fcntl(file,cmd);break;}
F_MEM_LOCK is accounted by i_size, but fallocate(,FALLOC_FL_KEEP_SIZE,,)
could have added many pages beyond i_size, which would also be held as
Unevictable from memory. The mlock_ucounts check in shmem_fallocate() is
fine, but shmem_memlock_fcntl() needs to check fallocend too. We could
change F_MEM_LOCK accounting to use the max of i_size and fallocend, but
fallocend is obscure: I think it's better just to refuse the F_MEM_LOCK
(with EPERM) if fallocend exceeds (page-rounded) i_size.
Signed-off-by: Hugh Dickins <hughd@google.com>
---
mm/shmem.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
@@ -2304,7 +2304,10 @@ static int shmem_memlock_fcntl(struct file *file, unsigned int cmd)inode_lock(inode);if(cmd==F_MEM_LOCK){-if(!info->mlock_ucounts){+if(info->fallocend>DIV_ROUND_UP(inode->i_size,PAGE_SIZE)){+/* locking is accounted by i_size: disallow excess */+retval=-EPERM;+}elseif(!info->mlock_ucounts){structucounts*ucounts=current_ucounts();/* capability/rlimit check is down in user_shm_lock */retval=shmem_lock(file,1,ucounts);
@@ -2854,9 +2857,10 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,spin_unlock(&inode->i_lock);/*-*info->fallocendisonlyrelevantwhenhugepagesmightbe+*info->fallocendismostlyrelevantwhenhugepagesmightbe*involved:topreventsplit_huge_page()freeingfallocated*pageswhenFALLOC_FL_KEEP_SIZEcommittedbeyondi_size.+*ButitisalsocheckedinF_MEM_LOCKvalidation.*/undo_fallocend=info->fallocend;if(info->fallocend<end)
user_shm_lock()'s size_t size was big enough for SysV SHM locking, but
not quite big enough for O_LARGEFILE on 32-bit: change to loff_t size.
And while changing the prototype, let's use bool rather than int here.
Signed-off-by: Hugh Dickins <hughd@google.com>
---
include/linux/mm.h | 4 ++--
mm/mlock.c | 14 +++++++-------
2 files changed, 9 insertions(+), 9 deletions(-)
user_shm_lock() and user_shm_unlock() have to get and put a reference on
the ucounts structure, and get fails at overflow. That will be awkward
for the next commit (shrinking ought not to fail), so add an argument
(always true in this commit) to condition that get and put. It would
be even easier to do the put_ucounts() separately when unlocking, but
messy for the get_ucounts() when locking: better to keep them symmetric.
Signed-off-by: Hugh Dickins <hughd@google.com>
---
fs/hugetlbfs/inode.c | 4 ++--
include/linux/mm.h | 4 ++--
ipc/shm.c | 4 ++--
mm/mlock.c | 9 +++++----
mm/shmem.c | 6 +++---
5 files changed, 14 insertions(+), 13 deletions(-)
We have users who change the size of their memlocked file by F_MEM_UNLOCK,
ftruncate, F_MEM_LOCK. That risks swapout in between, and is distasteful:
particularly if the file is very large (when shmem_unlock_mapping() has a
lot of work to move pages off the Unevictable list, only for them to be
moved back there later on).
Modify shmem_setattr() to grow or shrink, and shmem_fallocate() to grow,
the locked extent. But forbid (EPERM) both if current_ucounts() differs
from the locker's mlock_ucounts (without even a CAP_IPC_LOCK override).
They could be permitted (the caller already has unsealed write access),
but it's probably less confusing to restrict size change to the locker.
But leave shmem_write_begin() as is, preventing the memlocked file from
being extended implicitly by writes beyond EOF: I think that it's best to
demand an explicit size change, by truncate or fallocate, when memlocked.
(But notice in testing "echo x >memlockedfile" how the O_TRUNC succeeds
but the write fails: would F_MEM_UNLOCK on truncation to 0 be better?)
Signed-off-by: Hugh Dickins <hughd@google.com>
---
mm/shmem.c | 48 ++++++++++++++++++++++++++++++++++++++----------
1 file changed, 38 insertions(+), 10 deletions(-)
Now that the size of a memlocked file can be changed, memfd_create() can
accept an MFD_MEM_LOCK flag to request memlocking, even though the initial
size is of course 0.
Signed-off-by: Hugh Dickins <hughd@google.com>
---
include/uapi/linux/memfd.h | 1 +
mm/memfd.c | 7 +++++--
mm/shmem.c | 13 ++++++++++++-
3 files changed, 18 insertions(+), 3 deletions(-)
On Fri, Jul 30, 2021 at 12:25 AM Hugh Dickins [off-list ref] wrote:
shmem_fallocate() goes to a lot of trouble to leave its newly allocated
pages !Uptodate, partly to identify and undo them on failure, partly to
leave the overhead of clearing them until later. But the huge page case
did not skip to the end of the extent, walked through the tail pages one
by one, and appeared to work just fine: but in doing so, cleared and
Uptodated the huge page, so there was no way to undo it on failure.
Now advance immediately to the end of the huge extent, with a comment on
why this is more than just an optimization. But although this speeds up
huge tmpfs fallocation, it does leave the clearing until first use, and
some users may have come to appreciate slow fallocate but fast first use:
if they complain, then we can consider adding a pass to clear at the end.
Fixes: 800d8c63b2e9 ("shmem: add huge pages support")
Signed-off-by: Hugh Dickins <hughd@google.com>
@@ -2736,7 +2736,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,inode->i_private=&shmem_falloc;spin_unlock(&inode->i_lock);-for(index=start;index<end;index++){+for(index=start;index<end;){structpage*page;/*
@@ -2759,13 +2759,26 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,gotoundone;}+index++;+/*+*Hereisamoreimportantoptimizationthanitappears:+*asecondSGP_FALLOConthesamehugepagewillclearit,+*makingitPageUptodateandun-undoableifwefaillater.+*/+if(PageTransCompound(page)){+index=round_up(index,HPAGE_PMD_NR);+/* Beware 32-bit wraparound */+if(!index)+index--;+}+/**Informshmem_writepage()howfarwehavereached.*Noneedforlockorbarrier:wehavethepagelock.*/-shmem_falloc.next++;if(!PageUptodate(page))-shmem_falloc.nr_falloced++;+shmem_falloc.nr_falloced+=index-shmem_falloc.next;+shmem_falloc.next=index;
This also fixed the wrong accounting of nr_falloced, so it should be
able to avoid returning -ENOMEM prematurely IIUC. Is it worth
mentioning in the commit log?
/*
* If !PageUptodate, leave it that way so that freeable pages
--
2.26.2
On Fri, Jul 30, 2021 at 12:31 AM Hugh Dickins [off-list ref] wrote:
There's a block of code in shmem_setattr() to add the inode to
shmem_unused_huge_shrink()'s shrinklist when lowering i_size: it dates
from before 5.7 changed truncation to do split_huge_page() for itself,
and should have been removed at that time.
I am over-stating that: split_huge_page() can fail (notably if there's
an extra reference to the page at that time), so there might be value in
retrying. But there were already retries as truncation worked through
the tails, and this addition risks repeating unsuccessful retries
indefinitely: I'd rather remove it now, and work on reducing the
chance of split_huge_page() failures separately, if we need to.
On Fri, Jul 30, 2021 at 12:36 AM Hugh Dickins [off-list ref] wrote:
5.14 commit e6be37b2e7bd ("mm/huge_memory.c: add missing read-only THP
checking in transparent_hugepage_enabled()") added transhuge_vma_enabled()
as a wrapper for two very different checks: shmem_huge_enabled() prefers
to show those two checks explicitly, as before.
Basically I have no objection to separating them again. But IMHO they
seem not very different. Or just makes things easier for the following
patches?
On Fri, Jul 30, 2021 at 12:39 AM Hugh Dickins [off-list ref] wrote:
shmem_huge_enabled() is about to be enhanced into shmem_is_huge(),
so that it can be used more widely throughout: before making functional
changes, shift it to its final position (to avoid forward declaration).
Signed-off-by: Hugh Dickins <hughd@google.com>
On Fri, Jul 30, 2021 at 12:42 AM Hugh Dickins [off-list ref] 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.
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.
quoted hunk
Replace a couple of 0s by explicit SHMEM_HUGE_NEVERs; and replace the
obscure !shmem_mapping() symlink check by explicit S_ISLNK() - nothing
else needs that symlink check, so leave it there in shmem_getpage_gfp().
Signed-off-by: Hugh Dickins <hughd@google.com>
---
include/linux/shmem_fs.h | 9 +++--
mm/khugepaged.c | 2 +-
mm/shmem.c | 84 ++++++++++++----------------------------
3 files changed, 32 insertions(+), 63 deletions(-)
@@ -95,8 +100,6 @@ extern unsigned long shmem_partial_swap_usage(struct address_space *mapping,enumsgp_type{SGP_READ,/* don't exceed i_size, don't allocate page */SGP_CACHE,/* don't exceed i_size, may allocate page */-SGP_NOHUGE,/* like SGP_CACHE, but no huge pages */-SGP_HUGE,/* like SGP_CACHE, huge pages preferred */SGP_WRITE,/* may exceed i_size, may allocate !Uptodate page */SGP_FALLOC,/* like SGP_WRITE, but make existing page Uptodate */};
@@ -3961,7 +3927,7 @@ int __init shmem_init(void)if(has_transparent_hugepage()&&shmem_huge>SHMEM_HUGE_DENY)SHMEM_SB(shm_mnt->mnt_sb)->huge=shmem_huge;else-shmem_huge=0;/* just in case it was patched */+shmem_huge=SHMEM_HUGE_NEVER;/* just in case it was patched */#endifreturn0;--
On Fri, Jul 30, 2021 at 12:51 AM Hugh Dickins [off-list ref] wrote:
4.18 commit 89fdcd262fd4 ("mm: shmem: make stat.st_blksize return huge
page size if THP is on") added is_huge_enabled() to decide st_blksize:
now that hugeness can be defined per file, that too needs to be replaced
by shmem_is_huge().
Unless they have been fcntl'ed F_HUGEPAGE, this does give a different
answer (No) for small files on a "huge=within_size" mount: but that can
be considered a minor bugfix. And a different answer (No) for unfcntl'ed
files on a "huge=advise" mount: I'm reluctant to complicate it, just to
reproduce the same debatable answer as before.
Signed-off-by: Hugh Dickins <hughd@google.com>
On Fri, Jul 30, 2021 at 12:28 AM Hugh Dickins [off-list ref] wrote:
A successful shmem_fallocate() guarantees that the extent has been
reserved, even beyond i_size when the FALLOC_FL_KEEP_SIZE flag was used.
But that guarantee is broken by shmem_unused_huge_shrink()'s attempts to
split huge pages and free their excess beyond i_size; and by other uses
of split_huge_page() near i_size.
It's sad to add a shmem inode field just for this, but I did not find a
better way to keep the guarantee. A flag to say KEEP_SIZE has been used
would be cheaper, but I'm averse to unclearable flags. The fallocend
field is not perfect either (many disjoint ranges might be fallocated),
but good enough; and gains another use later on.
Fixes: 779750d20b93 ("shmem: split huge pages beyond i_size under memory pressure")
Signed-off-by: Hugh Dickins <hughd@google.com>
@@ -2454,11 +2454,11 @@ static void __split_huge_page(struct page *page, struct list_head *list,for(i=nr-1;i>=1;i--){__split_huge_page_tail(head,i,lruvec,list);-/* Some pages can be beyond i_size: drop them from page cache */+/* Some pages can be beyond EOF: drop them from page cache */if(head[i].index>=end){ClearPageDirty(head+i);__delete_from_page_cache(head+i,NULL);-if(IS_ENABLED(CONFIG_SHMEM)&&PageSwapBacked(head))+if(shmem_mapping(head->mapping))shmem_uncharge(head->mapping->host,1);put_page(head+i);}elseif(!PageAnon(page)){
@@ -905,6 +905,9 @@ static void shmem_undo_range(struct inode *inode, loff_t lstart, loff_t lend,if(lend==-1)end=-1;/* unsigned, so actually very big */+if(info->fallocend>start&&info->fallocend<=end&&!unfalloc)+info->fallocend=start;+pagevec_init(&pvec);index=start;while(index<end&&find_lock_entries(mapping,index,end-1,
@@ -2667,7 +2670,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,structshmem_sb_info*sbinfo=SHMEM_SB(inode->i_sb);structshmem_inode_info*info=SHMEM_I(inode);structshmem_fallocshmem_falloc;-pgoff_tstart,index,end;+pgoff_tstart,index,end,undo_fallocend;interror;if(mode&~(FALLOC_FL_KEEP_SIZE|FALLOC_FL_PUNCH_HOLE))
@@ -2736,6 +2739,15 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,inode->i_private=&shmem_falloc;spin_unlock(&inode->i_lock);+/*+*info->fallocendisonlyrelevantwhenhugepagesmightbe+*involved:topreventsplit_huge_page()freeingfallocated+*pageswhenFALLOC_FL_KEEP_SIZEcommittedbeyondi_size.+*/+undo_fallocend=info->fallocend;+if(info->fallocend<end)+info->fallocend=end;+for(index=start;index<end;){structpage*page;
@@ -2750,6 +2762,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,elseerror=shmem_getpage(inode,index,&page,SGP_FALLOC);if(error){+info->fallocend=undo_fallocend;/* Remove the !PageUptodate pages we added */if(index>start){shmem_undo_range(inode,--
On Fri, Jul 30, 2021 at 12:25 AM Hugh Dickins [off-list ref] wrote:
quoted
shmem_fallocate() goes to a lot of trouble to leave its newly allocated
pages !Uptodate, partly to identify and undo them on failure, partly to
leave the overhead of clearing them until later. But the huge page case
did not skip to the end of the extent, walked through the tail pages one
by one, and appeared to work just fine: but in doing so, cleared and
Uptodated the huge page, so there was no way to undo it on failure.
Now advance immediately to the end of the huge extent, with a comment on
why this is more than just an optimization. But although this speeds up
huge tmpfs fallocation, it does leave the clearing until first use, and
some users may have come to appreciate slow fallocate but fast first use:
if they complain, then we can consider adding a pass to clear at the end.
Fixes: 800d8c63b2e9 ("shmem: add huge pages support")
Signed-off-by: Hugh Dickins <hughd@google.com>
@@ -2736,7 +2736,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,inode->i_private=&shmem_falloc;spin_unlock(&inode->i_lock);-for(index=start;index<end;index++){+for(index=start;index<end;){structpage*page;/*
@@ -2759,13 +2759,26 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,gotoundone;}+index++;+/*+*Hereisamoreimportantoptimizationthanitappears:+*asecondSGP_FALLOConthesamehugepagewillclearit,+*makingitPageUptodateandun-undoableifwefaillater.+*/+if(PageTransCompound(page)){+index=round_up(index,HPAGE_PMD_NR);+/* Beware 32-bit wraparound */+if(!index)+index--;+}+/**Informshmem_writepage()howfarwehavereached.*Noneedforlockorbarrier:wehavethepagelock.*/-shmem_falloc.next++;if(!PageUptodate(page))-shmem_falloc.nr_falloced++;+shmem_falloc.nr_falloced+=index-shmem_falloc.next;+shmem_falloc.next=index;
This also fixed the wrong accounting of nr_falloced, so it should be
able to avoid returning -ENOMEM prematurely IIUC. Is it worth
mentioning in the commit log?
It took me a long time to see your point there: ah yes, because it made
the whole huge page Uptodate when it reached the first tail, there would
have been only one nr_falloced++ for the whole of the huge page: well
spotted, thanks, I hadn't realized that.
Though I'm not so sure about your premature -ENOMEM: because once it has
made the huge page Uptodate, the other end (shmem_writepage()) will not
be incrementing nr_unswapped at all: so -ENOMEM would have been deferred
rather than premature, wouldn't it?
Add a comment on this in the commit log: yes, I guess so, but I haven't
worked out what to write yet.
Hugh
quoted
/*
* If !PageUptodate, leave it that way so that freeable pages
--
2.26.2
On Fri, Jul 30, 2021 at 12:36 AM Hugh Dickins [off-list ref] wrote:
quoted
5.14 commit e6be37b2e7bd ("mm/huge_memory.c: add missing read-only THP
checking in transparent_hugepage_enabled()") added transhuge_vma_enabled()
as a wrapper for two very different checks: shmem_huge_enabled() prefers
to show those two checks explicitly, as before.
Basically I have no objection to separating them again. But IMHO they
seem not very different. Or just makes things easier for the following
patches?
Well, it made it easier to apply the patch I'd prepared earlier,
but that was not the point; and I thought it best to be upfront
about the reversion, rather than hiding it in the movement.
The end result of the two checks is the same (don't try for huge pages),
and they have been grouped together because they occurred together in
several places, and both rely on "vma".
But one check is whether the app has marked that address range not to use
THPs; and the other check is whether the process is running in a hierarchy
that has been marked never to use THPs (which just uses vma to get to mm
to get to mm->flags (whether current->mm would be more relevant is not an
argument I want to get into, I'm not at all sure)).
To me those are very different; and I'm particularly concerned to make
MMF_DISABLE_THP references visible, since it did not exist when Kirill
and I first implemented shmem huge pages, and I've tended to forget it:
but consider it more in this series.
Hugh
On Fri, Jul 30, 2021 at 12:42 AM Hugh Dickins [off-list ref] wrote:
quoted
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
On Fri, Jul 30, 2021 at 12:42 AM Hugh Dickins [off-list ref] wrote:
quoted
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.
Stupid me (and maybe I haven't given this enough consideration yet):
but, much better than SGP_NOHUGE, much better than SGP_CACHE, would be
SGP_READ there, wouldn't it? Needs to beware of the NULL too, of course.
Hugh
On Sat, Jul 31, 2021 at 8:38 PM Hugh Dickins [off-list ref] wrote:
On Fri, 30 Jul 2021, Yang Shi wrote:
quoted
On Fri, Jul 30, 2021 at 12:25 AM Hugh Dickins [off-list ref] wrote:
quoted
shmem_fallocate() goes to a lot of trouble to leave its newly allocated
pages !Uptodate, partly to identify and undo them on failure, partly to
leave the overhead of clearing them until later. But the huge page case
did not skip to the end of the extent, walked through the tail pages one
by one, and appeared to work just fine: but in doing so, cleared and
Uptodated the huge page, so there was no way to undo it on failure.
Now advance immediately to the end of the huge extent, with a comment on
why this is more than just an optimization. But although this speeds up
huge tmpfs fallocation, it does leave the clearing until first use, and
some users may have come to appreciate slow fallocate but fast first use:
if they complain, then we can consider adding a pass to clear at the end.
Fixes: 800d8c63b2e9 ("shmem: add huge pages support")
Signed-off-by: Hugh Dickins <hughd@google.com>
@@ -2736,7 +2736,7 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,inode->i_private=&shmem_falloc;spin_unlock(&inode->i_lock);-for(index=start;index<end;index++){+for(index=start;index<end;){structpage*page;/*
@@ -2759,13 +2759,26 @@ static long shmem_fallocate(struct file *file, int mode, loff_t offset,gotoundone;}+index++;+/*+*Hereisamoreimportantoptimizationthanitappears:+*asecondSGP_FALLOConthesamehugepagewillclearit,+*makingitPageUptodateandun-undoableifwefaillater.+*/+if(PageTransCompound(page)){+index=round_up(index,HPAGE_PMD_NR);+/* Beware 32-bit wraparound */+if(!index)+index--;+}+/**Informshmem_writepage()howfarwehavereached.*Noneedforlockorbarrier:wehavethepagelock.*/-shmem_falloc.next++;if(!PageUptodate(page))-shmem_falloc.nr_falloced++;+shmem_falloc.nr_falloced+=index-shmem_falloc.next;+shmem_falloc.next=index;
This also fixed the wrong accounting of nr_falloced, so it should be
able to avoid returning -ENOMEM prematurely IIUC. Is it worth
mentioning in the commit log?
It took me a long time to see your point there: ah yes, because it made
the whole huge page Uptodate when it reached the first tail, there would
have been only one nr_falloced++ for the whole of the huge page: well
spotted, thanks, I hadn't realized that.
Though I'm not so sure about your premature -ENOMEM: because once it has
made the huge page Uptodate, the other end (shmem_writepage()) will not
be incrementing nr_unswapped at all: so -ENOMEM would have been deferred
rather than premature, wouldn't it?
Ah, ok, I didn't pay too much attention to how nr_unswapped is
incremented. Just thought nr_falloced will be incremented by 512
rather than 1, so it is more unlikely to return -ENOMEM.
Add a comment on this in the commit log: yes, I guess so, but I haven't
worked out what to write yet.
Hugh
quoted
quoted
/*
* If !PageUptodate, leave it that way so that freeable pages
--
2.26.2
On Sat, Jul 31, 2021 at 9:01 PM Hugh Dickins [off-list ref] wrote:
On Fri, 30 Jul 2021, Yang Shi wrote:
quoted
On Fri, Jul 30, 2021 at 12:36 AM Hugh Dickins [off-list ref] wrote:
quoted
5.14 commit e6be37b2e7bd ("mm/huge_memory.c: add missing read-only THP
checking in transparent_hugepage_enabled()") added transhuge_vma_enabled()
as a wrapper for two very different checks: shmem_huge_enabled() prefers
to show those two checks explicitly, as before.
Basically I have no objection to separating them again. But IMHO they
seem not very different. Or just makes things easier for the following
patches?
Well, it made it easier to apply the patch I'd prepared earlier,
but that was not the point; and I thought it best to be upfront
about the reversion, rather than hiding it in the movement.
The end result of the two checks is the same (don't try for huge pages),
and they have been grouped together because they occurred together in
several places, and both rely on "vma".
But one check is whether the app has marked that address range not to use
THPs; and the other check is whether the process is running in a hierarchy
that has been marked never to use THPs (which just uses vma to get to mm
to get to mm->flags (whether current->mm would be more relevant is not an
argument I want to get into, I'm not at all sure)).
To me those are very different; and I'm particularly concerned to make
MMF_DISABLE_THP references visible, since it did not exist when Kirill
and I first implemented shmem huge pages, and I've tended to forget it:
but consider it more in this series.
Yes, I agree one checks vma the other one checks mm, they are
different from this perspective. Anyway, as I said I have no objection
to this change. You could add Reviewed-by: Yang Shi
[off-list ref]
On Sat, Jul 31, 2021 at 10:22 PM Hugh Dickins [off-list ref] wrote:
On Fri, 30 Jul 2021, Yang Shi wrote:
quoted
On Fri, Jul 30, 2021 at 12:42 AM Hugh Dickins [off-list ref] wrote:
quoted
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.
The small page could be collapsed to a huge page sooner or later, so
the mischarge may be transient. But huge page can't be replaced.
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
OK, if so the mischarge is not as bad as what I thought in the first place.
"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.
I agree the race is so unlikely and it may be not worth optimizing
against it right now, but a note or a comment may be worth.
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.
Yeah, that is a different problem.
quoted
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 supposed it is just performance consideration to keep the
allocated huge page, but I'm not sure how much the difference would be
if we remove it (remove the !CONFIG_NUMA path) because the pcp could
cache THP now since Mel's patch 44042b449872 ("mm/page_alloc: allow
high-order pages to be stored on the per-cpu lists").
It seems to provide the similar optimization but in the buddy
allocator layer so that khugepaged doesn't have to maintain its own
implementation.
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).
IIUC the NUMA page could hold on to one huge page indefinitely. But
I've never seen the BUG personally, so maybe you are right.
Collapse failure is not uncommon and leaking huge pages gets noticed.
Hugh
From: Matthew Wilcox <willy@infradead.org> Date: 2021-08-03 01:40:40
On Fri, Jul 30, 2021 at 12:55:22AM -0700, Hugh Dickins wrote:
A new uapi to lock the files on tmpfs in memory, to protect against swap
without mapping the files. This commit introduces two new commands to
fcntl and shmem: F_MEM_LOCK and F_MEM_UNLOCK. The locking will be
charged against RLIMIT_MEMLOCK of uid in namespace of the caller.
It's not clear to me why this is limited to shmfs. Would it not also
make sense for traditional filesystems, eg to force chrome's text pages
to stay in the page cache, no matter how much memory the tabs allocate?
On Sat, Jul 31, 2021 at 10:22 PM Hugh Dickins [off-list ref] wrote:
quoted
On Fri, 30 Jul 2021, Yang Shi wrote:
quoted
On Fri, Jul 30, 2021 at 12:42 AM Hugh Dickins [off-list ref] wrote:
quoted
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.
The small page could be collapsed to a huge page sooner or later, so
the mischarge may be transient. But huge page can't be replaced.
You're right, if all goes well, the mischarged small page could be
collapsed to a correctly charged huge page sooner or later (but all
may not go well), whereas the mischarged huge page is stuck there.
quoted
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
OK, if so the mischarge is not as bad as what I thought in the first place.
quoted
"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.
I agree the race is so unlikely and it may be not worth optimizing
against it right now, but a note or a comment may be worth.
Thanks, but despite us agreeing that the race is too unlikely to be worth
optimizing against, it does still nag at me ever since you questioned it:
silly, but I can't quite be convinced by my own dismissals.
I do still want to get rid of SGP_HUGE and SGP_NOHUGE, clearing up those
huge allocation decisions remains the intention; but now think to add
SGP_NOALLOC for collapse_file() in place of SGP_NOHUGE or SGP_CACHE -
to rule out that possibility of mischarge after racing hole-punch,
no matter whether it's huge or small. If any such race occurs,
collapse_file() should just give up.
This being the "Stupid me" SGP_READ idea, except that of course would
not work: because half the point of that block in collapse_file() is
to initialize the !Uptodate pages, whereas SGP_READ avoids doing so.
There is, of course, the danger that in fixing this unlikely mischarge,
I've got the code wrong and am introducing a bug: here's what a 17/16
would look like, though it will be better inserted early. I got sick
of all the "if (page "s, and was glad of the opportunity to fix that
outdated "bring it back from swap" comment - swap got done above.
What do you think? Should I add this in or leave it out?
Thanks,
Hugh
@@ -108,6 +108,7 @@ extern unsigned long shmem_partial_swap_usage(struct address_space *mapping,/* Flag allocation requirements to shmem_getpage */enumsgp_type{SGP_READ,/* don't exceed i_size, don't allocate page */+SGP_NOALLOC,/* like SGP_READ, but do use fallocated page */SGP_CACHE,/* don't exceed i_size, may allocate page */SGP_WRITE,/* may exceed i_size, may allocate !Uptodate page */SGP_FALLOC,/* like SGP_WRITE, but make existing page Uptodate */---a/mm/khugepaged.c+++b/mm/khugepaged.c
@@ -1721,7 +1721,7 @@ static void collapse_file(struct mm_struct *mm,xas_unlock_irq(&xas);/* swap in or instantiate fallocated page */if(shmem_getpage(mapping->host,index,&page,-SGP_CACHE)){+SGP_NOALLOC)){result=SCAN_FAIL;gotoxa_unlocked;}---a/mm/shmem.c+++b/mm/shmem.c
On Fri, Jul 30, 2021 at 12:55:22AM -0700, Hugh Dickins wrote:
quoted
A new uapi to lock the files on tmpfs in memory, to protect against swap
without mapping the files. This commit introduces two new commands to
fcntl and shmem: F_MEM_LOCK and F_MEM_UNLOCK. The locking will be
charged against RLIMIT_MEMLOCK of uid in namespace of the caller.
It's not clear to me why this is limited to shmfs. Would it not also
make sense for traditional filesystems, eg to force chrome's text pages
to stay in the page cache, no matter how much memory the tabs allocate?
Right: if VFS people would like this to be available for all filesystems,
that's fine by me - it's just that we have not given thought to other
filesystems, and the demand was for tmpfs, so that was where to start.
I'm more confident adding fields to shmem inode than to generic inode.
(Plus tmpfs does have a stronger claim on CAP_IPC_LOCK etc, but there's
no real reason why that cannot be extended to similar use by other FSs).
hugetlbfs and ramfs, where the files are already memlocked? Not worth a
special case, I think: if someone uses up memlock quota on them, so be it.
It looks as if tmpfs would still want its own special case, just to
handle the FALLOC_FL_KEEP_SIZE issue (see 12/16): tmpfs has beyond-i_size
pages in memory, but accounts them evictable; whereas I doubt any storage
filesystems would be using memory for them.
To be clear: I'm not intending to extend this to other filesystems at
the moment; but happy to do so if that's the consensus.
Hugh
From: Kirill A. Shutemov <hidden> Date: 2021-08-04 14:03:42
On Fri, Jul 30, 2021 at 12:45:49AM -0700, Hugh Dickins wrote:
Commit 749df87bd7be ("mm/shmem: add hugetlbfs support to memfd_create()")
in 4.14 added the MFD_HUGETLB flag to memfd_create(), to use hugetlbfs
pages instead of tmpfs pages: now add the MFD_HUGEPAGE flag, to use tmpfs
Transparent Huge Pages when they can be allocated (flag named to follow
the precedent of madvise's MADV_HUGEPAGE for THPs).
I don't like the interface. THP supposed to be transparent, not yet another
hugetlbs.
/sys/kernel/mm/transparent_hugepage/shmem_enabled "always" or "force"
already made this possible: but that is much too blunt an instrument,
affecting all the very different kinds of files on the internal shmem
mount, and was intended just for ease of testing hugepage loads.
I wounder if your tried "always" in production? What breaks? Maybe we can
make it work with a heuristic? This would speed up adoption.
If a tunable needed, I would rather go with fadvise(). It would operate on
a couple of bits per struct file and they get translated into VM_HUGEPAGE
and VM_NOHUGEPAGE on mmap().
Later if needed fadvise() implementation may be extended to track
requested ranges. But initially it can be simple.
--
Kirill A. Shutemov
From: Kirill A. Shutemov <hidden> Date: 2021-08-04 14:08:05
On Fri, Jul 30, 2021 at 12:48:33AM -0700, Hugh Dickins wrote:
Add support for fcntl(fd, F_HUGEPAGE) and fcntl(fd, F_NOHUGEPAGE), to
select hugeness per file: useful to override the default hugeness of the
shmem mount, when occasionally needing to store a hugepage file in a
smallpage mount or vice versa.
Hm. But why is the new MFD_* needed if the fcntl() can do the same.
These fcntls just specify whether or not to try for huge pages when
allocating to the object later: F_HUGEPAGE does not touch small pages
already allocated (though khugepaged may do so when the file is mapped
afterwards), F_NOHUGEPAGE does not split huge pages already allocated.
Why fcntl? Because it's already in use (for sealing) on memfds; and I'm
anxious to keep this simple, just applying it to whole files: fallocate,
madvise and posix_fadvise each involve a range, which would need a new
kind of tree attached to the inode for proper support.
Most of fadvise() operations ignore the range. I like fadvise() because
it's less prescriptive: kernel is free to ignore it.
--
Kirill A. Shutemov
On Wed, Aug 4, 2021 at 1:28 AM Hugh Dickins [off-list ref] wrote:
On Mon, 2 Aug 2021, Yang Shi wrote:
quoted
On Sat, Jul 31, 2021 at 10:22 PM Hugh Dickins [off-list ref] wrote:
quoted
On Fri, 30 Jul 2021, Yang Shi wrote:
quoted
On Fri, Jul 30, 2021 at 12:42 AM Hugh Dickins [off-list ref] wrote:
quoted
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.
The small page could be collapsed to a huge page sooner or later, so
the mischarge may be transient. But huge page can't be replaced.
You're right, if all goes well, the mischarged small page could be
collapsed to a correctly charged huge page sooner or later (but all
may not go well), whereas the mischarged huge page is stuck there.
quoted
quoted
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
OK, if so the mischarge is not as bad as what I thought in the first place.
quoted
"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.
I agree the race is so unlikely and it may be not worth optimizing
against it right now, but a note or a comment may be worth.
Thanks, but despite us agreeing that the race is too unlikely to be worth
optimizing against, it does still nag at me ever since you questioned it:
silly, but I can't quite be convinced by my own dismissals.
I do still want to get rid of SGP_HUGE and SGP_NOHUGE, clearing up those
huge allocation decisions remains the intention; but now think to add
SGP_NOALLOC for collapse_file() in place of SGP_NOHUGE or SGP_CACHE -
to rule out that possibility of mischarge after racing hole-punch,
no matter whether it's huge or small. If any such race occurs,
collapse_file() should just give up.
This being the "Stupid me" SGP_READ idea, except that of course would
not work: because half the point of that block in collapse_file() is
to initialize the !Uptodate pages, whereas SGP_READ avoids doing so.
There is, of course, the danger that in fixing this unlikely mischarge,
I've got the code wrong and am introducing a bug: here's what a 17/16
would look like, though it will be better inserted early. I got sick
of all the "if (page "s, and was glad of the opportunity to fix that
outdated "bring it back from swap" comment - swap got done above.
What do you think? Should I add this in or leave it out?
Thanks for keeping investigating this. The patch looks good to me. I
think we could go this way. Just a nit below.
@@ -108,6 +108,7 @@ extern unsigned long shmem_partial_swap_usage(struct address_space *mapping,/* Flag allocation requirements to shmem_getpage */enumsgp_type{SGP_READ,/* don't exceed i_size, don't allocate page */+SGP_NOALLOC,/* like SGP_READ, but do use fallocated page */
The comment looks misleading, it seems SGP_NOALLOC does clear the
Uptodate flag but SGP_READ doesn't. Or it is fine not to distinguish
this difference?
quoted hunk
SGP_CACHE, /* don't exceed i_size, may allocate page */
SGP_WRITE, /* may exceed i_size, may allocate !Uptodate page */
SGP_FALLOC, /* like SGP_WRITE, but make existing page Uptodate */
--- a/mm/khugepaged.c+++ b/mm/khugepaged.c
@@ -1721,7 +1721,7 @@ static void collapse_file(struct mm_struct *mm,xas_unlock_irq(&xas);/* swap in or instantiate fallocated page */if(shmem_getpage(mapping->host,index,&page,-SGP_CACHE)){+SGP_NOALLOC)){result=SCAN_FAIL;gotoxa_unlocked;}---a/mm/shmem.c+++b/mm/shmem.c
On Mon, Aug 2, 2021 at 2:14 PM Yang Shi [off-list ref] wrote:
On Sat, Jul 31, 2021 at 10:22 PM Hugh Dickins [off-list ref] wrote:
quoted
On Fri, 30 Jul 2021, Yang Shi wrote:
quoted
On Fri, Jul 30, 2021 at 12:42 AM Hugh Dickins [off-list ref] wrote:
quoted
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.
The small page could be collapsed to a huge page sooner or later, so
the mischarge may be transient. But huge page can't be replaced.
quoted
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
OK, if so the mischarge is not as bad as what I thought in the first place.
quoted
"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.
I agree the race is so unlikely and it may be not worth optimizing
against it right now, but a note or a comment may be worth.
quoted
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.
Yeah, that is a different problem.
quoted
quoted
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 supposed it is just performance consideration to keep the
allocated huge page, but I'm not sure how much the difference would be
if we remove it (remove the !CONFIG_NUMA path) because the pcp could
cache THP now since Mel's patch 44042b449872 ("mm/page_alloc: allow
high-order pages to be stored on the per-cpu lists").
It seems to provide the similar optimization but in the buddy
allocator layer so that khugepaged doesn't have to maintain its own
implementation.
quoted
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).
IIUC the NUMA page could hold on to one huge page indefinitely. But
I've never seen the BUG personally, so maybe you are right.
By rereading the code, I think you are correct. Both cases do work
correctly without leaking. And the !CONFIG_NUMA case may carry the
huge page indefinitely.
I think it is because khugepaged may collapse memory for another NUMA
node in the next loop, so it doesn't make too much sense to carry the
huge page, but it may be an optimization for !CONFIG_NUMA case.
However, as I mentioned in earlier email the new pcp implementation
could cache THP now, so we might not need keep this convoluted logic
anymore. Just free the page if collapse is failed then re-allocate
THP. The carried THP might improve the success rate a little bit but I
doubt how noticeable it would be, may be not worth for the extra
complexity at all.
quoted
Collapse failure is not uncommon and leaking huge pages gets noticed.
Hugh
On Fri, Jul 30, 2021 at 12:45:49AM -0700, Hugh Dickins wrote:
quoted
Commit 749df87bd7be ("mm/shmem: add hugetlbfs support to memfd_create()")
in 4.14 added the MFD_HUGETLB flag to memfd_create(), to use hugetlbfs
pages instead of tmpfs pages: now add the MFD_HUGEPAGE flag, to use tmpfs
Transparent Huge Pages when they can be allocated (flag named to follow
the precedent of madvise's MADV_HUGEPAGE for THPs).
I don't like the interface. THP supposed to be transparent, not yet another
hugetlbs.
THP is transparent in the sense that it builds hugepages from the
normal page pool, when it can (or not when it cannot), rather than
promising hugepages from a separate pre-reserved hugetlbfs pool.
Not transparent in the sense that it cannot be limited or guided.
quoted
/sys/kernel/mm/transparent_hugepage/shmem_enabled "always" or "force"
already made this possible: but that is much too blunt an instrument,
affecting all the very different kinds of files on the internal shmem
mount, and was intended just for ease of testing hugepage loads.
I wounder if your tried "always" in production? What breaks? Maybe we can
make it work with a heuristic? This would speed up adoption.
We have not tried /sys/kernel/mm/transparent_hugepage/shmem_enabled
"always" in production. Is that an experiment I want to recommend for
production? No, I don't think so! Why should we?
I am not looking to "speed up adoption" of huge tmpfs everywhere:
let those who find it useful use it, there is no need for it to be
used everywhere.
We have had this disagreement before: you were aiming for tmpfs on /tmp
huge=always, I didn't see the need for that; but we have always agreed
that it should not be broken there, and the better it works the better -
you did the unused_huge_shrink stuff in particular to meet such cases.
If a tunable needed, I would rather go with fadvise(). It would operate on
a couple of bits per struct file and they get translated into VM_HUGEPAGE
and VM_NOHUGEPAGE on mmap().
Later if needed fadvise() implementation may be extended to track
requested ranges. But initially it can be simple.
Let me shift that to the 08/16 (fcntl) response, and here answer:
Hm, But why is the MFD_* needed if the fcntl() can do the same.
You're right, MFD_HUGEPAGE (and MFD_MEM_LOCK) are not strictly
needed if there's an fcntl() or fadvise() which can do that too.
But MFD_HUGEPAGE is the option which was first asked for, and is
the most popular usage internally - I did the fcntl at the same time,
and it has been found useful, but MFD_HUGEPAGE was the priority
(largely because fiddling with shmem_enabled interferes with
everyone's different usages, whereas huge=always on a mount
can be deployed selectively).
And it makes good sense for memfd_create() to offer MFD_HUGEPAGE,
as it is already offering MFD_HUGETLB: when we document MFD_HUGEPAGE
next to MFD_HUGETLB in the memfd_create(2) man page, that will help
developers to make a good choice.
(You said MFD_*, so I take it that you're thinking of MFD_MEM_LOCK
too: MFD_MEM_LOCK is something I added when building this series,
when I realized that it became possible once size change permitted.
Nobody here is using it yet, I don't mind if it's dropped; but it's
natural to propose it as part of the series, and it can be justified
as offering the memlock option which MFD_HUGETLB already bundles in.)
Hugh
On Fri, Jul 30, 2021 at 12:48:33AM -0700, Hugh Dickins wrote:
quoted
Add support for fcntl(fd, F_HUGEPAGE) and fcntl(fd, F_NOHUGEPAGE), to
select hugeness per file: useful to override the default hugeness of the
shmem mount, when occasionally needing to store a hugepage file in a
smallpage mount or vice versa.
Hm. But why is the new MFD_* needed if the fcntl() can do the same.
That I've just addressed in the MFD_HUGEPAGE 07/16 thread.
quoted
These fcntls just specify whether or not to try for huge pages when
allocating to the object later: F_HUGEPAGE does not touch small pages
already allocated (though khugepaged may do so when the file is mapped
afterwards), F_NOHUGEPAGE does not split huge pages already allocated.
Why fcntl? Because it's already in use (for sealing) on memfds; and I'm
anxious to keep this simple, just applying it to whole files: fallocate,
madvise and posix_fadvise each involve a range, which would need a new
kind of tree attached to the inode for proper support.
Most of fadvise() operations ignore the range. I like fadvise() because
it's less prescriptive: kernel is free to ignore it.
As to ignoring the range, yes, I see now that some do; and I'm relieved
to see "Len == 0 means as much as possible", that's great, I was afraid
of compat bugs over 0xffy numbers for the len. And we would want, not
to ignore the range, but insist on offset 0, len 0 for now, if there's
any intention (not mine) of extending it to ranges in the future.
As to ignoring the prescription, that's just a matter of how we describe
it in the manpage, no matter whether it's fadvise() or fcntl().
And in the 07/16 thread you also said:
If a tunable needed, I would rather go with fadvise(). It would operate on
a couple of bits per struct file and they get translated into VM_HUGEPAGE
and VM_NOHUGEPAGE on mmap().
Not so sure about that detail: the point here is to decide what kind
of allocations to try for, before the file is mmap()ed; and it is the
file (the underlying object) that I want to condition here, rather than
the struct file of who has it open at the time, or their mmap()s.
But adding the flags into the vm_flags on mmap(): that's an interesting
idea, I haven't played with that at all. Offhand, I don't think it will
give different allocation results from what I'm already doing, but might
affect what is shown by default in /proc/<pid>/smaps.
Later if needed fadvise() implementation may be extended to track
requested ranges. But initially it can be simple.
I still prefer fcntl() myself, but we can go with either: what I'd
like to hear is the preference of linux-fsdevel and linux-api people.
Aside from the unused offset+len, my main problem with fadvise()
is that... it doesn't exist. It's posix_fadvise() or fadvise64() or
fadvise64_64(), and all its good advices are POSIX_MADV_whatever.
Are we comfortable now adding LINUX_MADV_HUGEPAGE, LINUX_MADV_NOHUGEPAGE?
I find myself singing 64 64 Zoo Lane.
Hugh
On Wed, Aug 4, 2021 at 1:28 AM Hugh Dickins [off-list ref] wrote:
quoted
Thanks, but despite us agreeing that the race is too unlikely to be worth
optimizing against, it does still nag at me ever since you questioned it:
silly, but I can't quite be convinced by my own dismissals.
I do still want to get rid of SGP_HUGE and SGP_NOHUGE, clearing up those
huge allocation decisions remains the intention; but now think to add
SGP_NOALLOC for collapse_file() in place of SGP_NOHUGE or SGP_CACHE -
to rule out that possibility of mischarge after racing hole-punch,
no matter whether it's huge or small. If any such race occurs,
collapse_file() should just give up.
This being the "Stupid me" SGP_READ idea, except that of course would
not work: because half the point of that block in collapse_file() is
to initialize the !Uptodate pages, whereas SGP_READ avoids doing so.
There is, of course, the danger that in fixing this unlikely mischarge,
I've got the code wrong and am introducing a bug: here's what a 17/16
would look like, though it will be better inserted early. I got sick
of all the "if (page "s, and was glad of the opportunity to fix that
outdated "bring it back from swap" comment - swap got done above.
What do you think? Should I add this in or leave it out?
Thanks for keeping investigating this. The patch looks good to me. I
think we could go this way. Just a nit below.
Thanks, I'll add it into the series, a patch before SGP_NOHUGE goes away;
but I'm not intending to respin the series until there's more feedback
from others - fcntl versus fadvise is the main issue so far.
@@ -108,6 +108,7 @@ extern unsigned long shmem_partial_swap_usage(struct address_space *mapping,/* Flag allocation requirements to shmem_getpage */enumsgp_type{SGP_READ,/* don't exceed i_size, don't allocate page */+SGP_NOALLOC,/* like SGP_READ, but do use fallocated page */
The comment looks misleading, it seems SGP_NOALLOC does clear the
Uptodate flag but SGP_READ doesn't. Or it is fine not to distinguish
this difference?
I think you meant to say, SGP_NOALLOC does *set* the Uptodate flag but
SGP_READ doesn't. And a more significant difference, as coded to suit
collapse_file(), is that SGP_NOALLOC returns failure on hole, whereas
SGP_READ returns success: I should have mentioned that.
When I wrote "like SGP_READ" there, I just meant "like what's said in
the line above": would "ditto" be okay with you, and I say
SGP_NOALLOC, /* ditto, but fail on hole, or use fallocated page */
I don't really want to get into the "Uptodate" business there.
And I'm afraid someone is going to ask me to write multi-line comments
on each of those SGP_flags, and I'm going to plead "read the source"!
Oh, now I see why you said SGP_NOALLOC does clear the Uptodate flag:
"goto clear", haha: when we clear the page we set the Uptodate flag.
And I may have another patch to slot in: I was half expecting you to
question why SGP_READ behaves as it does, so in preparing its defence
I checked, and found it was not doing quite what I remembered: changes
were made a long time ago, which have left it slightly suboptimal.
But that really has nothing to do with the rest of this series,
and I don't need to run it past you before reposting.
I hope that some of the features in this series can be useful to you.
Thanks,
Hugh
By rereading the code, I think you are correct. Both cases do work
correctly without leaking. And the !CONFIG_NUMA case may carry the
huge page indefinitely.
I think it is because khugepaged may collapse memory for another NUMA
node in the next loop, so it doesn't make too much sense to carry the
huge page, but it may be an optimization for !CONFIG_NUMA case.
Yes, that is its intention.
However, as I mentioned in earlier email the new pcp implementation
could cache THP now, so we might not need keep this convoluted logic
anymore. Just free the page if collapse is failed then re-allocate
THP. The carried THP might improve the success rate a little bit but I
doubt how noticeable it would be, may be not worth for the extra
complexity at all.
It would be great if the new pcp implementation is good enough to
get rid of khugepaged's confusing NUMA=y/NUMA=n differences; and all
the *hpage stuff too, I hope. That would be a welcome cleanup.
quoted
quoted
Collapse failure is not uncommon and leaking huge pages gets noticed.
After writing that, I realized how I'm almost always testing a NUMA=y
kernel (though on non-NUMA machines), and seldom try the NUMA=n build.
So did so to check no leak, indeed; but was surprised, when comparing
vmstats, that the NUMA=n run had done 5 times as much thp_collapse_alloc
as the NUMA=y run. I've merely made a note to look into that one day:
maybe it was just a one-off oddity, or maybe the incrementing of stats
is wrong down one path or the other.
Hugh
On Thu, Aug 5, 2021 at 10:21 PM Hugh Dickins [off-list ref] wrote:
On Wed, 4 Aug 2021, Yang Shi wrote:
quoted
On Wed, Aug 4, 2021 at 1:28 AM Hugh Dickins [off-list ref] wrote:
quoted
Thanks, but despite us agreeing that the race is too unlikely to be worth
optimizing against, it does still nag at me ever since you questioned it:
silly, but I can't quite be convinced by my own dismissals.
I do still want to get rid of SGP_HUGE and SGP_NOHUGE, clearing up those
huge allocation decisions remains the intention; but now think to add
SGP_NOALLOC for collapse_file() in place of SGP_NOHUGE or SGP_CACHE -
to rule out that possibility of mischarge after racing hole-punch,
no matter whether it's huge or small. If any such race occurs,
collapse_file() should just give up.
This being the "Stupid me" SGP_READ idea, except that of course would
not work: because half the point of that block in collapse_file() is
to initialize the !Uptodate pages, whereas SGP_READ avoids doing so.
There is, of course, the danger that in fixing this unlikely mischarge,
I've got the code wrong and am introducing a bug: here's what a 17/16
would look like, though it will be better inserted early. I got sick
of all the "if (page "s, and was glad of the opportunity to fix that
outdated "bring it back from swap" comment - swap got done above.
What do you think? Should I add this in or leave it out?
Thanks for keeping investigating this. The patch looks good to me. I
think we could go this way. Just a nit below.
Thanks, I'll add it into the series, a patch before SGP_NOHUGE goes away;
but I'm not intending to respin the series until there's more feedback
from others - fcntl versus fadvise is the main issue so far.
@@ -108,6 +108,7 @@ extern unsigned long shmem_partial_swap_usage(struct address_space *mapping,/* Flag allocation requirements to shmem_getpage */enumsgp_type{SGP_READ,/* don't exceed i_size, don't allocate page */+SGP_NOALLOC,/* like SGP_READ, but do use fallocated page */
The comment looks misleading, it seems SGP_NOALLOC does clear the
Uptodate flag but SGP_READ doesn't. Or it is fine not to distinguish
this difference?
I think you meant to say, SGP_NOALLOC does *set* the Uptodate flag but
SGP_READ doesn't. And a more significant difference, as coded to suit
collapse_file(), is that SGP_NOALLOC returns failure on hole, whereas
SGP_READ returns success: I should have mentioned that.
Yes, I mean "set". Sorry for the confusion.
When I wrote "like SGP_READ" there, I just meant "like what's said in
the line above": would "ditto" be okay with you, and I say
SGP_NOALLOC, /* ditto, but fail on hole, or use fallocated page */
I don't really want to get into the "Uptodate" business there.
And I'm afraid someone is going to ask me to write multi-line comments
on each of those SGP_flags, and I'm going to plead "read the source"!
OK, I'm fine as is.
Oh, now I see why you said SGP_NOALLOC does clear the Uptodate flag:
"goto clear", haha: when we clear the page we set the Uptodate flag.
And I may have another patch to slot in: I was half expecting you to
question why SGP_READ behaves as it does, so in preparing its defence
I checked, and found it was not doing quite what I remembered: changes
were made a long time ago, which have left it slightly suboptimal.
But that really has nothing to do with the rest of this series,
and I don't need to run it past you before reposting.
I hope that some of the features in this series can be useful to you.
On Thu, Aug 5, 2021 at 10:43 PM Hugh Dickins [off-list ref] wrote:
On Thu, 5 Aug 2021, Yang Shi wrote:
quoted
By rereading the code, I think you are correct. Both cases do work
correctly without leaking. And the !CONFIG_NUMA case may carry the
huge page indefinitely.
I think it is because khugepaged may collapse memory for another NUMA
node in the next loop, so it doesn't make too much sense to carry the
huge page, but it may be an optimization for !CONFIG_NUMA case.
Yes, that is its intention.
quoted
However, as I mentioned in earlier email the new pcp implementation
could cache THP now, so we might not need keep this convoluted logic
anymore. Just free the page if collapse is failed then re-allocate
THP. The carried THP might improve the success rate a little bit but I
doubt how noticeable it would be, may be not worth for the extra
complexity at all.
It would be great if the new pcp implementation is good enough to
get rid of khugepaged's confusing NUMA=y/NUMA=n differences; and all
the *hpage stuff too, I hope. That would be a welcome cleanup.
The other question is if that optimization is worth it nowadays or
not. I bet not too many users build NUMA=n kernel nowadays even though
the kernel is actually running on a non-NUMA machine. Some small
devices may run NUMA=n kernel, but I don't think they actually use
THP. So such code complexity could be removed from this point of view
too.
quoted
quoted
quoted
Collapse failure is not uncommon and leaking huge pages gets noticed.
After writing that, I realized how I'm almost always testing a NUMA=y
kernel (though on non-NUMA machines), and seldom try the NUMA=n build.
So did so to check no leak, indeed; but was surprised, when comparing
vmstats, that the NUMA=n run had done 5 times as much thp_collapse_alloc
as the NUMA=y run. I've merely made a note to look into that one day:
maybe it was just a one-off oddity, or maybe the incrementing of stats
is wrong down one path or the other.
On Fri, Aug 6, 2021 at 10:57 AM Yang Shi [off-list ref] wrote:
On Thu, Aug 5, 2021 at 10:43 PM Hugh Dickins [off-list ref] wrote:
quoted
On Thu, 5 Aug 2021, Yang Shi wrote:
quoted
By rereading the code, I think you are correct. Both cases do work
correctly without leaking. And the !CONFIG_NUMA case may carry the
huge page indefinitely.
I think it is because khugepaged may collapse memory for another NUMA
node in the next loop, so it doesn't make too much sense to carry the
huge page, but it may be an optimization for !CONFIG_NUMA case.
Yes, that is its intention.
quoted
However, as I mentioned in earlier email the new pcp implementation
could cache THP now, so we might not need keep this convoluted logic
anymore. Just free the page if collapse is failed then re-allocate
THP. The carried THP might improve the success rate a little bit but I
doubt how noticeable it would be, may be not worth for the extra
complexity at all.
It would be great if the new pcp implementation is good enough to
get rid of khugepaged's confusing NUMA=y/NUMA=n differences; and all
the *hpage stuff too, I hope. That would be a welcome cleanup.
The other question is if that optimization is worth it nowadays or
not. I bet not too many users build NUMA=n kernel nowadays even though
the kernel is actually running on a non-NUMA machine. Some small
devices may run NUMA=n kernel, but I don't think they actually use
THP. So such code complexity could be removed from this point of view
too.
quoted
quoted
quoted
quoted
Collapse failure is not uncommon and leaking huge pages gets noticed.
After writing that, I realized how I'm almost always testing a NUMA=y
kernel (though on non-NUMA machines), and seldom try the NUMA=n build.
So did so to check no leak, indeed; but was surprised, when comparing
vmstats, that the NUMA=n run had done 5 times as much thp_collapse_alloc
as the NUMA=y run. I've merely made a note to look into that one day:
maybe it was just a one-off oddity, or maybe the incrementing of stats
is wrong down one path or the other.
I came up with a patch to remove !CONFIG_NUMA case, and my test found
the same problem. NUMA=n run had done 5 times as much
thp_collapse_alloc as NUMA=y run with vanilla kernel just exactly as
what you saw.
A quick look shows the huge page allocation timing is different for
the two cases. For NUMA=n, the huge page is allocated by
khugepaged_prealloc_page() before scanning the address space, so it
means huge page may be allocated even though there is no suitable
range for collapsing. Then the page would be just freed if khugepaged
already made enough progress then try to reallocate again. The problem
should be more noticeable if you have a shorter scan interval
(scan_sleep_millisecs). I set it to 100ms for my test.
We could carry the huge page across scan passes for NUMA=n, but this
would make the code more complicated. I don't think it is really
worth, so just removing the special case for NUMA=n sounds more
reasonable to me.