| Message ID | 20221025170519.314511-1-hannes@cmpxchg.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: vmscan: split khugepaged stats from direct reclaim stats | expand |
On Tue, Oct 25, 2022 at 01:05:19PM -0400, Johannes Weiner wrote: > +static int reclaimer_offset(void) > +{ > + BUILD_BUG_ON(PGSTEAL_DIRECT - PGSTEAL_KSWAPD != 1); > + BUILD_BUG_ON(PGSTEAL_KHUGEPAGED - PGSTEAL_KSWAPD != 2); > + BUILD_BUG_ON(PGDEMOTE_DIRECT - PGDEMOTE_KSWAPD != 1); > + BUILD_BUG_ON(PGDEMOTE_KHUGEPAGED - PGDEMOTE_KSWAPD != 2); > + BUILD_BUG_ON(PGSCAN_DIRECT - PGSCAN_KSWAPD != 1); > + BUILD_BUG_ON(PGSCAN_KHUGEPAGED - PGSCAN_KSWAPD != 2); > + > + if (current_is_kswapd()) > + return 0; > + if (current_is_khugepaged()) > + return 2; > + return 1; > +} Would this be simpler as ... BUILD_BUG_ON(PGSTEAL_DIRECT - PGSTEAL_KSWAPD != PGDEMOTE_DIRECT - PGDEMOTE_KSWAPD); BUILD_BUG_ON(PGSTEAL_DIRECT - PGSTEAL_KSWAPD != PGSCAN_DIRECT - PGSCAN_KSWAPD); BUILD_BUG_ON(PGSTEAL_KHUGEPAGED - PGSTEAL_KSWAPD != PGDEMOTE_KHUGEPAGED - PGDEMOTE_KSWAPD); BUILD_BUG_ON(PGSTEAL_KHUGEPAGED - PGSTEAL_KSWAPD != PGSCAN_KHUGEPAGED - PGDEMOTE_KSWAPD); if (current_is_kswapd()) return 0; if (current_is_khugepaged()) return PGSTEAL_KHUGEPAGED - PGSTEAL_KSWAPD; return PGSTEAL_DIRECT - PGSTEAL_KSWAPD; Not that I think we'd ever want to separate them, but it is perhaps a bit less magic?
On Tue, Oct 25, 2022 at 10:05 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > Direct reclaim stats are useful for identifying a potential source for > application latency, as well as spotting issues with kswapd. However, > khugepaged currently distorts the picture: as a kernel thread it > doesn't impose allocation latencies on userspace, and it explicitly > opts out of kswapd reclaim. Its activity showing up in the direct > reclaim stats is misleading. Counting it as kswapd reclaim could also > cause confusion when trying to understand actual kswapd behavior. > > Break out khugepaged from the direct reclaim counters into new > pgsteal_khugepaged, pgdemote_khugepaged, pgscan_khugepaged counters. > > Test with a huge executable (CONFIG_READ_ONLY_THP_FOR_FS): > > pgsteal_kswapd 1342185 > pgsteal_direct 0 > pgsteal_khugepaged 3623 > pgscan_kswapd 1345025 > pgscan_direct 0 > pgscan_khugepaged 3623 There are other kernel threads or works may allocate memory then trigger memory reclaim, there may be similar problems for them and someone may try to add a new stat. So how's about we make the stats more general, for example, call it "pg{steal|scan}_kthread"? > > Reported-by: Eric Bergen <ebergen@meta.com> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > --- > Documentation/admin-guide/cgroup-v2.rst | 6 +++++ > include/linux/khugepaged.h | 6 +++++ > include/linux/vm_event_item.h | 3 +++ > mm/khugepaged.c | 5 +++++ > mm/memcontrol.c | 8 +++++-- > mm/vmscan.c | 30 ++++++++++++++++++------- > mm/vmstat.c | 3 +++ > 7 files changed, 51 insertions(+), 10 deletions(-) > > diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst > index dc254a3cb956..74cec76be9f2 100644 > --- a/Documentation/admin-guide/cgroup-v2.rst > +++ b/Documentation/admin-guide/cgroup-v2.rst > @@ -1488,12 +1488,18 @@ PAGE_SIZE multiple when read back. > pgscan_direct (npn) > Amount of scanned pages directly (in an inactive LRU list) > > + pgscan_khugepaged (npn) > + Amount of scanned pages by khugepaged (in an inactive LRU list) > + > pgsteal_kswapd (npn) > Amount of reclaimed pages by kswapd > > pgsteal_direct (npn) > Amount of reclaimed pages directly > > + pgsteal_khugepaged (npn) > + Amount of reclaimed pages by khugepaged > + > pgfault (npn) > Total number of page faults incurred > > diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h > index 70162d707caf..f68865e19b0b 100644 > --- a/include/linux/khugepaged.h > +++ b/include/linux/khugepaged.h > @@ -15,6 +15,7 @@ extern void __khugepaged_exit(struct mm_struct *mm); > extern void khugepaged_enter_vma(struct vm_area_struct *vma, > unsigned long vm_flags); > extern void khugepaged_min_free_kbytes_update(void); > +extern bool current_is_khugepaged(void); > #ifdef CONFIG_SHMEM > extern int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr, > bool install_pmd); > @@ -57,6 +58,11 @@ static inline int collapse_pte_mapped_thp(struct mm_struct *mm, > static inline void khugepaged_min_free_kbytes_update(void) > { > } > + > +static inline bool current_is_khugepaged(void) > +{ > + return false; > +} > #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ > > #endif /* _LINUX_KHUGEPAGED_H */ > diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h > index 3518dba1e02f..7f5d1caf5890 100644 > --- a/include/linux/vm_event_item.h > +++ b/include/linux/vm_event_item.h > @@ -40,10 +40,13 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT, > PGREUSE, > PGSTEAL_KSWAPD, > PGSTEAL_DIRECT, > + PGSTEAL_KHUGEPAGED, > PGDEMOTE_KSWAPD, > PGDEMOTE_DIRECT, > + PGDEMOTE_KHUGEPAGED, > PGSCAN_KSWAPD, > PGSCAN_DIRECT, > + PGSCAN_KHUGEPAGED, > PGSCAN_DIRECT_THROTTLE, > PGSCAN_ANON, > PGSCAN_FILE, > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index 4734315f7940..36318ebbf50d 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -2528,6 +2528,11 @@ void khugepaged_min_free_kbytes_update(void) > mutex_unlock(&khugepaged_mutex); > } > > +bool current_is_khugepaged(void) > +{ > + return kthread_func(current) == khugepaged; > +} > + > static int madvise_collapse_errno(enum scan_result r) > { > /* > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 2d8549ae1b30..a17a5cfa6a55 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -661,8 +661,10 @@ static const unsigned int memcg_vm_event_stat[] = { > PGPGOUT, > PGSCAN_KSWAPD, > PGSCAN_DIRECT, > + PGSCAN_KHUGEPAGED, > PGSTEAL_KSWAPD, > PGSTEAL_DIRECT, > + PGSTEAL_KHUGEPAGED, > PGFAULT, > PGMAJFAULT, > PGREFILL, > @@ -1574,10 +1576,12 @@ static void memory_stat_format(struct mem_cgroup *memcg, char *buf, int bufsize) > /* Accumulated memory events */ > seq_buf_printf(&s, "pgscan %lu\n", > memcg_events(memcg, PGSCAN_KSWAPD) + > - memcg_events(memcg, PGSCAN_DIRECT)); > + memcg_events(memcg, PGSCAN_DIRECT) + > + memcg_events(memcg, PGSCAN_KHUGEPAGED)); > seq_buf_printf(&s, "pgsteal %lu\n", > memcg_events(memcg, PGSTEAL_KSWAPD) + > - memcg_events(memcg, PGSTEAL_DIRECT)); > + memcg_events(memcg, PGSTEAL_DIRECT) + > + memcg_events(memcg, PGSTEAL_KHUGEPAGED)); > > for (i = 0; i < ARRAY_SIZE(memcg_vm_event_stat); i++) { > if (memcg_vm_event_stat[i] == PGPGIN || > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 04d8b88e5216..8ceae125bbf7 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -54,6 +54,7 @@ > #include <linux/shmem_fs.h> > #include <linux/ctype.h> > #include <linux/debugfs.h> > +#include <linux/khugepaged.h> > > #include <asm/tlbflush.h> > #include <asm/div64.h> > @@ -1047,6 +1048,22 @@ void drop_slab(void) > drop_slab_node(nid); > } > > +static int reclaimer_offset(void) > +{ > + BUILD_BUG_ON(PGSTEAL_DIRECT - PGSTEAL_KSWAPD != 1); > + BUILD_BUG_ON(PGSTEAL_KHUGEPAGED - PGSTEAL_KSWAPD != 2); > + BUILD_BUG_ON(PGDEMOTE_DIRECT - PGDEMOTE_KSWAPD != 1); > + BUILD_BUG_ON(PGDEMOTE_KHUGEPAGED - PGDEMOTE_KSWAPD != 2); > + BUILD_BUG_ON(PGSCAN_DIRECT - PGSCAN_KSWAPD != 1); > + BUILD_BUG_ON(PGSCAN_KHUGEPAGED - PGSCAN_KSWAPD != 2); > + > + if (current_is_kswapd()) > + return 0; > + if (current_is_khugepaged()) > + return 2; > + return 1; > +} > + > static inline int is_page_cache_freeable(struct folio *folio) > { > /* > @@ -1599,10 +1616,7 @@ static unsigned int demote_folio_list(struct list_head *demote_folios, > (unsigned long)&mtc, MIGRATE_ASYNC, MR_DEMOTION, > &nr_succeeded); > > - if (current_is_kswapd()) > - __count_vm_events(PGDEMOTE_KSWAPD, nr_succeeded); > - else > - __count_vm_events(PGDEMOTE_DIRECT, nr_succeeded); > + __count_vm_events(PGDEMOTE_KSWAPD + reclaimer_offset(), nr_succeeded); > > return nr_succeeded; > } > @@ -2475,7 +2489,7 @@ static unsigned long shrink_inactive_list(unsigned long nr_to_scan, > &nr_scanned, sc, lru); > > __mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, nr_taken); > - item = current_is_kswapd() ? PGSCAN_KSWAPD : PGSCAN_DIRECT; > + item = PGSCAN_KSWAPD + reclaimer_offset(); > if (!cgroup_reclaim(sc)) > __count_vm_events(item, nr_scanned); > __count_memcg_events(lruvec_memcg(lruvec), item, nr_scanned); > @@ -2492,7 +2506,7 @@ static unsigned long shrink_inactive_list(unsigned long nr_to_scan, > move_folios_to_lru(lruvec, &folio_list); > > __mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, -nr_taken); > - item = current_is_kswapd() ? PGSTEAL_KSWAPD : PGSTEAL_DIRECT; > + item = PGSTEAL_KSWAPD + reclaimer_offset(); > if (!cgroup_reclaim(sc)) > __count_vm_events(item, nr_reclaimed); > __count_memcg_events(lruvec_memcg(lruvec), item, nr_reclaimed); > @@ -4857,7 +4871,7 @@ static int scan_folios(struct lruvec *lruvec, struct scan_control *sc, > break; > } > > - item = current_is_kswapd() ? PGSCAN_KSWAPD : PGSCAN_DIRECT; > + item = PGSCAN_KSWAPD + reclaimer_offset(); > if (!cgroup_reclaim(sc)) { > __count_vm_events(item, isolated); > __count_vm_events(PGREFILL, sorted); > @@ -5015,7 +5029,7 @@ static int evict_folios(struct lruvec *lruvec, struct scan_control *sc, int swap > if (walk && walk->batched) > reset_batch_size(lruvec, walk); > > - item = current_is_kswapd() ? PGSTEAL_KSWAPD : PGSTEAL_DIRECT; > + item = PGSTEAL_KSWAPD + reclaimer_offset(); > if (!cgroup_reclaim(sc)) > __count_vm_events(item, reclaimed); > __count_memcg_events(memcg, item, reclaimed); > diff --git a/mm/vmstat.c b/mm/vmstat.c > index b2371d745e00..1ea6a5ce1c41 100644 > --- a/mm/vmstat.c > +++ b/mm/vmstat.c > @@ -1271,10 +1271,13 @@ const char * const vmstat_text[] = { > "pgreuse", > "pgsteal_kswapd", > "pgsteal_direct", > + "pgsteal_khugepaged", > "pgdemote_kswapd", > "pgdemote_direct", > + "pgdemote_khugepaged", > "pgscan_kswapd", > "pgscan_direct", > + "pgscan_khugepaged", > "pgscan_direct_throttle", > "pgscan_anon", > "pgscan_file", > -- > 2.38.1 > >
On Tue, Oct 25, 2022 at 06:16:53PM +0100, Matthew Wilcox wrote: > On Tue, Oct 25, 2022 at 01:05:19PM -0400, Johannes Weiner wrote: > > +static int reclaimer_offset(void) > > +{ > > + BUILD_BUG_ON(PGSTEAL_DIRECT - PGSTEAL_KSWAPD != 1); > > + BUILD_BUG_ON(PGSTEAL_KHUGEPAGED - PGSTEAL_KSWAPD != 2); > > + BUILD_BUG_ON(PGDEMOTE_DIRECT - PGDEMOTE_KSWAPD != 1); > > + BUILD_BUG_ON(PGDEMOTE_KHUGEPAGED - PGDEMOTE_KSWAPD != 2); > > + BUILD_BUG_ON(PGSCAN_DIRECT - PGSCAN_KSWAPD != 1); > > + BUILD_BUG_ON(PGSCAN_KHUGEPAGED - PGSCAN_KSWAPD != 2); > > + > > + if (current_is_kswapd()) > > + return 0; > > + if (current_is_khugepaged()) > > + return 2; > > + return 1; > > +} > > Would this be simpler as ... > > BUILD_BUG_ON(PGSTEAL_DIRECT - PGSTEAL_KSWAPD != > PGDEMOTE_DIRECT - PGDEMOTE_KSWAPD); > BUILD_BUG_ON(PGSTEAL_DIRECT - PGSTEAL_KSWAPD != > PGSCAN_DIRECT - PGSCAN_KSWAPD); > BUILD_BUG_ON(PGSTEAL_KHUGEPAGED - PGSTEAL_KSWAPD != > PGDEMOTE_KHUGEPAGED - PGDEMOTE_KSWAPD); > BUILD_BUG_ON(PGSTEAL_KHUGEPAGED - PGSTEAL_KSWAPD != > PGSCAN_KHUGEPAGED - PGDEMOTE_KSWAPD); > > if (current_is_kswapd()) > return 0; > if (current_is_khugepaged()) > return PGSTEAL_KHUGEPAGED - PGSTEAL_KSWAPD; > return PGSTEAL_DIRECT - PGSTEAL_KSWAPD; > > Not that I think we'd ever want to separate them, but it is perhaps a > bit less magic? Yeah that looks better. I'll do that in v2, thanks!
On Tue, Oct 25, 2022 at 12:40:15PM -0700, Yang Shi wrote: > On Tue, Oct 25, 2022 at 10:05 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > Direct reclaim stats are useful for identifying a potential source for > > application latency, as well as spotting issues with kswapd. However, > > khugepaged currently distorts the picture: as a kernel thread it > > doesn't impose allocation latencies on userspace, and it explicitly > > opts out of kswapd reclaim. Its activity showing up in the direct > > reclaim stats is misleading. Counting it as kswapd reclaim could also > > cause confusion when trying to understand actual kswapd behavior. > > > > Break out khugepaged from the direct reclaim counters into new > > pgsteal_khugepaged, pgdemote_khugepaged, pgscan_khugepaged counters. > > > > Test with a huge executable (CONFIG_READ_ONLY_THP_FOR_FS): > > > > pgsteal_kswapd 1342185 > > pgsteal_direct 0 > > pgsteal_khugepaged 3623 > > pgscan_kswapd 1345025 > > pgscan_direct 0 > > pgscan_khugepaged 3623 > > There are other kernel threads or works may allocate memory then > trigger memory reclaim, there may be similar problems for them and > someone may try to add a new stat. So how's about we make the stats > more general, for example, call it "pg{steal|scan}_kthread"? I'm not convinved that's a good idea. Can you generally say that userspace isn't indirectly waiting for one of those allocating threads? With khugepaged, we know. And those other allocations are usually ___GFP_KSWAPD_RECLAIM, so if they do direct reclaim, we'd probably want to know that kswapd is failing to keep up (doubly so if userspace is waiting). In a shared kthread counter, khugepaged would again muddy the waters.
On Tue, Oct 25, 2022 at 1:54 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Tue, Oct 25, 2022 at 12:40:15PM -0700, Yang Shi wrote: > > On Tue, Oct 25, 2022 at 10:05 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > > > Direct reclaim stats are useful for identifying a potential source for > > > application latency, as well as spotting issues with kswapd. However, > > > khugepaged currently distorts the picture: as a kernel thread it > > > doesn't impose allocation latencies on userspace, and it explicitly > > > opts out of kswapd reclaim. Its activity showing up in the direct > > > reclaim stats is misleading. Counting it as kswapd reclaim could also > > > cause confusion when trying to understand actual kswapd behavior. > > > > > > Break out khugepaged from the direct reclaim counters into new > > > pgsteal_khugepaged, pgdemote_khugepaged, pgscan_khugepaged counters. > > > > > > Test with a huge executable (CONFIG_READ_ONLY_THP_FOR_FS): > > > > > > pgsteal_kswapd 1342185 > > > pgsteal_direct 0 > > > pgsteal_khugepaged 3623 > > > pgscan_kswapd 1345025 > > > pgscan_direct 0 > > > pgscan_khugepaged 3623 > > > > There are other kernel threads or works may allocate memory then > > trigger memory reclaim, there may be similar problems for them and > > someone may try to add a new stat. So how's about we make the stats > > more general, for example, call it "pg{steal|scan}_kthread"? > > I'm not convinved that's a good idea. > > Can you generally say that userspace isn't indirectly waiting for one > of those allocating threads? With khugepaged, we know. AFAIK, ksm may do slab allocation with __GFP_DIRECT_RECLAIM. Some device mapper drivers may do heavy lift in the work queue, for example, dm-crypt, particularly for writing. > > And those other allocations are usually ___GFP_KSWAPD_RECLAIM, so if > they do direct reclaim, we'd probably want to know that kswapd is > failing to keep up (doubly so if userspace is waiting). In a shared > kthread counter, khugepaged would again muddy the waters.
On Tue, Oct 25, 2022 at 02:53:01PM -0700, Yang Shi wrote: > On Tue, Oct 25, 2022 at 1:54 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > On Tue, Oct 25, 2022 at 12:40:15PM -0700, Yang Shi wrote: > > > On Tue, Oct 25, 2022 at 10:05 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > > > > > Direct reclaim stats are useful for identifying a potential source for > > > > application latency, as well as spotting issues with kswapd. However, > > > > khugepaged currently distorts the picture: as a kernel thread it > > > > doesn't impose allocation latencies on userspace, and it explicitly > > > > opts out of kswapd reclaim. Its activity showing up in the direct > > > > reclaim stats is misleading. Counting it as kswapd reclaim could also > > > > cause confusion when trying to understand actual kswapd behavior. > > > > > > > > Break out khugepaged from the direct reclaim counters into new > > > > pgsteal_khugepaged, pgdemote_khugepaged, pgscan_khugepaged counters. > > > > > > > > Test with a huge executable (CONFIG_READ_ONLY_THP_FOR_FS): > > > > > > > > pgsteal_kswapd 1342185 > > > > pgsteal_direct 0 > > > > pgsteal_khugepaged 3623 > > > > pgscan_kswapd 1345025 > > > > pgscan_direct 0 > > > > pgscan_khugepaged 3623 > > > > > > There are other kernel threads or works may allocate memory then > > > trigger memory reclaim, there may be similar problems for them and > > > someone may try to add a new stat. So how's about we make the stats > > > more general, for example, call it "pg{steal|scan}_kthread"? > > > > I'm not convinved that's a good idea. > > > > Can you generally say that userspace isn't indirectly waiting for one > > of those allocating threads? With khugepaged, we know. > > AFAIK, ksm may do slab allocation with __GFP_DIRECT_RECLAIM. Right, but ksm also uses __GFP_KSWAPD_RECLAIM. So while userspace isn't directly waiting for ksm, when ksm enters direct reclaim it's because kswapd failed. This is of interest to kernel developers. Userspace will likely see direct reclaim in that scenario as well, so the ksm direct reclaim counts aren't liable to confuse users. Khugepaged on the other hand will *always* reclaim directly, even if there is no memory pressure or kswapd failure. The direct reclaim counts there are misleading to both developers and users. What it really should be is pgscan_nokswapd_nouserprocesswaiting, but that just seems kind of long ;-) I'm also not sure anybody but khugepaged is doing direct reclaim without kswapd reclaim. It seems unlikely we'll get more of those. > Some device mapper drivers may do heavy lift in the work queue, for > example, dm-crypt, particularly for writing. Userspace will wait for those through dirty throttling. We'd want to know about kswapd failures in that case - again, without them being muddied by khugepaged. > > And those other allocations are usually ___GFP_KSWAPD_RECLAIM, so if > > they do direct reclaim, we'd probably want to know that kswapd is > > failing to keep up (doubly so if userspace is waiting). In a shared > > kthread counter, khugepaged would again muddy the waters.
On Wed, Oct 26, 2022 at 10:32 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Tue, Oct 25, 2022 at 02:53:01PM -0700, Yang Shi wrote: > > On Tue, Oct 25, 2022 at 1:54 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > > > On Tue, Oct 25, 2022 at 12:40:15PM -0700, Yang Shi wrote: > > > > On Tue, Oct 25, 2022 at 10:05 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > > > > > > > Direct reclaim stats are useful for identifying a potential source for > > > > > application latency, as well as spotting issues with kswapd. However, > > > > > khugepaged currently distorts the picture: as a kernel thread it > > > > > doesn't impose allocation latencies on userspace, and it explicitly > > > > > opts out of kswapd reclaim. Its activity showing up in the direct > > > > > reclaim stats is misleading. Counting it as kswapd reclaim could also > > > > > cause confusion when trying to understand actual kswapd behavior. > > > > > > > > > > Break out khugepaged from the direct reclaim counters into new > > > > > pgsteal_khugepaged, pgdemote_khugepaged, pgscan_khugepaged counters. > > > > > > > > > > Test with a huge executable (CONFIG_READ_ONLY_THP_FOR_FS): > > > > > > > > > > pgsteal_kswapd 1342185 > > > > > pgsteal_direct 0 > > > > > pgsteal_khugepaged 3623 > > > > > pgscan_kswapd 1345025 > > > > > pgscan_direct 0 > > > > > pgscan_khugepaged 3623 > > > > > > > > There are other kernel threads or works may allocate memory then > > > > trigger memory reclaim, there may be similar problems for them and > > > > someone may try to add a new stat. So how's about we make the stats > > > > more general, for example, call it "pg{steal|scan}_kthread"? > > > > > > I'm not convinved that's a good idea. > > > > > > Can you generally say that userspace isn't indirectly waiting for one > > > of those allocating threads? With khugepaged, we know. > > > > AFAIK, ksm may do slab allocation with __GFP_DIRECT_RECLAIM. > > Right, but ksm also uses __GFP_KSWAPD_RECLAIM. So while userspace > isn't directly waiting for ksm, when ksm enters direct reclaim it's > because kswapd failed. This is of interest to kernel developers. > Userspace will likely see direct reclaim in that scenario as well, so > the ksm direct reclaim counts aren't liable to confuse users. > > Khugepaged on the other hand will *always* reclaim directly, even if > there is no memory pressure or kswapd failure. The direct reclaim > counts there are misleading to both developers and users. > > What it really should be is pgscan_nokswapd_nouserprocesswaiting, but > that just seems kind of long ;-) > > I'm also not sure anybody but khugepaged is doing direct reclaim > without kswapd reclaim. It seems unlikely we'll get more of those. IIUC you actually don't care about how many direct reclaim are triggered by khugepaged, but you would like to separate the direct reclaim stats between that are triggered directly by userspace actions, which may stall userspace, and that aren't, which don't stall userspace. If so it doesn't sound that important to distinguish whether the direct reclaim are triggered by khugepaged or other kernel threads even though other kthreads are not liable to confuse users IMHO. > > > Some device mapper drivers may do heavy lift in the work queue, for > > example, dm-crypt, particularly for writing. > > Userspace will wait for those through dirty throttling. We'd want to Not guaranteed. Anyway I just thought pgscan_khugepaged might be more confusing for the users even the developers who are not familiar with THP/khugepaged. > know about kswapd failures in that case - again, without them being > muddied by khugepaged. > > > > And those other allocations are usually ___GFP_KSWAPD_RECLAIM, so if > > > they do direct reclaim, we'd probably want to know that kswapd is > > > failing to keep up (doubly so if userspace is waiting). In a shared > > > kthread counter, khugepaged would again muddy the waters.
On Wed, Oct 26, 2022 at 1:51 PM Yang Shi <shy828301@gmail.com> wrote: > > On Wed, Oct 26, 2022 at 10:32 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > On Tue, Oct 25, 2022 at 02:53:01PM -0700, Yang Shi wrote: > > > On Tue, Oct 25, 2022 at 1:54 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > > > > > On Tue, Oct 25, 2022 at 12:40:15PM -0700, Yang Shi wrote: > > > > > On Tue, Oct 25, 2022 at 10:05 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > > > > > > > > > Direct reclaim stats are useful for identifying a potential source for > > > > > > application latency, as well as spotting issues with kswapd. However, > > > > > > khugepaged currently distorts the picture: as a kernel thread it > > > > > > doesn't impose allocation latencies on userspace, and it explicitly > > > > > > opts out of kswapd reclaim. Its activity showing up in the direct > > > > > > reclaim stats is misleading. Counting it as kswapd reclaim could also > > > > > > cause confusion when trying to understand actual kswapd behavior. > > > > > > > > > > > > Break out khugepaged from the direct reclaim counters into new > > > > > > pgsteal_khugepaged, pgdemote_khugepaged, pgscan_khugepaged counters. > > > > > > > > > > > > Test with a huge executable (CONFIG_READ_ONLY_THP_FOR_FS): > > > > > > > > > > > > pgsteal_kswapd 1342185 > > > > > > pgsteal_direct 0 > > > > > > pgsteal_khugepaged 3623 > > > > > > pgscan_kswapd 1345025 > > > > > > pgscan_direct 0 > > > > > > pgscan_khugepaged 3623 > > > > > > > > > > There are other kernel threads or works may allocate memory then > > > > > trigger memory reclaim, there may be similar problems for them and > > > > > someone may try to add a new stat. So how's about we make the stats > > > > > more general, for example, call it "pg{steal|scan}_kthread"? > > > > > > > > I'm not convinved that's a good idea. > > > > > > > > Can you generally say that userspace isn't indirectly waiting for one > > > > of those allocating threads? With khugepaged, we know. > > > > > > AFAIK, ksm may do slab allocation with __GFP_DIRECT_RECLAIM. > > > > Right, but ksm also uses __GFP_KSWAPD_RECLAIM. So while userspace > > isn't directly waiting for ksm, when ksm enters direct reclaim it's > > because kswapd failed. This is of interest to kernel developers. > > Userspace will likely see direct reclaim in that scenario as well, so > > the ksm direct reclaim counts aren't liable to confuse users. > > > > Khugepaged on the other hand will *always* reclaim directly, even if > > there is no memory pressure or kswapd failure. The direct reclaim > > counts there are misleading to both developers and users. > > > > What it really should be is pgscan_nokswapd_nouserprocesswaiting, but > > that just seems kind of long ;-) > > > > I'm also not sure anybody but khugepaged is doing direct reclaim > > without kswapd reclaim. It seems unlikely we'll get more of those. > > IIUC you actually don't care about how many direct reclaim are > triggered by khugepaged, but you would like to separate the direct > reclaim stats between that are triggered directly by userspace > actions, which may stall userspace, and that aren't, which don't stall > userspace. If so it doesn't sound that important to distinguish > whether the direct reclaim are triggered by khugepaged or other kernel > threads even though other kthreads are not liable to confuse users > IMHO. My 2c, if we care about direct reclaim as in reclaim that may stall user space application allocations, then there are other reclaim contexts that may pollute the direct reclaim stats. For instance, proactive reclaim, or reclaim done by writing a limit lower than the current usage to memory.max or memory.high, as they are not done in the context of the application allocating memory. At Google, we have some internal direct reclaim memcg statistics, and the way we handle this is by passing a flag from such contexts to try_to_free_mem_cgroup_pages() in the reclaim_options arg. This flag is echod into a scan_struct bit, which we then use to filter out direct reclaim operations that actually cause latencies in user space allocations. Perhaps something similar might be more generic here? I am not sure what context khugepaged reclaims memory from, but I think it's not a memcg context, so maybe we want to generalize the reclaim_options arg to try_to_free_pages() or whatever interface khugepaged uses to free memory. > > > > > > Some device mapper drivers may do heavy lift in the work queue, for > > > example, dm-crypt, particularly for writing. > > > > Userspace will wait for those through dirty throttling. We'd want to > > Not guaranteed. > > Anyway I just thought pgscan_khugepaged might be more confusing for > the users even the developers who are not familiar with > THP/khugepaged. > > > know about kswapd failures in that case - again, without them being > > muddied by khugepaged. > > > > > > And those other allocations are usually ___GFP_KSWAPD_RECLAIM, so if > > > > they do direct reclaim, we'd probably want to know that kswapd is > > > > failing to keep up (doubly so if userspace is waiting). In a shared > > > > kthread counter, khugepaged would again muddy the waters. >
On Wed, Oct 26, 2022 at 07:41:21PM -0700, Yosry Ahmed wrote: > On Wed, Oct 26, 2022 at 1:51 PM Yang Shi <shy828301@gmail.com> wrote: > > > > On Wed, Oct 26, 2022 at 10:32 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > > > On Tue, Oct 25, 2022 at 02:53:01PM -0700, Yang Shi wrote: > > > > On Tue, Oct 25, 2022 at 1:54 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > > > > > > > On Tue, Oct 25, 2022 at 12:40:15PM -0700, Yang Shi wrote: > > > > > > On Tue, Oct 25, 2022 at 10:05 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > > > > > > > > > > > Direct reclaim stats are useful for identifying a potential source for > > > > > > > application latency, as well as spotting issues with kswapd. However, > > > > > > > khugepaged currently distorts the picture: as a kernel thread it > > > > > > > doesn't impose allocation latencies on userspace, and it explicitly > > > > > > > opts out of kswapd reclaim. Its activity showing up in the direct > > > > > > > reclaim stats is misleading. Counting it as kswapd reclaim could also > > > > > > > cause confusion when trying to understand actual kswapd behavior. > > > > > > > > > > > > > > Break out khugepaged from the direct reclaim counters into new > > > > > > > pgsteal_khugepaged, pgdemote_khugepaged, pgscan_khugepaged counters. > > > > > > > > > > > > > > Test with a huge executable (CONFIG_READ_ONLY_THP_FOR_FS): > > > > > > > > > > > > > > pgsteal_kswapd 1342185 > > > > > > > pgsteal_direct 0 > > > > > > > pgsteal_khugepaged 3623 > > > > > > > pgscan_kswapd 1345025 > > > > > > > pgscan_direct 0 > > > > > > > pgscan_khugepaged 3623 > > > > > > > > > > > > There are other kernel threads or works may allocate memory then > > > > > > trigger memory reclaim, there may be similar problems for them and > > > > > > someone may try to add a new stat. So how's about we make the stats > > > > > > more general, for example, call it "pg{steal|scan}_kthread"? > > > > > > > > > > I'm not convinved that's a good idea. > > > > > > > > > > Can you generally say that userspace isn't indirectly waiting for one > > > > > of those allocating threads? With khugepaged, we know. > > > > > > > > AFAIK, ksm may do slab allocation with __GFP_DIRECT_RECLAIM. > > > > > > Right, but ksm also uses __GFP_KSWAPD_RECLAIM. So while userspace > > > isn't directly waiting for ksm, when ksm enters direct reclaim it's > > > because kswapd failed. This is of interest to kernel developers. > > > Userspace will likely see direct reclaim in that scenario as well, so > > > the ksm direct reclaim counts aren't liable to confuse users. > > > > > > Khugepaged on the other hand will *always* reclaim directly, even if > > > there is no memory pressure or kswapd failure. The direct reclaim > > > counts there are misleading to both developers and users. > > > > > > What it really should be is pgscan_nokswapd_nouserprocesswaiting, but > > > that just seems kind of long ;-) > > > > > > I'm also not sure anybody but khugepaged is doing direct reclaim > > > without kswapd reclaim. It seems unlikely we'll get more of those. > > > > IIUC you actually don't care about how many direct reclaim are > > triggered by khugepaged, but you would like to separate the direct > > reclaim stats between that are triggered directly by userspace > > actions, which may stall userspace, and that aren't, which don't stall > > userspace. If so it doesn't sound that important to distinguish > > whether the direct reclaim are triggered by khugepaged or other kernel > > threads even though other kthreads are not liable to confuse users > > IMHO. I feel like I've sufficiently explained my reason for wanting to separate out the __GFP_KSWAPD_RECLAIM special case from other sites. > My 2c, if we care about direct reclaim as in reclaim that may stall > user space application allocations, then there are other reclaim > contexts that may pollute the direct reclaim stats. For instance, > proactive reclaim, or reclaim done by writing a limit lower than the > current usage to memory.max or memory.high, as they are not done in > the context of the application allocating memory. > > At Google, we have some internal direct reclaim memcg statistics, and > the way we handle this is by passing a flag from such contexts to > try_to_free_mem_cgroup_pages() in the reclaim_options arg. This flag > is echod into a scan_struct bit, which we then use to filter out > direct reclaim operations that actually cause latencies in user space > allocations. > > Perhaps something similar might be more generic here? I am not sure > what context khugepaged reclaims memory from, but I think it's not a > memcg context, so maybe we want to generalize the reclaim_options arg > to try_to_free_pages() or whatever interface khugepaged uses to free > memory. So at the /proc/vmstat level, I'm not sure it matters much because it doesn't count any cgroup_reclaim() activity. But at the cgroup level, it sure would be nice to split out proactive reclaim churn. Both in terms of not polluting direct reclaim counts, but also for *knowing* how much proactive reclaim is doing. Do you have separate counters for this?
On Thu, Oct 27, 2022 at 7:15 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Wed, Oct 26, 2022 at 07:41:21PM -0700, Yosry Ahmed wrote: > > On Wed, Oct 26, 2022 at 1:51 PM Yang Shi <shy828301@gmail.com> wrote: > > > > > > On Wed, Oct 26, 2022 at 10:32 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > > > > > On Tue, Oct 25, 2022 at 02:53:01PM -0700, Yang Shi wrote: > > > > > On Tue, Oct 25, 2022 at 1:54 PM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > > > > > > > > > On Tue, Oct 25, 2022 at 12:40:15PM -0700, Yang Shi wrote: > > > > > > > On Tue, Oct 25, 2022 at 10:05 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > > > > > > > > > > > > > > Direct reclaim stats are useful for identifying a potential source for > > > > > > > > application latency, as well as spotting issues with kswapd. However, > > > > > > > > khugepaged currently distorts the picture: as a kernel thread it > > > > > > > > doesn't impose allocation latencies on userspace, and it explicitly > > > > > > > > opts out of kswapd reclaim. Its activity showing up in the direct > > > > > > > > reclaim stats is misleading. Counting it as kswapd reclaim could also > > > > > > > > cause confusion when trying to understand actual kswapd behavior. > > > > > > > > > > > > > > > > Break out khugepaged from the direct reclaim counters into new > > > > > > > > pgsteal_khugepaged, pgdemote_khugepaged, pgscan_khugepaged counters. > > > > > > > > > > > > > > > > Test with a huge executable (CONFIG_READ_ONLY_THP_FOR_FS): > > > > > > > > > > > > > > > > pgsteal_kswapd 1342185 > > > > > > > > pgsteal_direct 0 > > > > > > > > pgsteal_khugepaged 3623 > > > > > > > > pgscan_kswapd 1345025 > > > > > > > > pgscan_direct 0 > > > > > > > > pgscan_khugepaged 3623 > > > > > > > > > > > > > > There are other kernel threads or works may allocate memory then > > > > > > > trigger memory reclaim, there may be similar problems for them and > > > > > > > someone may try to add a new stat. So how's about we make the stats > > > > > > > more general, for example, call it "pg{steal|scan}_kthread"? > > > > > > > > > > > > I'm not convinved that's a good idea. > > > > > > > > > > > > Can you generally say that userspace isn't indirectly waiting for one > > > > > > of those allocating threads? With khugepaged, we know. > > > > > > > > > > AFAIK, ksm may do slab allocation with __GFP_DIRECT_RECLAIM. > > > > > > > > Right, but ksm also uses __GFP_KSWAPD_RECLAIM. So while userspace > > > > isn't directly waiting for ksm, when ksm enters direct reclaim it's > > > > because kswapd failed. This is of interest to kernel developers. > > > > Userspace will likely see direct reclaim in that scenario as well, so > > > > the ksm direct reclaim counts aren't liable to confuse users. > > > > > > > > Khugepaged on the other hand will *always* reclaim directly, even if > > > > there is no memory pressure or kswapd failure. The direct reclaim > > > > counts there are misleading to both developers and users. > > > > > > > > What it really should be is pgscan_nokswapd_nouserprocesswaiting, but > > > > that just seems kind of long ;-) > > > > > > > > I'm also not sure anybody but khugepaged is doing direct reclaim > > > > without kswapd reclaim. It seems unlikely we'll get more of those. > > > > > > IIUC you actually don't care about how many direct reclaim are > > > triggered by khugepaged, but you would like to separate the direct > > > reclaim stats between that are triggered directly by userspace > > > actions, which may stall userspace, and that aren't, which don't stall > > > userspace. If so it doesn't sound that important to distinguish > > > whether the direct reclaim are triggered by khugepaged or other kernel > > > threads even though other kthreads are not liable to confuse users > > > IMHO. > > I feel like I've sufficiently explained my reason for wanting to > separate out the __GFP_KSWAPD_RECLAIM special case from other sites. > > > My 2c, if we care about direct reclaim as in reclaim that may stall > > user space application allocations, then there are other reclaim > > contexts that may pollute the direct reclaim stats. For instance, > > proactive reclaim, or reclaim done by writing a limit lower than the > > current usage to memory.max or memory.high, as they are not done in > > the context of the application allocating memory. > > > > At Google, we have some internal direct reclaim memcg statistics, and > > the way we handle this is by passing a flag from such contexts to > > try_to_free_mem_cgroup_pages() in the reclaim_options arg. This flag > > is echod into a scan_struct bit, which we then use to filter out > > direct reclaim operations that actually cause latencies in user space > > allocations. > > > > Perhaps something similar might be more generic here? I am not sure > > what context khugepaged reclaims memory from, but I think it's not a > > memcg context, so maybe we want to generalize the reclaim_options arg > > to try_to_free_pages() or whatever interface khugepaged uses to free > > memory. > > So at the /proc/vmstat level, I'm not sure it matters much because it > doesn't count any cgroup_reclaim() activity. > > But at the cgroup level, it sure would be nice to split out proactive > reclaim churn. Both in terms of not polluting direct reclaim counts, > but also for *knowing* how much proactive reclaim is doing. > > Do you have separate counters for this? Not yet. Currently we only have the first part, not polluting direct reclaim counts. We basically exclude reclaim coming from memory.reclaim, setting memory.max/memory.limit_in_bytes, memory.high (on write, not hitting the high limit), and memory.force_empty from direct reclaim stats. As for having a separate counter for proactive reclaim, do you think it should be limited to reclaim coming from memory.reclaim (and potentially memory.force_empty), or should it include reclaim coming from limit-setting as well?
On Thu, Oct 27, 2022 at 01:43:24PM -0700, Yosry Ahmed wrote: > On Thu, Oct 27, 2022 at 7:15 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Wed, Oct 26, 2022 at 07:41:21PM -0700, Yosry Ahmed wrote: > > > My 2c, if we care about direct reclaim as in reclaim that may stall > > > user space application allocations, then there are other reclaim > > > contexts that may pollute the direct reclaim stats. For instance, > > > proactive reclaim, or reclaim done by writing a limit lower than the > > > current usage to memory.max or memory.high, as they are not done in > > > the context of the application allocating memory. > > > > > > At Google, we have some internal direct reclaim memcg statistics, and > > > the way we handle this is by passing a flag from such contexts to > > > try_to_free_mem_cgroup_pages() in the reclaim_options arg. This flag > > > is echod into a scan_struct bit, which we then use to filter out > > > direct reclaim operations that actually cause latencies in user space > > > allocations. > > > > > > Perhaps something similar might be more generic here? I am not sure > > > what context khugepaged reclaims memory from, but I think it's not a > > > memcg context, so maybe we want to generalize the reclaim_options arg > > > to try_to_free_pages() or whatever interface khugepaged uses to free > > > memory. > > > > So at the /proc/vmstat level, I'm not sure it matters much because it > > doesn't count any cgroup_reclaim() activity. > > > > But at the cgroup level, it sure would be nice to split out proactive > > reclaim churn. Both in terms of not polluting direct reclaim counts, > > but also for *knowing* how much proactive reclaim is doing. > > > > Do you have separate counters for this? > > Not yet. Currently we only have the first part, not polluting direct > reclaim counts. > > We basically exclude reclaim coming from memory.reclaim, setting > memory.max/memory.limit_in_bytes, memory.high (on write, not hitting > the high limit), and memory.force_empty from direct reclaim stats. > > As for having a separate counter for proactive reclaim, do you think > it should be limited to reclaim coming from memory.reclaim (and > potentially memory.force_empty), or should it include reclaim coming > from limit-setting as well? A combined counter seems reasonable to me. We *have* used the limit knobs to drive proactive reclaim in production in the past, so it's not a stretch. And I can't think of a scenario where you'd like them to be separate. I could think of two ways of describing it: pgscan_user: User-requested reclaim. Could be confusing if we ever have an in-kernel proactive reclaim driver - unless that would then go to another counter (new or kswapd). pgscan_ext: Reclaim activity from extraordinary/external requests. External as in: outside the allocation context.
On Fri, Oct 28, 2022 at 7:39 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Thu, Oct 27, 2022 at 01:43:24PM -0700, Yosry Ahmed wrote: > > On Thu, Oct 27, 2022 at 7:15 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > On Wed, Oct 26, 2022 at 07:41:21PM -0700, Yosry Ahmed wrote: > > > > My 2c, if we care about direct reclaim as in reclaim that may stall > > > > user space application allocations, then there are other reclaim > > > > contexts that may pollute the direct reclaim stats. For instance, > > > > proactive reclaim, or reclaim done by writing a limit lower than the > > > > current usage to memory.max or memory.high, as they are not done in > > > > the context of the application allocating memory. > > > > > > > > At Google, we have some internal direct reclaim memcg statistics, and > > > > the way we handle this is by passing a flag from such contexts to > > > > try_to_free_mem_cgroup_pages() in the reclaim_options arg. This flag > > > > is echod into a scan_struct bit, which we then use to filter out > > > > direct reclaim operations that actually cause latencies in user space > > > > allocations. > > > > > > > > Perhaps something similar might be more generic here? I am not sure > > > > what context khugepaged reclaims memory from, but I think it's not a > > > > memcg context, so maybe we want to generalize the reclaim_options arg > > > > to try_to_free_pages() or whatever interface khugepaged uses to free > > > > memory. > > > > > > So at the /proc/vmstat level, I'm not sure it matters much because it > > > doesn't count any cgroup_reclaim() activity. > > > > > > But at the cgroup level, it sure would be nice to split out proactive > > > reclaim churn. Both in terms of not polluting direct reclaim counts, > > > but also for *knowing* how much proactive reclaim is doing. > > > > > > Do you have separate counters for this? > > > > Not yet. Currently we only have the first part, not polluting direct > > reclaim counts. > > > > We basically exclude reclaim coming from memory.reclaim, setting > > memory.max/memory.limit_in_bytes, memory.high (on write, not hitting > > the high limit), and memory.force_empty from direct reclaim stats. > > > > As for having a separate counter for proactive reclaim, do you think > > it should be limited to reclaim coming from memory.reclaim (and > > potentially memory.force_empty), or should it include reclaim coming > > from limit-setting as well? > > A combined counter seems reasonable to me. We *have* used the limit > knobs to drive proactive reclaim in production in the past, so it's > not a stretch. And I can't think of a scenario where you'd like them > to be separate. > > I could think of two ways of describing it: > > pgscan_user: User-requested reclaim. Could be confusing if we ever > have an in-kernel proactive reclaim driver - unless that would then go > to another counter (new or kswapd). > > pgscan_ext: Reclaim activity from extraordinary/external > requests. External as in: outside the allocation context. I imagine if the kernel is doing proactive reclaim on its own, we might want a separate counter for that anyway to monitor what the kernel is doing. So maybe pgscan_user sounds nice for now, but I also like that the latter explicitly says "this is external to the allocation context". But we can just go with pgscan_user and document it properly. How would khugepaged fit in this story? Seems like it would be part of pgscan_ext but not pgscan_user. I imagine we also don't want to pollute proactive reclaim counters with khugepaged reclaim (or other non-direct reclaim). Maybe pgscan_user and pgscan_kernel/pgscan_indirect for things like khugepaged? The problem with pgscan_kernel/indirect is that if we add a proactive reclaim kthread in the future it would technically fit there but we would want a separate counter for it. I am honestly not sure where to put khugepaged. The reasons I don't like a dedicated counter for khugepaged are: - What if other kthreads like khugepaged start doing the same, do we add one counter per-thread? - What if we deprecate khugepaged (or such threads)? Seems more likely than deprecating kswapd. Looks like we want a stat that would group all of this reclaim coming from non-direct kthreads, but would not include a future proactive reclaim kthread.
On Fri, Oct 28, 2022 at 10:41:17AM -0700, Yosry Ahmed wrote: > On Fri, Oct 28, 2022 at 7:39 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > pgscan_user: User-requested reclaim. Could be confusing if we ever > > have an in-kernel proactive reclaim driver - unless that would then go > > to another counter (new or kswapd). > > > > pgscan_ext: Reclaim activity from extraordinary/external > > requests. External as in: outside the allocation context. > > I imagine if the kernel is doing proactive reclaim on its own, we > might want a separate counter for that anyway to monitor what the > kernel is doing. So maybe pgscan_user sounds nice for now, but I also > like that the latter explicitly says "this is external to the > allocation context". But we can just go with pgscan_user and document > it properly. Yes, I think you're right. pgscan_user sounds good to me. > How would khugepaged fit in this story? Seems like it would be part of > pgscan_ext but not pgscan_user. I imagine we also don't want to > pollute proactive reclaim counters with khugepaged reclaim (or other > non-direct reclaim). > > Maybe pgscan_user and pgscan_kernel/pgscan_indirect for things like khugepaged? > The problem with pgscan_kernel/indirect is that if we add a proactive > reclaim kthread in the future it would technically fit there but we > would want a separate counter for it. > > I am honestly not sure where to put khugepaged. The reasons I don't > like a dedicated counter for khugepaged are: > - What if other kthreads like khugepaged start doing the same, do we > add one counter per-thread? It's unlikely there will be more. The reason khugepaged doesn't rely on kswapd is unique to THP allocations: they can require an exorbitant amount of work to assemble, but due to fragmentation those requests may fail permanently. We don't want to burden a shared facility like kswapd with large amounts of speculative work on behalf of what are (still*) cornercase requests. This isn't true for other allocations. We do have __GFP_NORETRY sites here and there that rather fall back early than put in the full amount of work; but overall we expect allocations to succeed - and kswapd to be able to balance for them!!** - because the alternative tends to be OOMs, or drivers and workloads aborting on -ENOMEM. (* As we evolve the allocator and normalize huge page requests (folios), kswapd may also eventually balance for THPs again. IOW, it's more likely for this exception to disappear again than it is that we'll see more of them.) (** This is also why it's no big deal if other kthreads that rely on kswapd contribute to direct reclaim stats. First, it's highly error prone to determine on a case by case basis whether userspace could be waiting behind that direct reclaim - as Yang Shi's writeback example demonstrates. Second, if kswapd is overwhelmed, it's likely to impact userspace *anyway*! The benefit of this classification work is questionable.) > - What if we deprecate khugepaged (or such threads)? Seems more likely > than deprecating kswapd. If that happens, we can remove the counter again. The bar isn't as high for vmstat as it for other ABI, and we've updated it plenty of times to reflect changes in the MM implementation. > Looks like we want a stat that would group all of this reclaim coming > from non-direct kthreads, but would not include a future proactive > reclaim kthread. I think the desire to generalize overcomplicates things here in a way that isn't actually meaningful. Think of direct reclaim stats as a signal that either a) kswapd is broken or b) memory pressure is high enough to cause latencies in the class of requests that are of interest to userspace. This is true for all cases but khugepaged.
On Mon, Oct 31, 2022 at 9:00 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Fri, Oct 28, 2022 at 10:41:17AM -0700, Yosry Ahmed wrote: > > On Fri, Oct 28, 2022 at 7:39 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > > pgscan_user: User-requested reclaim. Could be confusing if we ever > > > have an in-kernel proactive reclaim driver - unless that would then go > > > to another counter (new or kswapd). > > > > > > pgscan_ext: Reclaim activity from extraordinary/external > > > requests. External as in: outside the allocation context. > > > > I imagine if the kernel is doing proactive reclaim on its own, we > > might want a separate counter for that anyway to monitor what the > > kernel is doing. So maybe pgscan_user sounds nice for now, but I also > > like that the latter explicitly says "this is external to the > > allocation context". But we can just go with pgscan_user and document > > it properly. > > Yes, I think you're right. pgscan_user sounds good to me. > > > How would khugepaged fit in this story? Seems like it would be part of > > pgscan_ext but not pgscan_user. I imagine we also don't want to > > pollute proactive reclaim counters with khugepaged reclaim (or other > > non-direct reclaim). > > > > Maybe pgscan_user and pgscan_kernel/pgscan_indirect for things like khugepaged? > > The problem with pgscan_kernel/indirect is that if we add a proactive > > reclaim kthread in the future it would technically fit there but we > > would want a separate counter for it. > > > > I am honestly not sure where to put khugepaged. The reasons I don't > > like a dedicated counter for khugepaged are: > > - What if other kthreads like khugepaged start doing the same, do we > > add one counter per-thread? > > It's unlikely there will be more. > > The reason khugepaged doesn't rely on kswapd is unique to THP > allocations: they can require an exorbitant amount of work to > assemble, but due to fragmentation those requests may fail > permanently. We don't want to burden a shared facility like kswapd > with large amounts of speculative work on behalf of what are (still*) > cornercase requests. > > This isn't true for other allocations. We do have __GFP_NORETRY sites > here and there that rather fall back early than put in the full amount > of work; but overall we expect allocations to succeed - and kswapd to > be able to balance for them!!** - because the alternative tends to be > OOMs, or drivers and workloads aborting on -ENOMEM. > > (* As we evolve the allocator and normalize huge page requests > (folios), kswapd may also eventually balance for THPs again. IOW, > it's more likely for this exception to disappear again than it is > that we'll see more of them.) > > (** This is also why it's no big deal if other kthreads that rely on > kswapd contribute to direct reclaim stats. First, it's highly > error prone to determine on a case by case basis whether userspace > could be waiting behind that direct reclaim - as Yang Shi's > writeback example demonstrates. Second, if kswapd is overwhelmed, > it's likely to impact userspace *anyway*! The benefit of this > classification work is questionable.) Thanks for the explanation :) > > > - What if we deprecate khugepaged (or such threads)? Seems more likely > > than deprecating kswapd. > > If that happens, we can remove the counter again. The bar isn't as > high for vmstat as it for other ABI, and we've updated it plenty of > times to reflect changes in the MM implementation. Good to know! I thought we'd be stuck with it forever. > > > Looks like we want a stat that would group all of this reclaim coming > > from non-direct kthreads, but would not include a future proactive > > reclaim kthread. > > I think the desire to generalize overcomplicates things here in a way > that isn't actually meaningful. > > Think of direct reclaim stats as a signal that either a) kswapd is > broken or b) memory pressure is high enough to cause latencies in the > class of requests that are of interest to userspace. This is true for > all cases but khugepaged. Agreed. I believe moving forward with pgscan_user and pgscan_khugepaged style stats makes sense. Thanks, Johannes!
diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst index dc254a3cb956..74cec76be9f2 100644 --- a/Documentation/admin-guide/cgroup-v2.rst +++ b/Documentation/admin-guide/cgroup-v2.rst @@ -1488,12 +1488,18 @@ PAGE_SIZE multiple when read back. pgscan_direct (npn) Amount of scanned pages directly (in an inactive LRU list) + pgscan_khugepaged (npn) + Amount of scanned pages by khugepaged (in an inactive LRU list) + pgsteal_kswapd (npn) Amount of reclaimed pages by kswapd pgsteal_direct (npn) Amount of reclaimed pages directly + pgsteal_khugepaged (npn) + Amount of reclaimed pages by khugepaged + pgfault (npn) Total number of page faults incurred diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h index 70162d707caf..f68865e19b0b 100644 --- a/include/linux/khugepaged.h +++ b/include/linux/khugepaged.h @@ -15,6 +15,7 @@ extern void __khugepaged_exit(struct mm_struct *mm); extern void khugepaged_enter_vma(struct vm_area_struct *vma, unsigned long vm_flags); extern void khugepaged_min_free_kbytes_update(void); +extern bool current_is_khugepaged(void); #ifdef CONFIG_SHMEM extern int collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr, bool install_pmd); @@ -57,6 +58,11 @@ static inline int collapse_pte_mapped_thp(struct mm_struct *mm, static inline void khugepaged_min_free_kbytes_update(void) { } + +static inline bool current_is_khugepaged(void) +{ + return false; +} #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ #endif /* _LINUX_KHUGEPAGED_H */ diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h index 3518dba1e02f..7f5d1caf5890 100644 --- a/include/linux/vm_event_item.h +++ b/include/linux/vm_event_item.h @@ -40,10 +40,13 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT, PGREUSE, PGSTEAL_KSWAPD, PGSTEAL_DIRECT, + PGSTEAL_KHUGEPAGED, PGDEMOTE_KSWAPD, PGDEMOTE_DIRECT, + PGDEMOTE_KHUGEPAGED, PGSCAN_KSWAPD, PGSCAN_DIRECT, + PGSCAN_KHUGEPAGED, PGSCAN_DIRECT_THROTTLE, PGSCAN_ANON, PGSCAN_FILE, diff --git a/mm/khugepaged.c b/mm/khugepaged.c index 4734315f7940..36318ebbf50d 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -2528,6 +2528,11 @@ void khugepaged_min_free_kbytes_update(void) mutex_unlock(&khugepaged_mutex); } +bool current_is_khugepaged(void) +{ + return kthread_func(current) == khugepaged; +} + static int madvise_collapse_errno(enum scan_result r) { /* diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 2d8549ae1b30..a17a5cfa6a55 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -661,8 +661,10 @@ static const unsigned int memcg_vm_event_stat[] = { PGPGOUT, PGSCAN_KSWAPD, PGSCAN_DIRECT, + PGSCAN_KHUGEPAGED, PGSTEAL_KSWAPD, PGSTEAL_DIRECT, + PGSTEAL_KHUGEPAGED, PGFAULT, PGMAJFAULT, PGREFILL, @@ -1574,10 +1576,12 @@ static void memory_stat_format(struct mem_cgroup *memcg, char *buf, int bufsize) /* Accumulated memory events */ seq_buf_printf(&s, "pgscan %lu\n", memcg_events(memcg, PGSCAN_KSWAPD) + - memcg_events(memcg, PGSCAN_DIRECT)); + memcg_events(memcg, PGSCAN_DIRECT) + + memcg_events(memcg, PGSCAN_KHUGEPAGED)); seq_buf_printf(&s, "pgsteal %lu\n", memcg_events(memcg, PGSTEAL_KSWAPD) + - memcg_events(memcg, PGSTEAL_DIRECT)); + memcg_events(memcg, PGSTEAL_DIRECT) + + memcg_events(memcg, PGSTEAL_KHUGEPAGED)); for (i = 0; i < ARRAY_SIZE(memcg_vm_event_stat); i++) { if (memcg_vm_event_stat[i] == PGPGIN || diff --git a/mm/vmscan.c b/mm/vmscan.c index 04d8b88e5216..8ceae125bbf7 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -54,6 +54,7 @@ #include <linux/shmem_fs.h> #include <linux/ctype.h> #include <linux/debugfs.h> +#include <linux/khugepaged.h> #include <asm/tlbflush.h> #include <asm/div64.h> @@ -1047,6 +1048,22 @@ void drop_slab(void) drop_slab_node(nid); } +static int reclaimer_offset(void) +{ + BUILD_BUG_ON(PGSTEAL_DIRECT - PGSTEAL_KSWAPD != 1); + BUILD_BUG_ON(PGSTEAL_KHUGEPAGED - PGSTEAL_KSWAPD != 2); + BUILD_BUG_ON(PGDEMOTE_DIRECT - PGDEMOTE_KSWAPD != 1); + BUILD_BUG_ON(PGDEMOTE_KHUGEPAGED - PGDEMOTE_KSWAPD != 2); + BUILD_BUG_ON(PGSCAN_DIRECT - PGSCAN_KSWAPD != 1); + BUILD_BUG_ON(PGSCAN_KHUGEPAGED - PGSCAN_KSWAPD != 2); + + if (current_is_kswapd()) + return 0; + if (current_is_khugepaged()) + return 2; + return 1; +} + static inline int is_page_cache_freeable(struct folio *folio) { /* @@ -1599,10 +1616,7 @@ static unsigned int demote_folio_list(struct list_head *demote_folios, (unsigned long)&mtc, MIGRATE_ASYNC, MR_DEMOTION, &nr_succeeded); - if (current_is_kswapd()) - __count_vm_events(PGDEMOTE_KSWAPD, nr_succeeded); - else - __count_vm_events(PGDEMOTE_DIRECT, nr_succeeded); + __count_vm_events(PGDEMOTE_KSWAPD + reclaimer_offset(), nr_succeeded); return nr_succeeded; } @@ -2475,7 +2489,7 @@ static unsigned long shrink_inactive_list(unsigned long nr_to_scan, &nr_scanned, sc, lru); __mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, nr_taken); - item = current_is_kswapd() ? PGSCAN_KSWAPD : PGSCAN_DIRECT; + item = PGSCAN_KSWAPD + reclaimer_offset(); if (!cgroup_reclaim(sc)) __count_vm_events(item, nr_scanned); __count_memcg_events(lruvec_memcg(lruvec), item, nr_scanned); @@ -2492,7 +2506,7 @@ static unsigned long shrink_inactive_list(unsigned long nr_to_scan, move_folios_to_lru(lruvec, &folio_list); __mod_node_page_state(pgdat, NR_ISOLATED_ANON + file, -nr_taken); - item = current_is_kswapd() ? PGSTEAL_KSWAPD : PGSTEAL_DIRECT; + item = PGSTEAL_KSWAPD + reclaimer_offset(); if (!cgroup_reclaim(sc)) __count_vm_events(item, nr_reclaimed); __count_memcg_events(lruvec_memcg(lruvec), item, nr_reclaimed); @@ -4857,7 +4871,7 @@ static int scan_folios(struct lruvec *lruvec, struct scan_control *sc, break; } - item = current_is_kswapd() ? PGSCAN_KSWAPD : PGSCAN_DIRECT; + item = PGSCAN_KSWAPD + reclaimer_offset(); if (!cgroup_reclaim(sc)) { __count_vm_events(item, isolated); __count_vm_events(PGREFILL, sorted); @@ -5015,7 +5029,7 @@ static int evict_folios(struct lruvec *lruvec, struct scan_control *sc, int swap if (walk && walk->batched) reset_batch_size(lruvec, walk); - item = current_is_kswapd() ? PGSTEAL_KSWAPD : PGSTEAL_DIRECT; + item = PGSTEAL_KSWAPD + reclaimer_offset(); if (!cgroup_reclaim(sc)) __count_vm_events(item, reclaimed); __count_memcg_events(memcg, item, reclaimed); diff --git a/mm/vmstat.c b/mm/vmstat.c index b2371d745e00..1ea6a5ce1c41 100644 --- a/mm/vmstat.c +++ b/mm/vmstat.c @@ -1271,10 +1271,13 @@ const char * const vmstat_text[] = { "pgreuse", "pgsteal_kswapd", "pgsteal_direct", + "pgsteal_khugepaged", "pgdemote_kswapd", "pgdemote_direct", + "pgdemote_khugepaged", "pgscan_kswapd", "pgscan_direct", + "pgscan_khugepaged", "pgscan_direct_throttle", "pgscan_anon", "pgscan_file",
Direct reclaim stats are useful for identifying a potential source for application latency, as well as spotting issues with kswapd. However, khugepaged currently distorts the picture: as a kernel thread it doesn't impose allocation latencies on userspace, and it explicitly opts out of kswapd reclaim. Its activity showing up in the direct reclaim stats is misleading. Counting it as kswapd reclaim could also cause confusion when trying to understand actual kswapd behavior. Break out khugepaged from the direct reclaim counters into new pgsteal_khugepaged, pgdemote_khugepaged, pgscan_khugepaged counters. Test with a huge executable (CONFIG_READ_ONLY_THP_FOR_FS): pgsteal_kswapd 1342185 pgsteal_direct 0 pgsteal_khugepaged 3623 pgscan_kswapd 1345025 pgscan_direct 0 pgscan_khugepaged 3623 Reported-by: Eric Bergen <ebergen@meta.com> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> --- Documentation/admin-guide/cgroup-v2.rst | 6 +++++ include/linux/khugepaged.h | 6 +++++ include/linux/vm_event_item.h | 3 +++ mm/khugepaged.c | 5 +++++ mm/memcontrol.c | 8 +++++-- mm/vmscan.c | 30 ++++++++++++++++++------- mm/vmstat.c | 3 +++ 7 files changed, 51 insertions(+), 10 deletions(-)