| Message ID | 20240506170024.202111-1-yosryahmed@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: do not update memcg stats for NR_{FILE/SHMEM}_PMDMAPPED | expand |
On Mon, May 6, 2024 at 10:00 AM Yosry Ahmed <yosryahmed@google.com> wrote: > > Do not use __lruvec_stat_mod_folio() when updating NR_FILE_PMDMAPPED and > NR_SHMEM_PMDMAPPED as these stats are not maintained per-memcg. Use > __mod_node_page_state() instead, which updates the global per-node stats > only. > > Reported-by: syzbot+9319a4268a640e26b72b@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/lkml/0000000000001b9d500617c8b23c@google.com > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> > --- > mm/rmap.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/mm/rmap.c b/mm/rmap.c > index 12be4241474ab..c2cfb750d2535 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1435,13 +1435,14 @@ static __always_inline void __folio_add_file_rmap(struct folio *folio, > struct page *page, int nr_pages, struct vm_area_struct *vma, > enum rmap_level level) > { > + pg_data_t *pgdat = folio_pgdat(folio); > int nr, nr_pmdmapped = 0; > > VM_WARN_ON_FOLIO(folio_test_anon(folio), folio); > > nr = __folio_add_rmap(folio, page, nr_pages, level, &nr_pmdmapped); > if (nr_pmdmapped) > - __lruvec_stat_mod_folio(folio, folio_test_swapbacked(folio) ? > + __mod_node_page_state(pgdat, folio_test_swapbacked(folio) ? > NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED, nr_pmdmapped); > if (nr) > __lruvec_stat_mod_folio(folio, NR_FILE_MAPPED, nr); > @@ -1493,6 +1494,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, > enum rmap_level level) > { > atomic_t *mapped = &folio->_nr_pages_mapped; > + pg_data_t *pgdat = folio_pgdat(folio); > int last, nr = 0, nr_pmdmapped = 0; > bool partially_mapped = false; > enum node_stat_item idx; > @@ -1540,13 +1542,14 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, > } > > if (nr_pmdmapped) { > + /* NR_{FILE/SHMEM}_PMDMAPPED are not maintained per-memcg */ > if (folio_test_anon(folio)) > - idx = NR_ANON_THPS; > - else if (folio_test_swapbacked(folio)) > - idx = NR_SHMEM_PMDMAPPED; > + __lruvec_stat_mod_folio(folio, NR_ANON_THPS, -nr_pmdmapped); > else > - idx = NR_FILE_PMDMAPPED; > - __lruvec_stat_mod_folio(folio, idx, -nr_pmdmapped); > + __mod_node_page_state(pgdat, > + folio_test_swapbacked(folio) ? > + NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED, > + nr_pmdmapped); ..and of course right after I press send I realized this should be -nr_pmdmapped. > } > if (nr) { > idx = folio_test_anon(folio) ? NR_ANON_MAPPED : NR_FILE_MAPPED; > -- > 2.45.0.rc1.225.g2a3ae87e7f-goog >
On 06.05.24 19:00, Yosry Ahmed wrote: > Do not use __lruvec_stat_mod_folio() when updating NR_FILE_PMDMAPPED and > NR_SHMEM_PMDMAPPED as these stats are not maintained per-memcg. Use > __mod_node_page_state() instead, which updates the global per-node stats > only. What's the effect of this? IIUC, it's been that way forever, no? Fixes: ? Do we want to CC stable?
On Mon, May 6, 2024 at 11:35 AM David Hildenbrand <david@redhat.com> wrote: > > On 06.05.24 19:00, Yosry Ahmed wrote: > > Do not use __lruvec_stat_mod_folio() when updating NR_FILE_PMDMAPPED and > > NR_SHMEM_PMDMAPPED as these stats are not maintained per-memcg. Use > > __mod_node_page_state() instead, which updates the global per-node stats > > only. > > What's the effect of this? IIUC, it's been that way forever, no? Yes, but it has been the case that all the NR_VM_EVENT_ITEMS stats were maintained per-memcg, although some of those fields are not exposed anywhere. Shakeel recently added commit14e0f6c957e39 ("memcg: reduce memory for the lruvec and memcg stats"), which changed this such that we only maintain the stats we actually expose per-memcg (via a translation table). He also added commit 514462bbe927b ("memcg: warn for unexpected events and stats"), which warns if we try to update a stat per-memcg that we do not maintain per-memcg (i.e. the warning firing here). The goal is to make sure the translation table has all the stats it needs to have. Both of these commits were just merged today into mm-stable, hence the need for the fix now. It is the warning working as intended. No Fixes or CC stable are needed, but if necessary I would think: Fixes: 514462bbe927b ("memcg: warn for unexpected events and stats") , because without the warning, the stat update will just be ignored. So if anything the warning should have been added *after* this was fixed up. > > Fixes: ? > > Do we want to CC stable? > > > -- > Cheers, > > David / dhildenb >
On 06.05.24 20:52, Yosry Ahmed wrote: > On Mon, May 6, 2024 at 11:35 AM David Hildenbrand <david@redhat.com> wrote: >> >> On 06.05.24 19:00, Yosry Ahmed wrote: >>> Do not use __lruvec_stat_mod_folio() when updating NR_FILE_PMDMAPPED and >>> NR_SHMEM_PMDMAPPED as these stats are not maintained per-memcg. Use >>> __mod_node_page_state() instead, which updates the global per-node stats >>> only. >> >> What's the effect of this? IIUC, it's been that way forever, no? > > Yes, but it has been the case that all the NR_VM_EVENT_ITEMS stats > were maintained per-memcg, although some of those fields are not > exposed anywhere. > > Shakeel recently added commit14e0f6c957e39 ("memcg: reduce memory for > the lruvec and memcg stats"), which changed this such that we only > maintain the stats we actually expose per-memcg (via a translation > table). Valuable information we should add to the patch description :) > > He also added commit 514462bbe927b ("memcg: warn for unexpected events > and stats"), which warns if we try to update a stat per-memcg that we > do not maintain per-memcg (i.e. the warning firing here). The goal is > to make sure the translation table has all the stats it needs to have. > > Both of these commits were just merged today into mm-stable, hence the > need for the fix now. It is the warning working as intended. No Fixes > or CC stable are needed, but if necessary I would think: WARN* should usually be "Fixes:"d, because WARN* expresses a condition that shouldn't be happening. Documentation/process/coding-style.rst contains details. > > Fixes: 514462bbe927b ("memcg: warn for unexpected events and stats") > > , because without the warning, the stat update will just be ignored. > So if anything the warning should have been added *after* this was > fixed up. Ideally, yes. But if it's in mm-stable, we usually can no longer reshuffle patches (commit IDs stable).
On Mon, May 6, 2024 at 12:18 PM David Hildenbrand <david@redhat.com> wrote: > > On 06.05.24 20:52, Yosry Ahmed wrote: > > On Mon, May 6, 2024 at 11:35 AM David Hildenbrand <david@redhat.com> wrote: > >> > >> On 06.05.24 19:00, Yosry Ahmed wrote: > >>> Do not use __lruvec_stat_mod_folio() when updating NR_FILE_PMDMAPPED and > >>> NR_SHMEM_PMDMAPPED as these stats are not maintained per-memcg. Use > >>> __mod_node_page_state() instead, which updates the global per-node stats > >>> only. > >> > >> What's the effect of this? IIUC, it's been that way forever, no? > > > > Yes, but it has been the case that all the NR_VM_EVENT_ITEMS stats > > were maintained per-memcg, although some of those fields are not > > exposed anywhere. > > > > Shakeel recently added commit14e0f6c957e39 ("memcg: reduce memory for > > the lruvec and memcg stats"), which changed this such that we only > > maintain the stats we actually expose per-memcg (via a translation > > table). > > Valuable information we should add to the patch description :) > > > > > He also added commit 514462bbe927b ("memcg: warn for unexpected events > > and stats"), which warns if we try to update a stat per-memcg that we > > do not maintain per-memcg (i.e. the warning firing here). The goal is > > to make sure the translation table has all the stats it needs to have. > > > > Both of these commits were just merged today into mm-stable, hence the > > need for the fix now. It is the warning working as intended. No Fixes > > or CC stable are needed, but if necessary I would think: > > WARN* should usually be "Fixes:"d, because WARN* expresses a condition > that shouldn't be happening. > > Documentation/process/coding-style.rst contains details. > > > > > Fixes: 514462bbe927b ("memcg: warn for unexpected events and stats") > > > > , because without the warning, the stat update will just be ignored. > > So if anything the warning should have been added *after* this was > > fixed up. > > Ideally, yes. But if it's in mm-stable, we usually can no longer > reshuffle patches (commit IDs stable). I will send v2 shortly with the missing negative sign, amended commit log, and the Fixes tag. Thanks for taking a look! > -- > Cheers, > > David / dhildenb > >
On Mon, May 06, 2024 at 05:00:24PM +0000, Yosry Ahmed wrote: > Do not use __lruvec_stat_mod_folio() when updating NR_FILE_PMDMAPPED and > NR_SHMEM_PMDMAPPED as these stats are not maintained per-memcg. Use > __mod_node_page_state() instead, which updates the global per-node stats > only. > > Reported-by: syzbot+9319a4268a640e26b72b@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/lkml/0000000000001b9d500617c8b23c@google.com > Signed-off-by: Yosry Ahmed <yosryahmed@google.com> I think we can put Fixes either for 443c077dc2ec ("memcg: cleanup __mod_memcg_lruvec_state") or ad86c0f0e089 ("memcg: warn for unexpected events and stats"). > --- > mm/rmap.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/mm/rmap.c b/mm/rmap.c > index 12be4241474ab..c2cfb750d2535 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1435,13 +1435,14 @@ static __always_inline void __folio_add_file_rmap(struct folio *folio, > struct page *page, int nr_pages, struct vm_area_struct *vma, > enum rmap_level level) > { > + pg_data_t *pgdat = folio_pgdat(folio); > int nr, nr_pmdmapped = 0; > > VM_WARN_ON_FOLIO(folio_test_anon(folio), folio); > > nr = __folio_add_rmap(folio, page, nr_pages, level, &nr_pmdmapped); > if (nr_pmdmapped) > - __lruvec_stat_mod_folio(folio, folio_test_swapbacked(folio) ? > + __mod_node_page_state(pgdat, folio_test_swapbacked(folio) ? > NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED, nr_pmdmapped); > if (nr) > __lruvec_stat_mod_folio(folio, NR_FILE_MAPPED, nr); > @@ -1493,6 +1494,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, > enum rmap_level level) > { > atomic_t *mapped = &folio->_nr_pages_mapped; > + pg_data_t *pgdat = folio_pgdat(folio); > int last, nr = 0, nr_pmdmapped = 0; > bool partially_mapped = false; > enum node_stat_item idx; > @@ -1540,13 +1542,14 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, > } > > if (nr_pmdmapped) { > + /* NR_{FILE/SHMEM}_PMDMAPPED are not maintained per-memcg */ > if (folio_test_anon(folio)) > - idx = NR_ANON_THPS; > - else if (folio_test_swapbacked(folio)) > - idx = NR_SHMEM_PMDMAPPED; > + __lruvec_stat_mod_folio(folio, NR_ANON_THPS, -nr_pmdmapped); > else > - idx = NR_FILE_PMDMAPPED; > - __lruvec_stat_mod_folio(folio, idx, -nr_pmdmapped); > + __mod_node_page_state(pgdat, > + folio_test_swapbacked(folio) ? > + NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED, > + nr_pmdmapped); After the above fixed, you can add: Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
diff --git a/mm/rmap.c b/mm/rmap.c index 12be4241474ab..c2cfb750d2535 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1435,13 +1435,14 @@ static __always_inline void __folio_add_file_rmap(struct folio *folio, struct page *page, int nr_pages, struct vm_area_struct *vma, enum rmap_level level) { + pg_data_t *pgdat = folio_pgdat(folio); int nr, nr_pmdmapped = 0; VM_WARN_ON_FOLIO(folio_test_anon(folio), folio); nr = __folio_add_rmap(folio, page, nr_pages, level, &nr_pmdmapped); if (nr_pmdmapped) - __lruvec_stat_mod_folio(folio, folio_test_swapbacked(folio) ? + __mod_node_page_state(pgdat, folio_test_swapbacked(folio) ? NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED, nr_pmdmapped); if (nr) __lruvec_stat_mod_folio(folio, NR_FILE_MAPPED, nr); @@ -1493,6 +1494,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, enum rmap_level level) { atomic_t *mapped = &folio->_nr_pages_mapped; + pg_data_t *pgdat = folio_pgdat(folio); int last, nr = 0, nr_pmdmapped = 0; bool partially_mapped = false; enum node_stat_item idx; @@ -1540,13 +1542,14 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, } if (nr_pmdmapped) { + /* NR_{FILE/SHMEM}_PMDMAPPED are not maintained per-memcg */ if (folio_test_anon(folio)) - idx = NR_ANON_THPS; - else if (folio_test_swapbacked(folio)) - idx = NR_SHMEM_PMDMAPPED; + __lruvec_stat_mod_folio(folio, NR_ANON_THPS, -nr_pmdmapped); else - idx = NR_FILE_PMDMAPPED; - __lruvec_stat_mod_folio(folio, idx, -nr_pmdmapped); + __mod_node_page_state(pgdat, + folio_test_swapbacked(folio) ? + NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED, + nr_pmdmapped); } if (nr) { idx = folio_test_anon(folio) ? NR_ANON_MAPPED : NR_FILE_MAPPED;
Do not use __lruvec_stat_mod_folio() when updating NR_FILE_PMDMAPPED and NR_SHMEM_PMDMAPPED as these stats are not maintained per-memcg. Use __mod_node_page_state() instead, which updates the global per-node stats only. Reported-by: syzbot+9319a4268a640e26b72b@syzkaller.appspotmail.com Closes: https://lore.kernel.org/lkml/0000000000001b9d500617c8b23c@google.com Signed-off-by: Yosry Ahmed <yosryahmed@google.com> --- mm/rmap.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)