| Message ID | 20201206101451.14706-2-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert all vmstat counters to pages or bytes | expand |
On Sun 06-12-20 18:14:40, Muchun Song wrote: > The unit of NR_ANON_THPS is HPAGE_PMD_NR already. So it should inc/dec > by one rather than nr_pages. > > Fixes: 468c398233da ("mm: memcontrol: switch to native NR_ANON_THPS counter") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> Acked-by: Michal Hocko <mhocko@suse.com> > --- > mm/memcontrol.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 22d9bd688d6d..695dedf8687a 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5634,10 +5634,8 @@ static int mem_cgroup_move_account(struct page *page, > __mod_lruvec_state(from_vec, NR_ANON_MAPPED, -nr_pages); > __mod_lruvec_state(to_vec, NR_ANON_MAPPED, nr_pages); > if (PageTransHuge(page)) { > - __mod_lruvec_state(from_vec, NR_ANON_THPS, > - -nr_pages); > - __mod_lruvec_state(to_vec, NR_ANON_THPS, > - nr_pages); > + __dec_lruvec_state(from_vec, NR_ANON_THPS); > + __inc_lruvec_state(to_vec, NR_ANON_THPS); > } > > } > -- > 2.11.0
On Sun, Dec 06, 2020 at 06:14:40PM +0800, Muchun Song wrote: > The unit of NR_ANON_THPS is HPAGE_PMD_NR already. So it should inc/dec > by one rather than nr_pages. This is a real bug, thanks for catching it. However, your patch changes the user-visible output of /proc/vmstat! NR_ANON_THPS isn't just used by memcg, it's a generic accounting item of the memory subsystem. See this from the Fixes:-patch: - __inc_node_page_state(page, NR_ANON_THPS); + __inc_lruvec_page_state(page, NR_ANON_THPS); While we've considered /proc/vmstat less official than other files like meminfo, and have in the past freely added and removed items, changing the unit of an existing one is not going to work.
On Thu, Dec 10, 2020 at 05:00:47PM +0100, Johannes Weiner wrote: > On Sun, Dec 06, 2020 at 06:14:40PM +0800, Muchun Song wrote: > > The unit of NR_ANON_THPS is HPAGE_PMD_NR already. So it should inc/dec > > by one rather than nr_pages. > > This is a real bug, thanks for catching it. > > However, your patch changes the user-visible output of /proc/vmstat! > > NR_ANON_THPS isn't just used by memcg, it's a generic accounting item > of the memory subsystem. See this from the Fixes:-patch: > > - __inc_node_page_state(page, NR_ANON_THPS); > + __inc_lruvec_page_state(page, NR_ANON_THPS); > > While we've considered /proc/vmstat less official than other files > like meminfo, and have in the past freely added and removed items, > changing the unit of an existing one is not going to work. Argh, I hit send instead of cancel after noticing that I misread your patch completely. Scratch what I wrote above.
On Sun, Dec 06, 2020 at 06:14:40PM +0800, Muchun Song wrote: > The unit of NR_ANON_THPS is HPAGE_PMD_NR already. So it should inc/dec > by one rather than nr_pages. > > Fixes: 468c398233da ("mm: memcontrol: switch to native NR_ANON_THPS counter") > Signed-off-by: Muchun Song <songmuchun@bytedance.com> Acked-by: Johannes Weiner <hannes@cmpxchg.org> But please change the subject to 'mm: memcontrol: fix NR_ANON_THPS accounting in charge moving'
On Fri, Dec 11, 2020 at 12:06 AM Johannes Weiner <hannes@cmpxchg.org> wrote: > > On Sun, Dec 06, 2020 at 06:14:40PM +0800, Muchun Song wrote: > > The unit of NR_ANON_THPS is HPAGE_PMD_NR already. So it should inc/dec > > by one rather than nr_pages. > > > > Fixes: 468c398233da ("mm: memcontrol: switch to native NR_ANON_THPS counter") > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > Acked-by: Johannes Weiner <hannes@cmpxchg.org> > > But please change the subject to > > 'mm: memcontrol: fix NR_ANON_THPS accounting in charge moving' OK. Will do that. Thanks.
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 22d9bd688d6d..695dedf8687a 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5634,10 +5634,8 @@ static int mem_cgroup_move_account(struct page *page, __mod_lruvec_state(from_vec, NR_ANON_MAPPED, -nr_pages); __mod_lruvec_state(to_vec, NR_ANON_MAPPED, nr_pages); if (PageTransHuge(page)) { - __mod_lruvec_state(from_vec, NR_ANON_THPS, - -nr_pages); - __mod_lruvec_state(to_vec, NR_ANON_THPS, - nr_pages); + __dec_lruvec_state(from_vec, NR_ANON_THPS); + __inc_lruvec_state(to_vec, NR_ANON_THPS); } }
The unit of NR_ANON_THPS is HPAGE_PMD_NR already. So it should inc/dec by one rather than nr_pages. Fixes: 468c398233da ("mm: memcontrol: switch to native NR_ANON_THPS counter") Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- mm/memcontrol.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)