* Re: [PATCH v2 1/1] mm/madvise: add MADV_F_COLLAPSE_LIGHT to process_madvise()
From: Lance Yang @ 2024-01-26 10:15 UTC (permalink / raw)
To: akpm, Michal Hocko, Zach O'Keefe, Yang Shi, David Hildenbrand
Cc: songmuchun, peterx, mknyszek, minchan, linux-mm, linux-kernel,
linux-api
In-Reply-To: <CAK1f24nS8MEA3wcS4za-uSp7ZBxvd+xqMRf8-u=m5uCvTs8yJQ@mail.gmail.com>
I would like to correct the information provided in my previous
email and also provide some additional information.
On Fri, Jan 26, 2024 at 2:16 PM Lance Yang <ioworker0@gmail.com> wrote:
>
> I’d like to add another real use case.
>
> In our company, we deploy applications using offline-online
> hybrid deployment. This approach leverages the distinctive
> resource utilization patterns of online services, utilizing idle
> resources during various time periods by filling them with
> offline jobs. This helps reduce the growing cost expenditures
> for the enterprise.
>
> Whether for online services or offline jobs, their requirements
> for THP can be roughly categorized into three types:
>
> * The first type aims to use huge pages as much as possible
> and tolerates unpredictable stalls caused by direct reclaim
> and/or compaction.
> * The second type attempts to use huge pages but is relatively
> latency-sensitive and cannot tolerate unpredictable stalls.
> * The third type prefers not to use huge pages at all and is
> extremely latency-sensitive.
>
> After careful consideration, we decided to prioritize the
> requirements of the first type and modify the THP settings
> as follows:
>
> echo madvise >/sys/kernel/mm/transparent_hugepage/enabled
> echo defer >/sys/kernel/mm/transparent_hugepage/defrag
>
> With the introduction of MADV_COLLAPSE into the kernel,
> it is no longer dependent on any sysfs setting under
> /sys/kernel/mm/transparent_hugepage. MADV_COLLAPSE
> offers the potential for fine-grained synchronous control over
> the huge page allocation mechanism, marking a significant
> enhancement for THP.
>
> If the kernel supports a more relaxed (opportunistic)
> MADV_COLLAPSE, we will modify the THP settings as follows:
>
> echo madvise >/sys/kernel/mm/transparent_hugepage/enabled
> echo madvise >/sys/kernel/mm/transparent_hugepage/defrag
The correct THP settings should be:
echo always >/sys/kernel/mm/transparent_hugepage/enabled
echo madvise >/sys/kernel/mm/transparent_hugepage/defrag
>
> Then, we will use process_madvise(MADV_COLLAPSE, xx_relaxed_flag)
> to address the requirements of the second type.
>
> Why don't we favor madvise(MADV_COLLAPSE) for the first type
> of requirements?
> The main reason is that these requirements are typically for offline
> jobs in the Hadoop ecosystem, such as MapReduce and Spark,
> which run primarily on the JVM. IIRC, the JVM currently does not
> support madvise(MADV_COLLAPSE). The second type of
To add, there are also some offline jobs that rely on PyTorch for
machine learning model training tasks. IIRC, PyTorch also does
not support madvise(MADV_COLLAPSE).
Thanks,
Lance
> requirements is all for our in-house developed online services.
> For us, integrating a more relaxed (opportunistic)
> MADV_COLLAPSE into our online services is relatively
> straightforward.
>
> By introducing various flags to MADV_COLLAPSE, we can offer
> multiple synchronous allocation strategies for applications. This
> fine-grained control may be more suitable for cloud-native
> environments than the widespread settings under
> /sys/kernel/mm/transparent_hugepage in sysfs.
>
> Thanks for your time!
> Lance
>
> On Sun, Jan 21, 2024 at 11:12 AM Lance Yang <ioworker0@gmail.com> wrote:
> >
> > Hello Everyone,
> >
> > For applications actively utilizing THP, the defrag mode may
> > not be a very user-friendly design. Here are the reasons:
> > 1. Before marking the address space with
> > MADV_HUGEPAGE,it is necessary to check if
> > the current configuration of the defrag mode aligns with
> > their preferences.
> > 2. Once the defrag mode configuration changes, these
> > applications may face the risk of unpredictable stalls.
> >
> > THP is an important feature of the Linux kernel that can
> > significantly enhance memory access performance.
> > However, due to the lack of fine-grained control over
> > the huge page allocation strategy, many applications
> > default to not using huge pages and even recommend
> > users to disable THP. This situation is regrettable.
> >
> > With the introduction of MADV_COLLAPSE into the kernel,
> > it is not affected by the defrag mode.
> > MADV_COLLAPSE offers the potential for
> > fine-grained synchronous control over the huge page
> > allocation mechanism, marking a significant enhancement
> > for THP.
> >
> > By adding flags to MADV_COLLAPSE, different
> > synchronous allocation strategies can be provided to
> > applications. This can instill confidence in them, allowing
> > them to reconsider using THP and allocate huge pages
> > according to their desired synchronous allocation strategy,
> > without worrying about the defrag mode configuration.
> >
> > BR,
> > Lance
> >
> >
> > On Thu, Jan 18, 2024 at 8:03 PM Lance Yang <ioworker0@gmail.com> wrote:
> > >
> > > This idea was inspired by MADV_COLLAPSE introduced by Zach O'Keefe[1].
> > >
> > > Allow MADV_F_COLLAPSE_LIGHT behavior for process_madvise(2) if the caller
> > > has CAP_SYS_ADMIN or is requesting the collapse of its own memory.
> > >
> > > The semantics of MADV_F_COLLAPSE_LIGHT are similar to MADV_COLLAPSE, but
> > > it avoids direct reclaim and/or compaction, quickly failing on allocation
> > > errors.
> > >
> > > This change enables a more flexible and efficient usage of memory collapse
> > > operations, providing additional control to userspace applications for
> > > system-wide THP optimization.
> > >
> > > Semantics
> > >
> > > This call is independent of the system-wide THP sysfs settings, but will
> > > fail for memory marked VM_NOHUGEPAGE. If the ranges provided span
> > > multiple VMAs, the semantics of the collapse over each VMA is independent
> > > from the others. This implies a hugepage cannot cross a VMA boundary. If
> > > collapse of a given hugepage-aligned/sized region fails, the operation may
> > > continue to attempt collapsing the remainder of memory specified.
> > >
> > > The memory ranges provided must be page-aligned, but are not required to
> > > be hugepage-aligned. If the memory ranges are not hugepage-aligned, the
> > > start/end of the range will be clamped to the first/last hugepage-aligned
> > > address covered by said range. The memory ranges must span at least one
> > > hugepage-sized region.
> > >
> > > All non-resident pages covered by the range will first be
> > > swapped/faulted-in, before being internally copied onto a freshly
> > > allocated hugepage. Unmapped pages will have their data directly
> > > initialized to 0 in the new hugepage. However, for every eligible
> > > hugepage aligned/sized region to-be collapsed, at least one page must
> > > currently be backed by memory (a PMD covering the address range must
> > > already exist).
> > >
> > > Allocation for the new hugepage will not enter direct reclaim and/or
> > > compaction, quickly failing if allocation fails. When the system has
> > > multiple NUMA nodes, the hugepage will be allocated from the node providing
> > > the most native pages. This operation operates on the current state of the
> > > specified process and makes no persistent changes or guarantees on how pages
> > > will be mapped, constructed, or faulted in the future.
> > >
> > > Use Cases
> > >
> > > An immediate user of this new functionality is the Go runtime heap allocator
> > > that manages memory in hugepage-sized chunks. In the past, whether it was a
> > > newly allocated chunk through mmap() or a reused chunk released by
> > > madvise(MADV_DONTNEED), the allocator attempted to eagerly back memory with
> > > huge pages using madvise(MADV_HUGEPAGE)[2] and madvise(MADV_COLLAPSE)[3]
> > > respectively. However, both approaches resulted in performance issues; for
> > > both scenarios, there could be entries into direct reclaim and/or compaction,
> > > leading to unpredictable stalls[4]. Now, the allocator can confidently use
> > > process_madvise(MADV_F_COLLAPSE_LIGHT) to attempt the allocation of huge pages.
> > >
> > > [1] https://github.com/torvalds/linux/commit/7d8faaf155454f8798ec56404faca29a82689c77
> > > [2] https://github.com/golang/go/commit/8fa9e3beee8b0e6baa7333740996181268b60a3a
> > > [3] https://github.com/golang/go/commit/9f9bb26880388c5bead158e9eca3be4b3a9bd2af
> > > [4] https://github.com/golang/go/issues/63334
> > >
> > > [v1] https://lore.kernel.org/lkml/20240117050217.43610-1-ioworker0@gmail.com/
> > >
> > > Signed-off-by: Lance Yang <ioworker0@gmail.com>
> > > Suggested-by: Zach O'Keefe <zokeefe@google.com>
> > > Suggested-by: David Hildenbrand <david@redhat.com>
> > > ---
> > > V1 -> V2: Treat process_madvise(MADV_F_COLLAPSE_LIGHT) as the lighter-weight alternative
> > > to madvise(MADV_COLLAPSE)
> > >
> > > arch/alpha/include/uapi/asm/mman.h | 1 +
> > > arch/mips/include/uapi/asm/mman.h | 1 +
> > > arch/parisc/include/uapi/asm/mman.h | 1 +
> > > arch/xtensa/include/uapi/asm/mman.h | 1 +
> > > include/linux/huge_mm.h | 5 +--
> > > include/uapi/asm-generic/mman-common.h | 1 +
> > > mm/khugepaged.c | 15 ++++++--
> > > mm/madvise.c | 36 +++++++++++++++++---
> > > tools/include/uapi/asm-generic/mman-common.h | 1 +
> > > 9 files changed, 52 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/arch/alpha/include/uapi/asm/mman.h b/arch/alpha/include/uapi/asm/mman.h
> > > index 763929e814e9..22f23ca04f1a 100644
> > > --- a/arch/alpha/include/uapi/asm/mman.h
> > > +++ b/arch/alpha/include/uapi/asm/mman.h
> > > @@ -77,6 +77,7 @@
> > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> > >
> > > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> > >
> > > /* compatibility flags */
> > > #define MAP_FILE 0
> > > diff --git a/arch/mips/include/uapi/asm/mman.h b/arch/mips/include/uapi/asm/mman.h
> > > index c6e1fc77c996..acec0b643e9c 100644
> > > --- a/arch/mips/include/uapi/asm/mman.h
> > > +++ b/arch/mips/include/uapi/asm/mman.h
> > > @@ -104,6 +104,7 @@
> > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> > >
> > > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> > >
> > > /* compatibility flags */
> > > #define MAP_FILE 0
> > > diff --git a/arch/parisc/include/uapi/asm/mman.h b/arch/parisc/include/uapi/asm/mman.h
> > > index 68c44f99bc93..812029c98cd7 100644
> > > --- a/arch/parisc/include/uapi/asm/mman.h
> > > +++ b/arch/parisc/include/uapi/asm/mman.h
> > > @@ -71,6 +71,7 @@
> > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> > >
> > > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> > >
> > > #define MADV_HWPOISON 100 /* poison a page for testing */
> > > #define MADV_SOFT_OFFLINE 101 /* soft offline page for testing */
> > > diff --git a/arch/xtensa/include/uapi/asm/mman.h b/arch/xtensa/include/uapi/asm/mman.h
> > > index 1ff0c858544f..52ef463dd5b6 100644
> > > --- a/arch/xtensa/include/uapi/asm/mman.h
> > > +++ b/arch/xtensa/include/uapi/asm/mman.h
> > > @@ -112,6 +112,7 @@
> > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> > >
> > > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> > >
> > > /* compatibility flags */
> > > #define MAP_FILE 0
> > > diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> > > index 5adb86af35fc..075fdb5d481a 100644
> > > --- a/include/linux/huge_mm.h
> > > +++ b/include/linux/huge_mm.h
> > > @@ -303,7 +303,7 @@ int hugepage_madvise(struct vm_area_struct *vma, unsigned long *vm_flags,
> > > int advice);
> > > int madvise_collapse(struct vm_area_struct *vma,
> > > struct vm_area_struct **prev,
> > > - unsigned long start, unsigned long end);
> > > + unsigned long start, unsigned long end, int behavior);
> > > void vma_adjust_trans_huge(struct vm_area_struct *vma, unsigned long start,
> > > unsigned long end, long adjust_next);
> > > spinlock_t *__pmd_trans_huge_lock(pmd_t *pmd, struct vm_area_struct *vma);
> > > @@ -450,7 +450,8 @@ static inline int hugepage_madvise(struct vm_area_struct *vma,
> > >
> > > static inline int madvise_collapse(struct vm_area_struct *vma,
> > > struct vm_area_struct **prev,
> > > - unsigned long start, unsigned long end)
> > > + unsigned long start, unsigned long end,
> > > + int behavior)
> > > {
> > > return -EINVAL;
> > > }
> > > diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> > > index 6ce1f1ceb432..92c67bc755da 100644
> > > --- a/include/uapi/asm-generic/mman-common.h
> > > +++ b/include/uapi/asm-generic/mman-common.h
> > > @@ -78,6 +78,7 @@
> > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> > >
> > > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> > >
> > > /* compatibility flags */
> > > #define MAP_FILE 0
> > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > > index 2b219acb528e..2840051c0ae2 100644
> > > --- a/mm/khugepaged.c
> > > +++ b/mm/khugepaged.c
> > > @@ -97,6 +97,8 @@ static struct kmem_cache *mm_slot_cache __ro_after_init;
> > > struct collapse_control {
> > > bool is_khugepaged;
> > >
> > > + int behavior;
> > > +
> > > /* Num pages scanned per node */
> > > u32 node_load[MAX_NUMNODES];
> > >
> > > @@ -1058,10 +1060,16 @@ static int __collapse_huge_page_swapin(struct mm_struct *mm,
> > > static int alloc_charge_hpage(struct page **hpage, struct mm_struct *mm,
> > > struct collapse_control *cc)
> > > {
> > > - gfp_t gfp = (cc->is_khugepaged ? alloc_hugepage_khugepaged_gfpmask() :
> > > - GFP_TRANSHUGE);
> > > int node = hpage_collapse_find_target_node(cc);
> > > struct folio *folio;
> > > + gfp_t gfp;
> > > +
> > > + if (cc->is_khugepaged)
> > > + gfp = alloc_hugepage_khugepaged_gfpmask();
> > > + else
> > > + gfp = (cc->behavior == MADV_F_COLLAPSE_LIGHT ?
> > > + GFP_TRANSHUGE_LIGHT :
> > > + GFP_TRANSHUGE);
> > >
> > > if (!hpage_collapse_alloc_folio(&folio, gfp, node, &cc->alloc_nmask)) {
> > > *hpage = NULL;
> > > @@ -2697,7 +2705,7 @@ static int madvise_collapse_errno(enum scan_result r)
> > > }
> > >
> > > int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev,
> > > - unsigned long start, unsigned long end)
> > > + unsigned long start, unsigned long end, int behavior)
> > > {
> > > struct collapse_control *cc;
> > > struct mm_struct *mm = vma->vm_mm;
> > > @@ -2718,6 +2726,7 @@ int madvise_collapse(struct vm_area_struct *vma, struct vm_area_struct **prev,
> > > if (!cc)
> > > return -ENOMEM;
> > > cc->is_khugepaged = false;
> > > + cc->behavior = behavior;
> > >
> > > mmgrab(mm);
> > > lru_add_drain_all();
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index 912155a94ed5..9c40226505aa 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -60,6 +60,7 @@ static int madvise_need_mmap_write(int behavior)
> > > case MADV_POPULATE_READ:
> > > case MADV_POPULATE_WRITE:
> > > case MADV_COLLAPSE:
> > > + case MADV_F_COLLAPSE_LIGHT:
> > > return 0;
> > > default:
> > > /* be safe, default to 1. list exceptions explicitly */
> > > @@ -1082,8 +1083,9 @@ static int madvise_vma_behavior(struct vm_area_struct *vma,
> > > if (error)
> > > goto out;
> > > break;
> > > + case MADV_F_COLLAPSE_LIGHT:
> > > case MADV_COLLAPSE:
> > > - return madvise_collapse(vma, prev, start, end);
> > > + return madvise_collapse(vma, prev, start, end, behavior);
> > > }
> > >
> > > anon_name = anon_vma_name(vma);
> > > @@ -1178,6 +1180,7 @@ madvise_behavior_valid(int behavior)
> > > case MADV_HUGEPAGE:
> > > case MADV_NOHUGEPAGE:
> > > case MADV_COLLAPSE:
> > > + case MADV_F_COLLAPSE_LIGHT:
> > > #endif
> > > case MADV_DONTDUMP:
> > > case MADV_DODUMP:
> > > @@ -1194,6 +1197,17 @@ madvise_behavior_valid(int behavior)
> > > }
> > > }
> > >
> > > +
> > > +static bool process_madvise_behavior_only(int behavior)
> > > +{
> > > + switch (behavior) {
> > > + case MADV_F_COLLAPSE_LIGHT:
> > > + return true;
> > > + default:
> > > + return false;
> > > + }
> > > +}
> > > +
> > > static bool process_madvise_behavior_valid(int behavior)
> > > {
> > > switch (behavior) {
> > > @@ -1201,6 +1215,7 @@ static bool process_madvise_behavior_valid(int behavior)
> > > case MADV_PAGEOUT:
> > > case MADV_WILLNEED:
> > > case MADV_COLLAPSE:
> > > + case MADV_F_COLLAPSE_LIGHT:
> > > return true;
> > > default:
> > > return false;
> > > @@ -1368,6 +1383,8 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> > > * transparent huge pages so the existing pages will not be
> > > * coalesced into THP and new pages will not be allocated as THP.
> > > * MADV_COLLAPSE - synchronously coalesce pages into new THP.
> > > + * MADV_F_COLLAPSE_LIGHT - only for process_madvise, avoids direct reclaim and/or
> > > + * compaction.
> > > * MADV_DONTDUMP - the application wants to prevent pages in the given range
> > > * from being included in its core dump.
> > > * MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
> > > @@ -1394,7 +1411,8 @@ int madvise_set_anon_name(struct mm_struct *mm, unsigned long start,
> > > * -EBADF - map exists, but area maps something that isn't a file.
> > > * -EAGAIN - a kernel resource was temporarily unavailable.
> > > */
> > > -int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior)
> > > +int _do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in,
> > > + int behavior, bool is_process_madvise)
> > > {
> > > unsigned long end;
> > > int error;
> > > @@ -1405,6 +1423,9 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
> > > if (!madvise_behavior_valid(behavior))
> > > return -EINVAL;
> > >
> > > + if (!is_process_madvise && process_madvise_behavior_only(behavior))
> > > + return -EINVAL;
> > > +
> > > if (!PAGE_ALIGNED(start))
> > > return -EINVAL;
> > > len = PAGE_ALIGN(len_in);
> > > @@ -1448,9 +1469,14 @@ int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int beh
> > > return error;
> > > }
> > >
> > > +int do_madvise(struct mm_struct *mm, unsigned long start, size_t len_in, int behavior)
> > > +{
> > > + return _do_madvise(mm, start, len_in, behavior, false);
> > > +}
> > > +
> > > SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > > {
> > > - return do_madvise(current->mm, start, len_in, behavior);
> > > + return _do_madvise(current->mm, start, len_in, behavior, false);
> > > }
> > >
> > > SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> > > @@ -1504,8 +1530,8 @@ SYSCALL_DEFINE5(process_madvise, int, pidfd, const struct iovec __user *, vec,
> > > total_len = iov_iter_count(&iter);
> > >
> > > while (iov_iter_count(&iter)) {
> > > - ret = do_madvise(mm, (unsigned long)iter_iov_addr(&iter),
> > > - iter_iov_len(&iter), behavior);
> > > + ret = _do_madvise(mm, (unsigned long)iter_iov_addr(&iter),
> > > + iter_iov_len(&iter), behavior, true);
> > > if (ret < 0)
> > > break;
> > > iov_iter_advance(&iter, iter_iov_len(&iter));
> > > diff --git a/tools/include/uapi/asm-generic/mman-common.h b/tools/include/uapi/asm-generic/mman-common.h
> > > index 6ce1f1ceb432..92c67bc755da 100644
> > > --- a/tools/include/uapi/asm-generic/mman-common.h
> > > +++ b/tools/include/uapi/asm-generic/mman-common.h
> > > @@ -78,6 +78,7 @@
> > > #define MADV_DONTNEED_LOCKED 24 /* like DONTNEED, but drop locked pages too */
> > >
> > > #define MADV_COLLAPSE 25 /* Synchronous hugepage collapse */
> > > +#define MADV_F_COLLAPSE_LIGHT 26 /* Similar to COLLAPSE, but avoids direct reclaim and/or compaction */
> > >
> > > /* compatibility flags */
> > > #define MAP_FILE 0
> > > --
> > > 2.33.1
> > >
^ permalink raw reply
* Re: [PATCH v2 1/1] mm/madvise: add MADV_F_COLLAPSE_LIGHT to process_madvise()
From: Lance Yang @ 2024-01-26 12:52 UTC (permalink / raw)
To: akpm, Michal Hocko, Zach O'Keefe, Yang Shi, David Hildenbrand
Cc: songmuchun, peterx, mknyszek, minchan, linux-mm, linux-kernel,
linux-api
In-Reply-To: <CAK1f24kOYDOw26ov5TVpAyNP13hCjm=cDo4rooOTPDuv8L6Pnw@mail.gmail.com>
On Fri, Jan 26, 2024 at 6:15 PM Lance Yang <ioworker0@gmail.com> wrote:
[...]
> > If the kernel supports a more relaxed (opportunistic)
> > MADV_COLLAPSE, we will modify the THP settings as follows:
> >
> > echo madvise >/sys/kernel/mm/transparent_hugepage/enabled
> > echo madvise >/sys/kernel/mm/transparent_hugepage/defrag
>
> The correct THP settings should be:
> echo always >/sys/kernel/mm/transparent_hugepage/enabled
> echo madvise >/sys/kernel/mm/transparent_hugepage/defrag
>
Apologize for the confusion in my previous email.
The third type of requirements prefers not to use huge pages at all.
The correct THP settings should be:
echo madvise >/sys/kernel/mm/transparent_hugepage/enabled
echo defer+madvise >/sys/kernel/mm/transparent_hugepage/defrag
> >
> > Then, we will use process_madvise(MADV_COLLAPSE, xx_relaxed_flag)
> > to address the requirements of the second type.
[...]
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Oleg Nesterov @ 2024-01-26 14:33 UTC (permalink / raw)
To: Christian Brauner
Cc: Tycho Andersen, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240126-lokal-aktualisieren-fef41d9bce9f@brauner>
On 01/26, Christian Brauner wrote:
>
> No, it doesn't. I'm trying to understand what you are suggesting though.
> Are you saying !task || tas->exit_state is enough
If PIDFD_THREAD then I think it is enough. Otherwise we still need
!task || (exit_state && thread_group_empty)
> and we shouldn't use
> the helper that was added in commit 38fd525a4c61 ("exit: Factor
> thread_group_exited out of pidfd_poll"). If so what does that buy us
> open-coding the check instead of using that helper? Is there an actual
> bug here?
The patch adds the new xxx_exited(task, excl) helper which checks
!task || (exit_state && (excl || thread_group_empty))
yes, the naming is not good.
> > Well, I didn't say this is a problem. I simply do not know how/why people
> > use pidfd_poll().
>
> Sorry, I just have a hard time understanding what you wanted then. :)
>
> "I guess it is too late to change this behavior." made it sound like a)
> there's a problem and b) that you would prefer to change behavior. Thus,
> it seems that wait(WNOHANG) hanging when a traced leader of an empty
> thread-group has exited is a problem in your eyes.
Again, I mostly tried to argue with do_notify_pidfd() called by realese_task().
I think that with PIDFD_THREAD set pidfd_poll() should succeed right
after the exiting thread becomes a zombie (passes exit_notify), whether
it is a leader or not.
Let me quote part of my reply to Tycho's patch
> + /*
> + * If we're not the leader, notify any waiters on our pidfds. Note that
> + * we don't want to notify the leader until /everyone/ in the thread
> + * group is dead, viz. the condition below.
> + *
> + * We have to do this here, since __exit_signal() will
> + * __unhash_processes(), and break do_notify_pidfd()'s lookup.
> + */
> + if (!thread_group_leader(p))
> + do_notify_pidfd(p);
This doesn't look consistent.
If the task is a group leader do_notify_pidfd() is called by exit_notify()
when it becomes a zombie (if no other threads), before it is reaped by its
parent (unless autoreap).
If it is a sub-thread, it is called by release_task() above. Note that a
sub-thread can become a zombie too if it is traced.
Not to mention that this is racy.
I would not mind if we simply move do_notify_pidfd() from exit_notify() to
release_task() and do it regardless of thread_group_leader(). And in some
sense this looks more logical to me.
But as I said:
- I do not know how/why people actually use poll(pidfd)
- it is too late to change the current behaviour
Sorry for confusion.
> I'm not sure whether you remember that but when we originally did the
> pidfd work you and I discussed thread support and already decided back
> then that having a flag like PIDFD_THREAD would likely be the way to go.
All I can recall is that, yes, we had some discussions about pidfd in
the past ;)
> The PIDFD_THREAD flag would be would be interesting because we could
> make pidfd_send_signal() support this flag
Agreed,
Oleg.
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Oleg Nesterov @ 2024-01-26 14:33 UTC (permalink / raw)
To: Christian Brauner
Cc: Tycho Andersen, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240126-kulinarisch-ausziehen-d2af51882d2f@brauner>
On 01/26, Christian Brauner wrote:
>
> > --- a/include/uapi/linux/pidfd.h
> > +++ b/include/uapi/linux/pidfd.h
> > @@ -7,6 +7,7 @@
> > #include <linux/fcntl.h>
> >
> > /* Flags for pidfd_open(). */
> > -#define PIDFD_NONBLOCK O_NONBLOCK
> > +#define PIDFD_NONBLOCK O_NONBLOCK
> > +#define PIDFD_THREAD O_EXCL // or anything else not used by anon_inode's
>
> I like it!
>
> The only request I would have is to not alias O_EXCL and PIDFD_THREAD.
> Because it doesn't map as clearly as NONBLOCK did.
But it would be nice to have PIDFD_THREAD in file->f_flags. Where else
can we keep it?
I chose O_EXCL because it can only be used at open time, it can never
be used or changed after anon_inode_getfile(), so we can safely do
pidfd_file->f_flags |= PIDFD_THREAD;
in __pidfd_prepare() and then check in pidfd_poll/pidfd_send_signal.
What do you suggest instead?
Oleg.
^ permalink raw reply
* Re: [PATCH v3 3/4] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Gregory Price @ 2024-01-26 15:57 UTC (permalink / raw)
To: Huang, Ying
Cc: Gregory Price, linux-mm, linux-kernel, linux-doc, linux-fsdevel,
linux-api, corbet, akpm, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Srinivasulu Thanneeru
In-Reply-To: <87y1cclgcm.fsf@yhuang6-desk2.ccr.corp.intel.com>
On Fri, Jan 26, 2024 at 03:10:49PM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@gmail.com> writes:
>
> > + } else if (pol == current->mempolicy &&
> > + (pol->mode == MPOL_WEIGHTED_INTERLEAVE)) {
> > + if (pol->cur_il_weight)
> > + *policy = current->il_prev;
> > + else
> > + *policy = next_node_in(current->il_prev,
> > + pol->nodes);
>
> It appears that my previous comments about this is ignored.
>
> https://lore.kernel.org/linux-mm/875xzkv3x2.fsf@yhuang6-desk2.ccr.corp.intel.com/
>
> Please correct me if I am wrong.
>
The fix is in the following patch. I'd originally planned to squash the
atomic patch into this one, but decided against it because it probably
warranted isolated scrutiny.
@@ -973,8 +974,10 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
*policy = next_node_in(current->il_prev, pol->nodes);
} else if (pol == current->mempolicy &&
(pol->mode == MPOL_WEIGHTED_INTERLEAVE)) {
- if (pol->cur_il_weight)
- *policy = current->il_prev;
+ int cweight = atomic_read(&pol->cur_il_weight);
+
+ if (cweight & 0xFF)
+ *policy = cweight >> 8;
in this we return the node the weight applies to, otherwise we return
whatever is after il_prev.
I can pull this fix ahead.
> > + /* if now at 0, move to next node and set up that node's weight */
> > + if (unlikely(!policy->cur_il_weight)) {
> > + me->il_prev = node;
> > + next = next_node_in(node, policy->nodes);
> > + rcu_read_lock();
> > + table = rcu_dereference(iw_table);
> > + /* detect system-default values */
> > + weight = table ? table[next] : 1;
> > + policy->cur_il_weight = weight ? weight : 1;
> > + rcu_read_unlock();
> > + }
>
> It appears that the code could be more concise if we allow
> policy->cur_il_weight == 0. Duplicated code are in
> alloc_pages_bulk_array_weighted_interleave() too. Anyway, can we define
> some function to reduce duplicated code.
>
This is kind of complicated by the next patch, which places the node and
the weight into the same field to resolve the stale weight issue.
In that patch (cur_il_weight = 0) means "cur_il_weight invalid",
because the weight part can only be 0 when:
a) an error occuring during bulk allocation
b) a rebind event
I'll take some time to think about whether we can do away with
task->il_prev (as your next patch notes mentioned).
> > + /* Otherwise we adjust nr_pages down, and continue from there */
> > + rem_pages -= pol->cur_il_weight;
> > + pol->cur_il_weight = 0;
>
> This break the rule to keep pol->cur_il_weight != 0 except after initial
> setup. Is it OK?
>
The only way cur_il_weight can leave this function 0 at this point is if
an error occurs (specifically the failure to kmalloc immediately next).
If we don't clear cur_il_weight here, then we have a stale weight, and
the next allocation pass will over-allocate on the current node.
This semantic also changes a bit in the next patch, but is basically the
same. If il_weight is 0, then either an error occurred or a rebind
event occured.
> > + /* resume from this node w/ remaining weight */
> > + resume_node = prev_node;
> > + resume_weight = weight - (node_pages % weight);
>
> resume_weight = weight - delta; ?
>
ack
~Gregory
^ permalink raw reply
* Re: [PATCH v3 4/4] mm/mempolicy: change cur_il_weight to atomic and carry the node with it
From: Gregory Price @ 2024-01-26 16:38 UTC (permalink / raw)
To: Huang, Ying
Cc: Gregory Price, linux-mm, linux-kernel, linux-doc, linux-fsdevel,
linux-api, corbet, akpm, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams
In-Reply-To: <87sf2klez8.fsf@yhuang6-desk2.ccr.corp.intel.com>
On Fri, Jan 26, 2024 at 03:40:27PM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@gmail.com> writes:
>
> > Two special observations:
> > - if the weight is non-zero, cur_il_weight must *always* have a
> > valid node number, e.g. it cannot be NUMA_NO_NODE (-1).
>
> IIUC, we don't need that, "MAX_NUMNODES-1" is used instead.
>
Correct, I just thought it pertinent to call this out explicitly since
I'm stealing the top byte, but the node value has traditionally been a
full integer.
This may be relevant should anyone try to carry, a random node value
into this field. For example, if someone tried to copy policy->home_node
into cur_il_weight for whatever reason.
It's worth breaking out a function to defend against this - plus to hide
the bit operations directly as you recommend below.
> > /* Weighted interleave settings */
> > - u8 cur_il_weight;
> > + atomic_t cur_il_weight;
>
> If we use this field for node and weight, why not change the field name?
> For example, cur_wil_node_weight.
>
ack.
> > + if (cweight & 0xFF)
> > + *policy = cweight >> 8;
>
> Please define some helper functions or macros instead of operate on bits
> directly.
>
ack.
> > else
> > *policy = next_node_in(current->il_prev,
> > pol->nodes);
>
> If we record current node in pol->cur_il_weight, why do we still need
> curren->il_prev. Can we only use pol->cur_il_weight? And if so, we can
> even make current->il_prev a union.
>
I just realized that there's a problem here for shared memory policies.
from weighted_interleave_nodes, I do this:
cur_weight = atomic_read(&policy->cur_il_weight);
...
weight--;
...
atomic_set(&policy->cur_il_weight, cur_weight);
On a shared memory policy, this is a race condition.
I don't think we can combine il_prev and cur_wil_node_weight because
the task policy may be different than the current policy.
i.e. it's totally valid to do the following:
1) set_mempolicy(MPOL_INTERLEAVE)
2) mbind(..., MPOL_WEIGHTED_INTERLEAVE)
Using current->il_prev between these two policies, is just plain incorrect,
so I will need to rethink this, and the existing code will need to be
updated such that weighted_interleave does not use current->il_prev.
~Gregory
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-01-26 16:52 UTC (permalink / raw)
To: Christian Brauner
Cc: Greg Kroah-Hartman, linux-kernel, netdev, chuck.lever, jlayton,
linux-api, edumazet, davem, alexander.duyck, sridhar.samudrala,
kuba, willemdebruijn.kernel, weiwan, Jonathan Corbet,
Alexander Viro, Jan Kara, Michael Ellerman, Nathan Lynch,
Steve French, Thomas Zimmermann, Jiri Slaby, Julien Panis,
Arnd Bergmann, Andrew Waterman, Thomas Huth, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240126-kribbeln-sonnabend-35dcb3d1fc48@brauner>
On Fri, Jan 26, 2024 at 11:07:36AM +0100, Christian Brauner wrote:
> On Thu, Jan 25, 2024 at 06:36:30PM -0800, Joe Damato wrote:
> > On Thu, Jan 25, 2024 at 04:23:58PM -0800, Greg Kroah-Hartman wrote:
> > > On Thu, Jan 25, 2024 at 04:11:28PM -0800, Joe Damato wrote:
> > > > On Thu, Jan 25, 2024 at 03:21:46PM -0800, Greg Kroah-Hartman wrote:
> > > > > On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:
> > > > > > +struct epoll_params {
> > > > > > + u64 busy_poll_usecs;
> > > > > > + u16 busy_poll_budget;
> > > > > > +
> > > > > > + /* for future fields */
> > > > > > + u8 data[118];
> > > > > > +} EPOLL_PACKED;
> > > > >
> > > > > variables that cross the user/kernel boundry need to be __u64, __u16,
> > > > > and __u8 here.
> > > >
> > > > I'll make that change for the next version, thank you.
> > > >
> > > > > And why 118?
> > > >
> > > > I chose this arbitrarily. I figured that a 128 byte struct would support 16
> > > > u64s in the event that other fields needed to be added in the future. 118
> > > > is what was left after the existing fields. There's almost certainly a
> > > > better way to do this - or perhaps it is unnecessary as per your other
> > > > message.
> > > >
> > > > I am not sure if leaving extra space in the struct is a recommended
> > > > practice for ioctls or not - I thought I noticed some code that did and
> > > > some that didn't in the kernel so I err'd on the side of leaving the space
> > > > and probably did it in the worst way possible.
> > >
> > > It's not really a good idea unless you know exactly what you are going
> > > to do with it. Why not just have a new ioctl if you need new
> > > information in the future? That's simpler, right?
> >
> > Sure, that makes sense to me. I'll remove it in the v4 alongside the other
> > changes you've requested.
>
> Fwiw, we do support extensible ioctls since they encode the size. Take a
> look at kernel/seccomp.c. It's a clean extensible interface built on top
> of the copy_struct_from_user() pattern we added for system calls
> (openat(), clone3() etc.):
>
> static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> struct seccomp_filter *filter = file->private_data;
> void __user *buf = (void __user *)arg;
>
> /* Fixed-size ioctls */
> switch (cmd) {
> case SECCOMP_IOCTL_NOTIF_RECV:
> return seccomp_notify_recv(filter, buf);
> case SECCOMP_IOCTL_NOTIF_SEND:
> return seccomp_notify_send(filter, buf);
> case SECCOMP_IOCTL_NOTIF_ID_VALID_WRONG_DIR:
> case SECCOMP_IOCTL_NOTIF_ID_VALID:
> return seccomp_notify_id_valid(filter, buf);
> case SECCOMP_IOCTL_NOTIF_SET_FLAGS:
> return seccomp_notify_set_flags(filter, arg);
> }
>
> /* Extensible Argument ioctls */
> #define EA_IOCTL(cmd) ((cmd) & ~(IOC_INOUT | IOCSIZE_MASK))
> switch (EA_IOCTL(cmd)) {
> case EA_IOCTL(SECCOMP_IOCTL_NOTIF_ADDFD):
> return seccomp_notify_addfd(filter, buf, _IOC_SIZE(cmd));
> default:
> return -EINVAL;
> }
> }
>
> static long seccomp_notify_addfd(struct seccomp_filter *filter,
> struct seccomp_notif_addfd __user *uaddfd,
> unsigned int size)
> {
> struct seccomp_notif_addfd addfd;
> struct seccomp_knotif *knotif;
> struct seccomp_kaddfd kaddfd;
> int ret;
>
> BUILD_BUG_ON(sizeof(addfd) < SECCOMP_NOTIFY_ADDFD_SIZE_VER0);
> BUILD_BUG_ON(sizeof(addfd) != SECCOMP_NOTIFY_ADDFD_SIZE_LATEST);
>
> if (size < SECCOMP_NOTIFY_ADDFD_SIZE_VER0 || size >= PAGE_SIZE)
> return -EINVAL;
>
> ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size);
> if (ret)
> return ret;
Thanks; that's a really helpful note and example.
I'm inclined to believe that new fields probably won't be needed for a
while, but if they are: an extensible ioctl could be added in the future
to deal with that problem at that point.
Thanks,
Joe
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Tycho Andersen @ 2024-01-26 21:50 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240125140830.GA5513@redhat.com>
Hi Oleg,
On Thu, Jan 25, 2024 at 03:08:31PM +0100, Oleg Nesterov wrote:
> What do you think?
Thank you, it passes all my tests.
> + /* unnecessary if do_notify_parent() was already called,
> + we can do better */
> + do_notify_pidfd(tsk);
"do better" here could be something like,
diff --git a/kernel/exit.c b/kernel/exit.c
index efe8f1d3a6af..7e545393f2f5 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -742,6 +742,7 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
bool autoreap;
struct task_struct *p, *n;
LIST_HEAD(dead);
+ bool needs_notify = true;
write_lock_irq(&tasklist_lock);
forget_original_parent(tsk, &dead);
@@ -756,16 +757,21 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
!ptrace_reparented(tsk) ?
tsk->exit_signal : SIGCHLD;
autoreap = do_notify_parent(tsk, sig);
+ needs_notify = false;
} else if (thread_group_leader(tsk)) {
- autoreap = thread_group_empty(tsk) &&
- do_notify_parent(tsk, tsk->exit_signal);
+ autoreap = false;
+ if (thread_group_empty(tsk)) {
+ autoreap = do_notify_parent(tsk, tsk->exit_signal);
+ needs_notify = false;
+ }
} else {
autoreap = true;
}
/* unnecessary if do_notify_parent() was already called,
we can do better */
- do_notify_pidfd(tsk);
+ if (needs_notify)
+ do_notify_pidfd(tsk);
if (autoreap) {
tsk->exit_state = EXIT_DEAD;
but even with that, there's other calls in the tree to
do_notify_parent() that might double notify.
This brings up another interesting behavior that I noticed while
testing this, if you do a poll() on pidfd, followed quickly by a
pidfd_getfd() on the same thread you just got an event on, you can
sometimes get an EBADF from __pidfd_fget() instead of the more
expected ESRCH higher up the stack.
I wonder if it makes sense to abuse ->f_flags to add a PIDFD_NOTIFIED?
Then we can refuse further pidfd syscall operations in a sane way, and
also "do better" above by checking this flag from do_pidfd_notify()
before doing it again?
Tycho
^ permalink raw reply related
* Re: [PATCH v2 1/1] mm/madvise: add MADV_F_COLLAPSE_LIGHT to process_madvise()
From: Zach O'Keefe @ 2024-01-26 23:26 UTC (permalink / raw)
To: Lance Yang
Cc: Michal Hocko, akpm, david, songmuchun, shy828301, peterx,
mknyszek, minchan, linux-mm, linux-kernel, linux-api
In-Reply-To: <CAK1f24=7fKU-+xRSOhrJrZ56=YoVfnkONZwNOJRS4yY4SzhANA@mail.gmail.com>
On Mon, Jan 22, 2024 at 6:35 AM Lance Yang <ioworker0@gmail.com> wrote:
>
> Hey Zach,
>
> What do you think about the semantic?
Hey Lance,
Sorry for the late reply.
I can see both sides of the argument; though I would argue that
"non-blocking" is equally as vague in this context. E.g. we'll "block" on
acquiring a number of different locks along the collapse path.
If you really want to talk about not entering direct reclaim /
compaction, then keeping with the sys/kernel/vm/thp notion of "defrag"
would be better, IMO. I don't feel that strongly about it though.
But I see you've provided some more use cases in another mail, so let
me pick up my thoughts over there.
Best,
Zach
> Thanks,
> Lance
>
> On Mon, Jan 22, 2024 at 10:14 PM Lance Yang <ioworker0@gmail.com> wrote:
> >
> > On Mon, Jan 22, 2024 at 9:50 PM Michal Hocko <mhocko@suse.com> wrote:
> > >
> > > On Sat 20-01-24 10:09:32, Lance Yang wrote:
> > > [...]
> > > > Hey Michal,
> > > >
> > > > Thanks for your suggestion!
> > > >
> > > > It seems that the implementation should try but not too hard aligns well
> > > > with my desired behavior.
> > >
> > > The problem I have with this semantic is that it is really hard to
> > > define and then stick with. Our implementation might change over time
> > > and what somebody considers good ATM might turn int "trying harder than
> > > I wanted" later on.
> > >
> > > > Non-blocking in general is also a great idea.
> > > > Perhaps in the future, we can add a MADV_F_COLLAPSE_NOBLOCK
> > > > flag for scenarios where latency is extremely critical.
> > >
> > > Non blocking semantic is much easier to define and maintain. The actual
> > > allocation/compaction implementation might change as well over time but
> > > the userspace at least knows that the request will not block waiting for
> > > any required resources.
> >
> > I appreciate your insights!
> >
> > It makes sense that a non-blocking semantic is easier to define and maintain,
> > providing userspace with the certainty that requests won’t be blocked.
> >
> > Thanks,
> > Lance
> >
> > >
> > > --
> > > Michal Hocko
> > > SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 1/1] mm/madvise: add MADV_F_COLLAPSE_LIGHT to process_madvise()
From: Zach O'Keefe @ 2024-01-26 23:46 UTC (permalink / raw)
To: Lance Yang
Cc: akpm, Michal Hocko, Yang Shi, David Hildenbrand, songmuchun,
peterx, mknyszek, minchan, linux-mm, linux-kernel, linux-api
In-Reply-To: <CAK1f24n0152CmPghBLQ7225=rwEuP6mMdBybWZz7heTLzemBqw@mail.gmail.com>
> I’d like to add another real use case.
>
> In our company, we deploy applications using offline-online
> hybrid deployment. This approach leverages the distinctive
> resource utilization patterns of online services, utilizing idle
> resources during various time periods by filling them with
> offline jobs. This helps reduce the growing cost expenditures
> for the enterprise.
>
> Whether for online services or offline jobs, their requirements
> for THP can be roughly categorized into three types:
>
> * The first type aims to use huge pages as much as possible
> and tolerates unpredictable stalls caused by direct reclaim
> and/or compaction.
> * The second type attempts to use huge pages but is relatively
> latency-sensitive and cannot tolerate unpredictable stalls.
> * The third type prefers not to use huge pages at all and is
> extremely latency-sensitive.
>
> After careful consideration, we decided to prioritize the
> requirements of the first type and modify the THP settings
> as follows:
>
> echo madvise >/sys/kernel/mm/transparent_hugepage/enabled
> echo defer >/sys/kernel/mm/transparent_hugepage/defrag
>
> With the introduction of MADV_COLLAPSE into the kernel,
> it is no longer dependent on any sysfs setting under
> /sys/kernel/mm/transparent_hugepage. MADV_COLLAPSE
> offers the potential for fine-grained synchronous control over
> the huge page allocation mechanism, marking a significant
> enhancement for THP.
>
> If the kernel supports a more relaxed (opportunistic)
> MADV_COLLAPSE, we will modify the THP settings as follows:
>
> echo madvise >/sys/kernel/mm/transparent_hugepage/enabled
> echo madvise >/sys/kernel/mm/transparent_hugepage/defrag
[corrected, via 2 previous mails, to: echo madvise
>/sys/kernel/mm/transparent_hugepage/enabled
echo defer+madvise >/sys/kernel/mm/transparent_hugepage/defrag]
> Then, we will use process_madvise(MADV_COLLAPSE, xx_relaxed_flag)
> to address the requirements of the second type.
>
> Why don't we favor madvise(MADV_COLLAPSE) for the first type
> of requirements?
> The main reason is that these requirements are typically for offline
> jobs in the Hadoop ecosystem, such as MapReduce and Spark,
> which run primarily on the JVM. [..]
Hey Lance,
Thanks for proving this context, it's very helpful.
Though, couldn't you use enabled=always, defrag=defer+madvise, then
just use prctl(PR_SET_THP_DISABLE) on type-3 workloads to get the
behaviour you want? i.e.
type 1: apply MADV_HUGEPAGE -> sync defrag to get THP
type 2: don't apply MADV_HUGEPAGE -> use THP if available, kick
kswapd+kcompactd otherwise
type 3: use prctl(PR_SET_THP_DISABLE) (or MADV_NOHUGEPAGE) -> no THPs
Or am I missing something? It sounds like a confounding issue is that
these are external workloads, or you don't have ability to modify? But
that would preclude MADV_COLLAPSE (unless you're using
process_madvise()).
Appreciate the help understanding the use case. I'm not opposed to the
idea in general, but IMO would be great to have a clear need for it
(and right now, we don't currently have alignment with the original
motivating usecase (Go) in that regard w.r.t their plans).
Thanks,
Zach
^ permalink raw reply
* Re: [PATCH v2 1/1] mm/madvise: add MADV_F_COLLAPSE_LIGHT to process_madvise()
From: Lance Yang @ 2024-01-27 8:03 UTC (permalink / raw)
To: Zach O'Keefe
Cc: akpm, Michal Hocko, Yang Shi, David Hildenbrand, songmuchun,
peterx, mknyszek, minchan, linux-mm, linux-kernel, linux-api
In-Reply-To: <CAAa6QmRZP5pL_5O7BpfjQf5LZ_ADGqYF_xdAYEbKXkqMViAwLw@mail.gmail.com>
Hey Zach,
Thanks for taking time to look into this!
On Sat, Jan 27, 2024 at 7:47 AM Zach O'Keefe <zokeefe@google.com> wrote:
>
> > I’d like to add another real use case.
> >
> > In our company, we deploy applications using offline-online
> > hybrid deployment. This approach leverages the distinctive
> > resource utilization patterns of online services, utilizing idle
> > resources during various time periods by filling them with
> > offline jobs. This helps reduce the growing cost expenditures
> > for the enterprise.
> >
> > Whether for online services or offline jobs, their requirements
> > for THP can be roughly categorized into three types:
> >
> > * The first type aims to use huge pages as much as possible
> > and tolerates unpredictable stalls caused by direct reclaim
> > and/or compaction.
> > * The second type attempts to use huge pages but is relatively
> > latency-sensitive and cannot tolerate unpredictable stalls.
> > * The third type prefers not to use huge pages at all and is
> > extremely latency-sensitive.
> >
> > After careful consideration, we decided to prioritize the
> > requirements of the first type and modify the THP settings
> > as follows:
> >
> > echo madvise >/sys/kernel/mm/transparent_hugepage/enabled
> > echo defer >/sys/kernel/mm/transparent_hugepage/defrag
> >
> > With the introduction of MADV_COLLAPSE into the kernel,
> > it is no longer dependent on any sysfs setting under
> > /sys/kernel/mm/transparent_hugepage. MADV_COLLAPSE
> > offers the potential for fine-grained synchronous control over
> > the huge page allocation mechanism, marking a significant
> > enhancement for THP.
> >
> > If the kernel supports a more relaxed (opportunistic)
> > MADV_COLLAPSE, we will modify the THP settings as follows:
> >
> > echo madvise >/sys/kernel/mm/transparent_hugepage/enabled
> > echo madvise >/sys/kernel/mm/transparent_hugepage/defrag
>
> [corrected, via 2 previous mails, to: echo madvise
> >/sys/kernel/mm/transparent_hugepage/enabled
> echo defer+madvise >/sys/kernel/mm/transparent_hugepage/defrag]
>
>
> > Then, we will use process_madvise(MADV_COLLAPSE, xx_relaxed_flag)
> > to address the requirements of the second type.
> >
> > Why don't we favor madvise(MADV_COLLAPSE) for the first type
> > of requirements?
> > The main reason is that these requirements are typically for offline
> > jobs in the Hadoop ecosystem, such as MapReduce and Spark,
> > which run primarily on the JVM. [..]
>
> Hey Lance,
>
> Thanks for proving this context, it's very helpful.
>
> Though, couldn't you use enabled=always, defrag=defer+madvise, then
> just use prctl(PR_SET_THP_DISABLE) on type-3 workloads to get the
> behaviour you want? i.e.
prctl(PR_SET_THP_DISABLE) is a good choice that can fully meet
the needs of type-3 workloads.
I might prefer using enabled=madvise, as this would allow
applications to implement specific calls to madvise to request huge
pages selectively. If we set enabled=always, some applications
may not be optimized for or may not benefit from huge pages.
In such cases, using huge pages for all allocations could lead
to suboptimal performance.
>
> type 1: apply MADV_HUGEPAGE -> sync defrag to get THP
> type 2: don't apply MADV_HUGEPAGE -> use THP if available, kick
> kswapd+kcompactd otherwise
Sorry, I did not express myself clearly. The type 2 of requirements
should be:
type 2: apply MADV_HUGEPAGE with defrag=defer, or use a more
relaxed (opportunistic) MADV_COLLAPSE.
> type 3: use prctl(PR_SET_THP_DISABLE) (or MADV_NOHUGEPAGE) -> no THPs
>
> Or am I missing something? It sounds like a confounding issue is that
> these are external workloads, or you don't have ability to modify? But
> that would preclude MADV_COLLAPSE (unless you're using
> process_madvise()).
Sorry, my previous explanation has been unclear. What I meant is
that the requirements of type-1 workloads can be independent of
any sysfs setting and can be addressed using madvise(MADV_COLLAPSE).
In this scenario, why haven't I utilized it? The reason is that I
currently lack the capability to modify the JVM or PyTorch to
make them compatible with madvise(MADV_COLLAPSE).
Therefore, the needs of type-1 workloads still rely on sysfs settings.
>
> Appreciate the help understanding the use case. I'm not opposed to the
> idea in general, but IMO would be great to have a clear need for it
I appreciate your perspective!
Thanks again for your valuable insights and your suggestions!
Lance
> (and right now, we don't currently have alignment with the original
> motivating usecase (Go) in that regard w.r.t their plans).
>
> Thanks,
> Zach
^ permalink raw reply
* Re: [PATCH v2 1/1] mm/madvise: add MADV_F_COLLAPSE_LIGHT to process_madvise()
From: Lance Yang @ 2024-01-27 8:06 UTC (permalink / raw)
To: Zach O'Keefe
Cc: Michal Hocko, akpm, david, songmuchun, shy828301, peterx,
mknyszek, minchan, linux-mm, linux-kernel, linux-api
In-Reply-To: <CAAa6QmRLfwhyh66bVVakp__Rkzpkt3zq2kAe9F-KN5sdFdqLAg@mail.gmail.com>
How about MADV_F_COLLAPSE_NODEFRAG?
On Sat, Jan 27, 2024 at 7:27 AM Zach O'Keefe <zokeefe@google.com> wrote:
>
> On Mon, Jan 22, 2024 at 6:35 AM Lance Yang <ioworker0@gmail.com> wrote:
> >
> > Hey Zach,
> >
> > What do you think about the semantic?
>
> Hey Lance,
>
> Sorry for the late reply.
>
> I can see both sides of the argument; though I would argue that
> "non-blocking" is equally as vague in this context. E.g. we'll "block" on
> acquiring a number of different locks along the collapse path.
>
> If you really want to talk about not entering direct reclaim /
> compaction, then keeping with the sys/kernel/vm/thp notion of "defrag"
> would be better, IMO. I don't feel that strongly about it though.
>
> But I see you've provided some more use cases in another mail, so let
> me pick up my thoughts over there.
>
> Best,
> Zach
>
>
>
> > Thanks,
> > Lance
> >
> > On Mon, Jan 22, 2024 at 10:14 PM Lance Yang <ioworker0@gmail.com> wrote:
> > >
> > > On Mon, Jan 22, 2024 at 9:50 PM Michal Hocko <mhocko@suse.com> wrote:
> > > >
> > > > On Sat 20-01-24 10:09:32, Lance Yang wrote:
> > > > [...]
> > > > > Hey Michal,
> > > > >
> > > > > Thanks for your suggestion!
> > > > >
> > > > > It seems that the implementation should try but not too hard aligns well
> > > > > with my desired behavior.
> > > >
> > > > The problem I have with this semantic is that it is really hard to
> > > > define and then stick with. Our implementation might change over time
> > > > and what somebody considers good ATM might turn int "trying harder than
> > > > I wanted" later on.
> > > >
> > > > > Non-blocking in general is also a great idea.
> > > > > Perhaps in the future, we can add a MADV_F_COLLAPSE_NOBLOCK
> > > > > flag for scenarios where latency is extremely critical.
> > > >
> > > > Non blocking semantic is much easier to define and maintain. The actual
> > > > allocation/compaction implementation might change as well over time but
> > > > the userspace at least knows that the request will not block waiting for
> > > > any required resources.
> > >
> > > I appreciate your insights!
> > >
> > > It makes sense that a non-blocking semantic is easier to define and maintain,
> > > providing userspace with the certainty that requests won’t be blocked.
> > >
> > > Thanks,
> > > Lance
> > >
> > > >
> > > > --
> > > > Michal Hocko
> > > > SUSE Labs
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Oleg Nesterov @ 2024-01-27 10:54 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <ZbQpPknTTCyiyxrP@tycho.pizza>
Hi Tycho,
On 01/26, Tycho Andersen wrote:
>
> On Thu, Jan 25, 2024 at 03:08:31PM +0100, Oleg Nesterov wrote:
> > What do you think?
>
> Thank you, it passes all my tests.
Great, thanks!
OK, I'll make v2 on top of the recent
"pidfd: cleanup the usage of __pidfd_prepare's flags"
but we need to finish our discussion with Christian about the
usage of O_EXCL.
As for clone(CLONE_PIDFD | CLONE_THREAD), this is trivial but
I think this needs another discussion too, lets do this later.
> > + /* unnecessary if do_notify_parent() was already called,
> > + we can do better */
> > + do_notify_pidfd(tsk);
>
> "do better" here could be something like,
>
> [...snip...]
No, no, please see below.
For the moment, please forget about PIDFD_THREAD, lets discuss
the current behaviour.
> but even with that, there's other calls in the tree to
> do_notify_parent() that might double notify.
Yes, and we can't avoid this. Well, perhaps do_notify_parent()
can do something like
if (ptrace_reparented())
do_notify_pidfd();
so that only the "final" do_notify_parent() does do_notify_pidfd()
but this needs another discussion and in fact I don't think this
would be right or make much sense. Lets forget this for now.
Now. Even without PIDFD_THREAD, I think it makes sense to change
do_notify_parent() to do
if (thread_group_empty(tsk))
do_notify_pidfd(tsk);
thread_group_empty(tsk) can only be true if tsk is a group leader
and it is the last thread. And this is exactly what pidfd_poll()
currently needs.
In fact I'd even prefer to do this in a separate patch for the
documentation purposes.
Now, PIDFD_THREAD can just add
if (!thread_group_empty(tsk))
do_notify_pidfd(tsk);
right after "tsk->exit_state = EXIT_ZOMBIE", that is all.
This also preserves the do_notify_pidfd/__wake_up_parent ordering.
Not that I think this is important, just for consistency.
> This brings up another interesting behavior that I noticed while
> testing this, if you do a poll() on pidfd, followed quickly by a
> pidfd_getfd() on the same thread you just got an event on, you can
> sometimes get an EBADF from __pidfd_fget() instead of the more
> expected ESRCH higher up the stack.
exit_notify() is called after exit_files(). pidfd_getfd() returns
ESRCH if the exiting thread completes release_task(), otherwise it
returns EBADF because ->files == NULL. This too doesn't really
depend on PIDFD_THREAD.
> I wonder if it makes sense to abuse ->f_flags to add a PIDFD_NOTIFIED?
> Then we can refuse further pidfd syscall operations in a sane way, and
But how? We only have "struct pid *", how can we find all files
"attached" to this pid?
> also "do better" above by checking this flag from do_pidfd_notify()
> before doing it again?
and even it was possible, I don't think it makes a lot of sense, see
also above.
but perhaps I understood you...
Oleg.
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Christian Brauner @ 2024-01-27 14:26 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Tycho Andersen, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240126143349.GD7386@redhat.com>
On Fri, Jan 26, 2024 at 03:33:50PM +0100, Oleg Nesterov wrote:
> On 01/26, Christian Brauner wrote:
> >
> > > --- a/include/uapi/linux/pidfd.h
> > > +++ b/include/uapi/linux/pidfd.h
> > > @@ -7,6 +7,7 @@
> > > #include <linux/fcntl.h>
> > >
> > > /* Flags for pidfd_open(). */
> > > -#define PIDFD_NONBLOCK O_NONBLOCK
> > > +#define PIDFD_NONBLOCK O_NONBLOCK
> > > +#define PIDFD_THREAD O_EXCL // or anything else not used by anon_inode's
> >
> > I like it!
> >
> > The only request I would have is to not alias O_EXCL and PIDFD_THREAD.
> > Because it doesn't map as clearly as NONBLOCK did.
>
> But it would be nice to have PIDFD_THREAD in file->f_flags. Where else
> can we keep it?
No, I did just mean that the uapi value doesn't necessarily have to
reflect what we do internally. IOW, we can still raise O_EXCL internally
in ->f_flags but there's no need to expose it as O_EXCL to userspace. We
often have internal and external flag spaces. If you prefer it your way
I'm not going argue.
> I chose O_EXCL because it can only be used at open time, it can never
> be used or changed after anon_inode_getfile(), so we can safely do
>
> pidfd_file->f_flags |= PIDFD_THREAD;
>
> in __pidfd_prepare() and then check in pidfd_poll/pidfd_send_signal.
>
> What do you suggest instead?
(Long-term and unrelated to this here, I think we will need to consider
not just stashing struct pid in pidfd_file->private_data but instead
struct pid with additional data because there's various functionality
that users would like that requires additional state to be stored and we
can't or don't want to do that in struct pid directly.)
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Christian Brauner @ 2024-01-27 14:33 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Tycho Andersen, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240127105410.GA13787@redhat.com>
On Sat, Jan 27, 2024 at 11:54:32AM +0100, Oleg Nesterov wrote:
> Hi Tycho,
>
> On 01/26, Tycho Andersen wrote:
> >
> > On Thu, Jan 25, 2024 at 03:08:31PM +0100, Oleg Nesterov wrote:
> > > What do you think?
> >
> > Thank you, it passes all my tests.
>
> Great, thanks!
>
> OK, I'll make v2 on top of the recent
> "pidfd: cleanup the usage of __pidfd_prepare's flags"
>
> but we need to finish our discussion with Christian about the
> usage of O_EXCL.
Just write the patch exactly like you want. If it's as a little a detail
as the uapi flag value we can just always change that when applying.
There's no reason to introduce artificial delays because of my
preference here..
>
> As for clone(CLONE_PIDFD | CLONE_THREAD), this is trivial but
> I think this needs another discussion too, lets do this later.
>
> > > + /* unnecessary if do_notify_parent() was already called,
> > > + we can do better */
> > > + do_notify_pidfd(tsk);
> >
> > "do better" here could be something like,
> >
> > [...snip...]
>
> No, no, please see below.
>
> For the moment, please forget about PIDFD_THREAD, lets discuss
> the current behaviour.
>
> > but even with that, there's other calls in the tree to
> > do_notify_parent() that might double notify.
>
> Yes, and we can't avoid this. Well, perhaps do_notify_parent()
> can do something like
>
> if (ptrace_reparented())
> do_notify_pidfd();
>
> so that only the "final" do_notify_parent() does do_notify_pidfd()
> but this needs another discussion and in fact I don't think this
> would be right or make much sense. Lets forget this for now.
>
> Now. Even without PIDFD_THREAD, I think it makes sense to change
> do_notify_parent() to do
>
> if (thread_group_empty(tsk))
> do_notify_pidfd(tsk);
>
> thread_group_empty(tsk) can only be true if tsk is a group leader
> and it is the last thread. And this is exactly what pidfd_poll()
> currently needs.
>
> In fact I'd even prefer to do this in a separate patch for the
> documentation purposes.
>
> Now, PIDFD_THREAD can just add
>
> if (!thread_group_empty(tsk))
> do_notify_pidfd(tsk);
>
> right after "tsk->exit_state = EXIT_ZOMBIE", that is all.
Sounds good.
>
> This also preserves the do_notify_pidfd/__wake_up_parent ordering.
> Not that I think this is important, just for consistency.
>
> > This brings up another interesting behavior that I noticed while
> > testing this, if you do a poll() on pidfd, followed quickly by a
> > pidfd_getfd() on the same thread you just got an event on, you can
> > sometimes get an EBADF from __pidfd_fget() instead of the more
> > expected ESRCH higher up the stack.
>
> exit_notify() is called after exit_files(). pidfd_getfd() returns
> ESRCH if the exiting thread completes release_task(), otherwise it
> returns EBADF because ->files == NULL. This too doesn't really
> depend on PIDFD_THREAD.
>
> > I wonder if it makes sense to abuse ->f_flags to add a PIDFD_NOTIFIED?
> > Then we can refuse further pidfd syscall operations in a sane way, and
>
> But how? We only have "struct pid *", how can we find all files
> "attached" to this pid?
We can't. There's some use-cases that would make this desirable but that
would mean we would need another data structure to track this. I once
toyed with a patch for this but never got so excited about it to
actually finish it.
^ permalink raw reply
* RE: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: David Laight @ 2024-01-27 14:51 UTC (permalink / raw)
To: 'Arnd Bergmann', Joe Damato, Greg Kroah-Hartman
Cc: linux-kernel@vger.kernel.org, Netdev, Chuck Lever, Jeff Layton,
linux-api@vger.kernel.org, Christian Brauner, Eric Dumazet,
David S . Miller, alexander.duyck@gmail.com, Sridhar Samudrala,
Jakub Kicinski, Willem de Bruijn, weiwan@google.com,
Jonathan Corbet, Alexander Viro, Jan Kara, Michael Ellerman,
Nathan Lynch, Steve French, Thomas Zimmermann, Jiri Slaby,
Julien Panis, Andrew Waterman, Thomas Huth, Palmer Dabbelt,
open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <57b62135-2159-493d-a6bb-47d5be55154a@app.fastmail.com>
From: Arnd Bergmann
> Sent: 26 January 2024 06:16
>
> On Fri, Jan 26, 2024, at 03:36, Joe Damato wrote:
> > On Thu, Jan 25, 2024 at 04:23:58PM -0800, Greg Kroah-Hartman wrote:
> >> On Thu, Jan 25, 2024 at 04:11:28PM -0800, Joe Damato wrote:
> >> > On Thu, Jan 25, 2024 at 03:21:46PM -0800, Greg Kroah-Hartman wrote:
> >> > > On Thu, Jan 25, 2024 at 10:56:59PM +0000, Joe Damato wrote:
> >> > > > +struct epoll_params {
> >> > > > + u64 busy_poll_usecs;
> >> > > > + u16 busy_poll_budget;
> >> > > > +
> >> > > > + /* for future fields */
> >> > > > + u8 data[118];
> >> > > > +} EPOLL_PACKED;
> >> > >
> >
> > Sure, that makes sense to me. I'll remove it in the v4 alongside the other
> > changes you've requested.
> >
> > Thanks for your time and patience reviewing my code. I greatly appreciate
> > your helpful comments and feedback.
>
> Note that you should still pad the structure to its normal
> alignment. On non-x86 targets this would currently mean a
> multiple of 64 bits.
>
> I would suggest dropping the EPOLL_PACKED here entirely and
> just using a fully aligned structure on all architectures, like
>
> struct epoll_params {
> __aligned_u64 busy_poll_usecs;
> __u16 busy_poll_budget;
> __u8 __pad[6];
> };
>
> The explicit padding can help avoid leaking stack data when
> a structure is copied back from kernel to userspace, so I would
> just always use it in ioctl data structures.
Or just use 32bit types for both fields.
Both values need erroring before they get that large.
32bit of usec is about 20 seconds.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Tycho Andersen @ 2024-01-27 15:55 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240127105410.GA13787@redhat.com>
On Sat, Jan 27, 2024 at 11:54:32AM +0100, Oleg Nesterov wrote:
> Hi Tycho,
>
> On 01/26, Tycho Andersen wrote:
> >
> > On Thu, Jan 25, 2024 at 03:08:31PM +0100, Oleg Nesterov wrote:
> > > What do you think?
> >
> > Thank you, it passes all my tests.
>
> Great, thanks!
>
> OK, I'll make v2 on top of the recent
> "pidfd: cleanup the usage of __pidfd_prepare's flags"
>
> but we need to finish our discussion with Christian about the
> usage of O_EXCL.
>
> As for clone(CLONE_PIDFD | CLONE_THREAD), this is trivial but
> I think this needs another discussion too, lets do this later.
>
> > > + /* unnecessary if do_notify_parent() was already called,
> > > + we can do better */
> > > + do_notify_pidfd(tsk);
> >
> > "do better" here could be something like,
> >
> > [...snip...]
>
> No, no, please see below.
>
> For the moment, please forget about PIDFD_THREAD, lets discuss
> the current behaviour.
>
> > but even with that, there's other calls in the tree to
> > do_notify_parent() that might double notify.
>
> Yes, and we can't avoid this. Well, perhaps do_notify_parent()
> can do something like
>
> if (ptrace_reparented())
> do_notify_pidfd();
>
> so that only the "final" do_notify_parent() does do_notify_pidfd()
> but this needs another discussion and in fact I don't think this
> would be right or make much sense. Lets forget this for now.
It seems like (and the current pidfd_test enforces for some cases) we
want exactly one notification for a task dying. I don't understand
how we guarantee this now, with all of these calls.
> > This brings up another interesting behavior that I noticed while
> > testing this, if you do a poll() on pidfd, followed quickly by a
> > pidfd_getfd() on the same thread you just got an event on, you can
> > sometimes get an EBADF from __pidfd_fget() instead of the more
> > expected ESRCH higher up the stack.
>
> exit_notify() is called after exit_files(). pidfd_getfd() returns
> ESRCH if the exiting thread completes release_task(), otherwise it
> returns EBADF because ->files == NULL. This too doesn't really
> depend on PIDFD_THREAD.
Yup, understood. It just seems like an inconsistency we might want to
fix.
> > I wonder if it makes sense to abuse ->f_flags to add a PIDFD_NOTIFIED?
> > Then we can refuse further pidfd syscall operations in a sane way, and
>
> But how? We only have "struct pid *", how can we find all files
> "attached" to this pid?
Yeah, we'd need some other linkage as Christian points out. But if
there is a predicate we can write that says whether this task has been
notified or not, it's not necessary. I just don't understand what that
is. But maybe your patch will make it clearer.
Tycho
^ permalink raw reply
* Re: [PATCH net-next v3 0/3] Per epoll context busy poll support
From: Willem de Bruijn @ 2024-01-27 16:20 UTC (permalink / raw)
To: Joe Damato, linux-kernel, netdev
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, willemdebruijn.kernel,
weiwan, Joe Damato, Alexander Viro, Andrew Waterman,
Arnd Bergmann, Dominik Brodowski, Greg Kroah-Hartman, Jan Kara,
Jiri Slaby, Jonathan Corbet, Julien Panis,
open list:DOCUMENTATION,
(open list:FILESYSTEMS \(VFS and infrastructure\)),
Michael Ellerman, Nathan Lynch, Palmer Dabbelt, Steve French,
Thomas Huth, Thomas Zimmermann
In-Reply-To: <20240125225704.12781-1-jdamato@fastly.com>
Joe Damato wrote:
> Greetings:
>
> Welcome to v3. Cover letter updated from v2 to explain why ioctl and
> adjusted my cc_cmd to try to get the correct people in addition to folks
> who were added in v1 & v2. Labeled as net-next because it seems networking
> related to me even though it is fs code.
>
> TL;DR This builds on commit bf3b9f6372c4 ("epoll: Add busy poll support to
> epoll with socket fds.") by allowing user applications to enable
> epoll-based busy polling and set a busy poll packet budget on a per epoll
> context basis.
>
> This makes epoll-based busy polling much more usable for user
> applications than the current system-wide sysctl and hardcoded budget.
>
> To allow for this, two ioctls have been added for epoll contexts for
> getting and setting a new struct, struct epoll_params.
>
> ioctl was chosen vs a new syscall after reviewing a suggestion by Willem
> de Bruijn [1]. I am open to using a new syscall instead of an ioctl, but it
> seemed that:
> - Busy poll affects all existing epoll_wait and epoll_pwait variants in
> the same way, so new verions of many syscalls might be needed. It
There is no need to support a new feature on legacy calls. Applications have
to be upgraded to the new ioctl, so they can also be upgraded to the latest
epoll_wait variant.
epoll_pwait extends epoll_wait with a sigmask.
epoll_pwait2 extends extends epoll_pwait with nsec resolution timespec.
Since they are supersets, nothing is lots by limiting to the most recent API.
In the discussion of epoll_pwait2 the addition of a forward looking flags
argument was discussed, but eventually dropped. Based on the argument that
adding a syscall is not a big task and does not warrant preemptive code.
This decision did receive a suitably snarky comment from Jonathan Corbet [1].
It is definitely more boilerplate, but essentially it is as feasible to add an
epoll_pwait3 that takes an optional busy poll argument. In which case, I also
believe that it makes more sense to configure the behavior of the syscall
directly, than through another syscall and state stored in the kernel.
I don't think that the usec fine grain busy poll argument is all that useful.
Documentation always suggests setting it to 50us or 100us, based on limited
data. Main point is to set it to exceed the round-trip delay of whatever the
process is trying to wait on. Overestimating is not costly, as the call
returns as soon as the condition is met. An epoll_pwait3 flag EPOLL_BUSY_POLL
with default 100us might be sufficient.
[1] https://lwn.net/Articles/837816/
> seems much simpler for users to use the correct
> epoll_wait/epoll_pwait for their app and add a call to ioctl to enable
> or disable busy poll as needed. This also probably means less work to
> get an existing epoll app using busy poll.
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Oleg Nesterov @ 2024-01-27 16:31 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <ZbUngjQMg+YUBAME@tycho.pizza>
On 01/27, Tycho Andersen wrote:
>
> It seems like (and the current pidfd_test enforces for some cases)
Which pidfd_test ?
> we
> want exactly one notification for a task dying.
This can't be right. EVERY user of poll_wait() or wait_event/etc
must handle/tolerate the false wakeups.
> I don't understand
> how we guarantee this now, with all of these calls.
I don't understand why do we need or even want to guarantee this.
The extra wakeup must be always fine correctness-wise. Sure, it
would be nice to avoid the unnecessary wakeups, and perhaps we
can change wake_up_all() to pass a key to, say, only wake_up the
PIDFD_THREAD waiters from exit_notify(). But certainly this is
outside the scope of PIDFD_THREAD change we discuss.
The changes in do_notify_parent() (I have already sent the patch) and
in exit_notify() (proposed in my previous email) just ensure that,
with the minimal changes, we avoid 2 do_notify_pidfd's from the same
exit_notify() path.
> > exit_notify() is called after exit_files(). pidfd_getfd() returns
> > ESRCH if the exiting thread completes release_task(), otherwise it
> > returns EBADF because ->files == NULL. This too doesn't really
> > depend on PIDFD_THREAD.
>
> Yup, understood. It just seems like an inconsistency we might want to
> fix.
Not sure this worth "fixing"...
Oleg.
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Tycho Andersen @ 2024-01-27 17:20 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240127163117.GB13787@redhat.com>
On Sat, Jan 27, 2024 at 05:31:39PM +0100, Oleg Nesterov wrote:
> On 01/27, Tycho Andersen wrote:
> >
> > It seems like (and the current pidfd_test enforces for some cases)
>
> Which pidfd_test ?
I was thinking of poll_pidfd() in pidfd_test.c, but,
> > we
> > want exactly one notification for a task dying.
>
> This can't be right. EVERY user of poll_wait() or wait_event/etc
> must handle/tolerate the false wakeups.
you're right, it doesn't enforce exactly once.
> > I don't understand
> > how we guarantee this now, with all of these calls.
>
> I don't understand why do we need or even want to guarantee this.
>
> The extra wakeup must be always fine correctness-wise. Sure, it
> would be nice to avoid the unnecessary wakeups, and perhaps we
> can change wake_up_all() to pass a key to, say, only wake_up the
> PIDFD_THREAD waiters from exit_notify(). But certainly this is
> outside the scope of PIDFD_THREAD change we discuss.
>
> The changes in do_notify_parent() (I have already sent the patch) and
> in exit_notify() (proposed in my previous email) just ensure that,
> with the minimal changes, we avoid 2 do_notify_pidfd's from the same
> exit_notify() path.
Sounds good.
> > > exit_notify() is called after exit_files(). pidfd_getfd() returns
> > > ESRCH if the exiting thread completes release_task(), otherwise it
> > > returns EBADF because ->files == NULL. This too doesn't really
> > > depend on PIDFD_THREAD.
> >
> > Yup, understood. It just seems like an inconsistency we might want to
> > fix.
>
> Not sure this worth "fixing"...
Yep, maybe not. Just wanted to point it out.
Tycho
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Oleg Nesterov @ 2024-01-27 19:31 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <ZbU7d0dpTY08JgIl@tycho.pizza>
On 01/27, Tycho Andersen wrote:
>
> > > > exit_notify() is called after exit_files(). pidfd_getfd() returns
> > > > ESRCH if the exiting thread completes release_task(), otherwise it
> > > > returns EBADF because ->files == NULL. This too doesn't really
> > > > depend on PIDFD_THREAD.
> > >
> > > Yup, understood. It just seems like an inconsistency we might want to
> > > fix.
> >
> > Not sure this worth "fixing"...
>
> Yep, maybe not. Just wanted to point it out.
On the second thought I am starting to understand your concern...
Indeed, in this case -EBADF is technically correct but it can confuse
the user which doesn't or can't know that this task/thread is exiting,
because EBADF looks as if the "int fd" argument was wrong.
Sorry I missed your point before.
Oleg.
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Tycho Andersen @ 2024-01-27 20:44 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240127193127.GC13787@redhat.com>
On Sat, Jan 27, 2024 at 08:31:27PM +0100, Oleg Nesterov wrote:
> On 01/27, Tycho Andersen wrote:
> >
> > > > > exit_notify() is called after exit_files(). pidfd_getfd() returns
> > > > > ESRCH if the exiting thread completes release_task(), otherwise it
> > > > > returns EBADF because ->files == NULL. This too doesn't really
> > > > > depend on PIDFD_THREAD.
> > > >
> > > > Yup, understood. It just seems like an inconsistency we might want to
> > > > fix.
> > >
> > > Not sure this worth "fixing"...
> >
> > Yep, maybe not. Just wanted to point it out.
>
> On the second thought I am starting to understand your concern...
>
> Indeed, in this case -EBADF is technically correct but it can confuse
> the user which doesn't or can't know that this task/thread is exiting,
> because EBADF looks as if the "int fd" argument was wrong.
>
> Sorry I missed your point before.
No worries. I realized it's not so hard to fix with your new
xxx_exited() helper from the PIDFD_THREAD patch, so maybe worth
cleaning up after all?
Tycho
^ permalink raw reply
* Re: [PATCH v3 1/3] pidfd: allow pidfd_open() on non-thread-group leaders
From: Oleg Nesterov @ 2024-01-27 21:10 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <ZbVrRgIvudX242ZU@tycho.pizza>
On 01/27, Tycho Andersen wrote:
>
> On Sat, Jan 27, 2024 at 08:31:27PM +0100, Oleg Nesterov wrote:
> >
> > On the second thought I am starting to understand your concern...
> >
> > Indeed, in this case -EBADF is technically correct but it can confuse
> > the user which doesn't or can't know that this task/thread is exiting,
> > because EBADF looks as if the "int fd" argument was wrong.
> >
> > Sorry I missed your point before.
>
> No worries. I realized it's not so hard to fix with your new
> xxx_exited() helper from the PIDFD_THREAD patch, so maybe worth
> cleaning up after all?
OK, lets discuss this later.
I'll (hopefully) send v2 on top of
pidfd: cleanup the usage of __pidfd_prepare's flags
pidfd: don't do_notify_pidfd() if !thread_group_empty()
on Monday, will be busy tomorrow (family duties ;)
Oleg.
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
From: kernel test robot @ 2024-01-28 11:24 UTC (permalink / raw)
To: Joe Damato, linux-kernel, netdev
Cc: oe-kbuild-all, chuck.lever, jlayton, linux-api, brauner, edumazet,
davem, alexander.duyck, sridhar.samudrala, kuba,
willemdebruijn.kernel, weiwan, Joe Damato, Jonathan Corbet,
Alexander Viro, Jan Kara, Michael Ellerman, Greg Kroah-Hartman,
Nathan Lynch, Steve French, Thomas Zimmermann, Jiri Slaby,
Julien Panis, Arnd Bergmann, Andrew Waterman, Thomas Huth,
Palmer Dabbelt, linux-doc,
(open list:FILESYSTEMS (VFS and infrastructure))
In-Reply-To: <20240125225704.12781-4-jdamato@fastly.com>
Hi Joe,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Joe-Damato/eventpoll-support-busy-poll-per-epoll-instance/20240126-070250
base: net-next/main
patch link: https://lore.kernel.org/r/20240125225704.12781-4-jdamato%40fastly.com
patch subject: [PATCH net-next v3 3/3] eventpoll: Add epoll ioctl for epoll_params
config: i386-buildonly-randconfig-002-20240127 (https://download.01.org/0day-ci/archive/20240128/202401281917.WeFbZE56-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240128/202401281917.WeFbZE56-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401281917.WeFbZE56-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from <command-line>:
>> ./usr/include/linux/eventpoll.h:89:9: error: unknown type name 'u64'
89 | u64 busy_poll_usecs;
| ^~~
>> ./usr/include/linux/eventpoll.h:90:9: error: unknown type name 'u16'
90 | u16 busy_poll_budget;
| ^~~
>> ./usr/include/linux/eventpoll.h:93:9: error: unknown type name 'u8'
93 | u8 data[118];
| ^~
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v3 4/4] mm/mempolicy: change cur_il_weight to atomic and carry the node with it
From: Huang, Ying @ 2024-01-29 8:17 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-kernel, linux-doc, linux-fsdevel,
linux-api, corbet, akpm, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams
In-Reply-To: <ZbPf6d2cQykdl3Eb@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Fri, Jan 26, 2024 at 03:40:27PM +0800, Huang, Ying wrote:
>> Gregory Price <gourry.memverge@gmail.com> writes:
>>
>> > Two special observations:
>> > - if the weight is non-zero, cur_il_weight must *always* have a
>> > valid node number, e.g. it cannot be NUMA_NO_NODE (-1).
>>
>> IIUC, we don't need that, "MAX_NUMNODES-1" is used instead.
>>
>
> Correct, I just thought it pertinent to call this out explicitly since
> I'm stealing the top byte, but the node value has traditionally been a
> full integer.
>
> This may be relevant should anyone try to carry, a random node value
> into this field. For example, if someone tried to copy policy->home_node
> into cur_il_weight for whatever reason.
>
> It's worth breaking out a function to defend against this - plus to hide
> the bit operations directly as you recommend below.
>
>> > /* Weighted interleave settings */
>> > - u8 cur_il_weight;
>> > + atomic_t cur_il_weight;
>>
>> If we use this field for node and weight, why not change the field name?
>> For example, cur_wil_node_weight.
>>
>
> ack.
>
>> > + if (cweight & 0xFF)
>> > + *policy = cweight >> 8;
>>
>> Please define some helper functions or macros instead of operate on bits
>> directly.
>>
>
> ack.
>
>> > else
>> > *policy = next_node_in(current->il_prev,
>> > pol->nodes);
>>
>> If we record current node in pol->cur_il_weight, why do we still need
>> curren->il_prev. Can we only use pol->cur_il_weight? And if so, we can
>> even make current->il_prev a union.
>>
>
> I just realized that there's a problem here for shared memory policies.
>
> from weighted_interleave_nodes, I do this:
>
> cur_weight = atomic_read(&policy->cur_il_weight);
> ...
> weight--;
> ...
> atomic_set(&policy->cur_il_weight, cur_weight);
>
> On a shared memory policy, this is a race condition.
>
>
> I don't think we can combine il_prev and cur_wil_node_weight because
> the task policy may be different than the current policy.
>
> i.e. it's totally valid to do the following:
>
> 1) set_mempolicy(MPOL_INTERLEAVE)
> 2) mbind(..., MPOL_WEIGHTED_INTERLEAVE)
>
> Using current->il_prev between these two policies, is just plain incorrect,
> so I will need to rethink this, and the existing code will need to be
> updated such that weighted_interleave does not use current->il_prev.
IIUC, weighted_interleave_nodes() is only used for mempolicy of tasks
(set_mempolicy()), as in the following code.
+ *nid = (ilx == NO_INTERLEAVE_INDEX) ?
+ weighted_interleave_nodes(pol) :
+ weighted_interleave_nid(pol, ilx);
But, in contrast, it's bad to put task-local "current weight" in
mempolicy. So, I think that it's better to move cur_il_weight to
task_struct. And maybe combine it with current->il_prev.
--
Best Regards,
Huang, Ying
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox