| Message ID | c140f56a-1aa3-f7ae-b7d1-93da7d5a3572@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tmpfs: HUGEPAGE and MEM_LOCK fcntls and memfds | expand |
Hi Hugh,
I love your patch! Perhaps something to improve:
[auto build test WARNING on hch-configfs/for-next]
[also build test WARNING on linus/master v5.14-rc3 next-20210729]
[cannot apply to hnaz-linux-mm/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Hugh-Dickins/tmpfs-HUGEPAGE-and-MEM_LOCK-fcntls-and-memfds/20210730-161413
base: git://git.infradead.org/users/hch/configfs.git for-next
config: ia64-randconfig-r016-20210730 (attached as .config)
compiler: ia64-linux-gcc (GCC) 10.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/7d2aab7d716c95551b2bcab51ca8ff33c2d1dd58
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Hugh-Dickins/tmpfs-HUGEPAGE-and-MEM_LOCK-fcntls-and-memfds/20210730-161413
git checkout 7d2aab7d716c95551b2bcab51ca8ff33c2d1dd58
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from arch/ia64/include/asm/pgtable.h:153,
from include/linux/pgtable.h:6,
from arch/ia64/include/asm/uaccess.h:40,
from include/linux/uaccess.h:11,
from include/linux/sched/task.h:11,
from include/linux/sched/signal.h:9,
from include/linux/rcuwait.h:6,
from include/linux/percpu-rwsem.h:7,
from include/linux/fs.h:33,
from mm/shmem.c:24:
arch/ia64/include/asm/mmu_context.h: In function 'reload_context':
arch/ia64/include/asm/mmu_context.h:127:41: warning: variable 'old_rr4' set but not used [-Wunused-but-set-variable]
127 | unsigned long rr0, rr1, rr2, rr3, rr4, old_rr4;
| ^~~~~~~
mm/shmem.c: At top level:
>> mm/shmem.c:695:6: warning: no previous prototype for 'transparent_hugepage_allowed' [-Wmissing-prototypes]
695 | bool transparent_hugepage_allowed(void)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/transparent_hugepage_allowed +695 mm/shmem.c
694
> 695 bool transparent_hugepage_allowed(void)
696 {
697 return false;
698 }
699
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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.
On Wed, 4 Aug 2021, Kirill A. Shutemov wrote: > 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. 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. > > > /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
diff --git a/include/uapi/linux/memfd.h b/include/uapi/linux/memfd.h index 7a8a26751c23..8358a69e78cc 100644 --- a/include/uapi/linux/memfd.h +++ b/include/uapi/linux/memfd.h @@ -7,7 +7,8 @@ /* flags for memfd_create(2) (unsigned int) */ #define MFD_CLOEXEC 0x0001U #define MFD_ALLOW_SEALING 0x0002U -#define MFD_HUGETLB 0x0004U +#define MFD_HUGETLB 0x0004U /* Use hugetlbfs */ +#define MFD_HUGEPAGE 0x0008U /* Use huge tmpfs */ /* * Huge page size encoding when MFD_HUGETLB is specified, and a huge page diff --git a/mm/memfd.c b/mm/memfd.c index 081dd33e6a61..0d1a504d2fc9 100644 --- a/mm/memfd.c +++ b/mm/memfd.c @@ -245,7 +245,10 @@ long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg) #define MFD_NAME_PREFIX_LEN (sizeof(MFD_NAME_PREFIX) - 1) #define MFD_NAME_MAX_LEN (NAME_MAX - MFD_NAME_PREFIX_LEN) -#define MFD_ALL_FLAGS (MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_HUGETLB) +#define MFD_ALL_FLAGS (MFD_CLOEXEC | \ + MFD_ALLOW_SEALING | \ + MFD_HUGETLB | \ + MFD_HUGEPAGE) SYSCALL_DEFINE2(memfd_create, const char __user *, uname, @@ -257,14 +260,17 @@ SYSCALL_DEFINE2(memfd_create, char *name; long len; - if (!(flags & MFD_HUGETLB)) { - if (flags & ~(unsigned int)MFD_ALL_FLAGS) + if (flags & MFD_HUGETLB) { + /* Disallow huge tmpfs when choosing hugetlbfs */ + if (flags & MFD_HUGEPAGE) return -EINVAL; - } else { /* Allow huge page size encoding in flags. */ if (flags & ~(unsigned int)(MFD_ALL_FLAGS | (MFD_HUGE_MASK << MFD_HUGE_SHIFT))) return -EINVAL; + } else { + if (flags & ~(unsigned int)MFD_ALL_FLAGS) + return -EINVAL; } /* length includes terminating zero */ @@ -303,8 +309,14 @@ SYSCALL_DEFINE2(memfd_create, HUGETLB_ANONHUGE_INODE, (flags >> MFD_HUGE_SHIFT) & MFD_HUGE_MASK); - } else - file = shmem_file_setup(name, 0, VM_NORESERVE); + } else { + unsigned long vm_flags = VM_NORESERVE; + + if (flags & MFD_HUGEPAGE) + vm_flags |= VM_HUGEPAGE; + file = shmem_file_setup(name, 0, vm_flags); + } + if (IS_ERR(file)) { error = PTR_ERR(file); goto err_fd; diff --git a/mm/shmem.c b/mm/shmem.c index 6def7391084c..e2bcf3313686 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -476,6 +476,20 @@ static bool shmem_confirm_swap(struct address_space *mapping, static int shmem_huge __read_mostly = SHMEM_HUGE_NEVER; +/* + * Does either /sys/kernel/mm/transparent_hugepage/shmem_enabled or + * /sys/kernel/mm/transparent_hugepage/enabled allow transparent hugepages? + * (Can only return true when the machine has_transparent_hugepage() too.) + */ +static bool transparent_hugepage_allowed(void) +{ + return shmem_huge > SHMEM_HUGE_NEVER || + test_bit(TRANSPARENT_HUGEPAGE_FLAG, + &transparent_hugepage_flags) || + test_bit(TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG, + &transparent_hugepage_flags); +} + bool shmem_is_huge(struct vm_area_struct *vma, struct inode *inode, pgoff_t index) { @@ -486,6 +500,8 @@ bool shmem_is_huge(struct vm_area_struct *vma, if (vma && ((vma->vm_flags & VM_NOHUGEPAGE) || test_bit(MMF_DISABLE_THP, &vma->vm_mm->flags))) return false; + if (SHMEM_I(inode)->flags & VM_HUGEPAGE) + return true; if (shmem_huge == SHMEM_HUGE_FORCE) return true; @@ -676,6 +692,11 @@ static long shmem_unused_huge_count(struct super_block *sb, #define shmem_huge SHMEM_HUGE_DENY +bool transparent_hugepage_allowed(void) +{ + return false; +} + bool shmem_is_huge(struct vm_area_struct *vma, struct inode *inode, pgoff_t index) { @@ -2171,10 +2192,14 @@ unsigned long shmem_get_unmapped_area(struct file *file, if (shmem_huge != SHMEM_HUGE_FORCE) { struct super_block *sb; + struct inode *inode; if (file) { VM_BUG_ON(file->f_op != &shmem_file_operations); - sb = file_inode(file)->i_sb; + inode = file_inode(file); + if (SHMEM_I(inode)->flags & VM_HUGEPAGE) + goto huge; + sb = inode->i_sb; } else { /* * Called directly from mm/mmap.c, or drivers/char/mem.c @@ -2187,7 +2212,7 @@ unsigned long shmem_get_unmapped_area(struct file *file, if (SHMEM_SB(sb)->huge == SHMEM_HUGE_NEVER) return addr; } - +huge: offset = (pgoff << PAGE_SHIFT) & (HPAGE_PMD_SIZE-1); if (offset && offset + len < 2 * HPAGE_PMD_SIZE) return addr; @@ -2308,6 +2333,10 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode atomic_set(&info->stop_eviction, 0); info->seals = F_SEAL_SEAL; info->flags = flags & VM_NORESERVE; + if ((flags & VM_HUGEPAGE) && + transparent_hugepage_allowed() && + !test_bit(MMF_DISABLE_THP, ¤t->mm->flags)) + info->flags |= VM_HUGEPAGE; INIT_LIST_HEAD(&info->shrinklist); INIT_LIST_HEAD(&info->swaplist); simple_xattrs_init(&info->xattrs);
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(-)