| Message ID | 20220128195642.416743-3-longman@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/page_owner: Extend page_owner to show memcg | expand |
On Fri, Jan 28, 2022 at 02:56:42PM -0500, Waiman Long wrote: > It was found that a number of offlined memcgs were not freed because > they were pinned by some charged pages that were present. Even "echo > 1 > /proc/sys/vm/drop_caches" wasn't able to free those pages. These > offlined but not freed memcgs tend to increase in number over time with > the side effect that percpu memory consumption as shown in /proc/meminfo > also increases over time. > > In order to find out more information about those pages that pin > offlined memcgs, the page_owner feature is extended to dump memory > cgroup information especially whether the cgroup is offlined or not. > > Signed-off-by: Waiman Long <longman@redhat.com> > --- > mm/page_owner.c | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/mm/page_owner.c b/mm/page_owner.c > index c52ce9d6bc3b..e5d8c642296b 100644 > --- a/mm/page_owner.c > +++ b/mm/page_owner.c > @@ -10,6 +10,7 @@ > #include <linux/migrate.h> > #include <linux/stackdepot.h> > #include <linux/seq_file.h> > +#include <linux/memcontrol.h> > #include <linux/sched/clock.h> > > #include "internal.h" > @@ -339,6 +340,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, > depot_stack_handle_t handle) > { > int ret = 0, pageblock_mt, page_mt; > + unsigned long __maybe_unused memcg_data; > char *kbuf; > > count = min_t(size_t, count, PAGE_SIZE); > @@ -371,6 +373,32 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, > "Page has been migrated, last migrate reason: %s\n", > migrate_reason_names[page_owner->last_migrate_reason]); > > +#ifdef CONFIG_MEMCG > + /* > + * Look for memcg information and print it out > + */ > + memcg_data = READ_ONCE(page->memcg_data); > + if (memcg_data) { > + struct mem_cgroup *memcg = page_memcg_check(page); > + bool onlined; > + char name[80]; > + > + if (memcg_data & MEMCG_DATA_OBJCGS) > + SNPRINTF(kbuf, count, ret, err, "Slab cache page\n"); > + > + if (!memcg) > + goto copy_out; > + > + onlined = (memcg->css.flags & CSS_ONLINE); > + cgroup_name(memcg->css.cgroup, name, sizeof(name) - 1); > + SNPRINTF(kbuf, count, ret, err, "Charged %sto %smemcg %s\n", ^^^ Extra specifier? Did this compile without warnings? Ira > + PageMemcgKmem(page) ? "(via objcg) " : "", > + onlined ? "" : "offlined ", name); > + } > + > +copy_out: > +#endif > + > SNPRINTF(kbuf, count, ret, err, "\n"); > > if (copy_to_user(buf, kbuf, ret)) > -- > 2.27.0 > >
On 1/28/22 16:22, Ira Weiny wrote: > On Fri, Jan 28, 2022 at 02:56:42PM -0500, Waiman Long wrote: >> It was found that a number of offlined memcgs were not freed because >> they were pinned by some charged pages that were present. Even "echo >> 1 > /proc/sys/vm/drop_caches" wasn't able to free those pages. These >> offlined but not freed memcgs tend to increase in number over time with >> the side effect that percpu memory consumption as shown in /proc/meminfo >> also increases over time. >> >> In order to find out more information about those pages that pin >> offlined memcgs, the page_owner feature is extended to dump memory >> cgroup information especially whether the cgroup is offlined or not. >> >> Signed-off-by: Waiman Long <longman@redhat.com> >> --- >> mm/page_owner.c | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/mm/page_owner.c b/mm/page_owner.c >> index c52ce9d6bc3b..e5d8c642296b 100644 >> --- a/mm/page_owner.c >> +++ b/mm/page_owner.c >> @@ -10,6 +10,7 @@ >> #include <linux/migrate.h> >> #include <linux/stackdepot.h> >> #include <linux/seq_file.h> >> +#include <linux/memcontrol.h> >> #include <linux/sched/clock.h> >> >> #include "internal.h" >> @@ -339,6 +340,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, >> depot_stack_handle_t handle) >> { >> int ret = 0, pageblock_mt, page_mt; >> + unsigned long __maybe_unused memcg_data; >> char *kbuf; >> >> count = min_t(size_t, count, PAGE_SIZE); >> @@ -371,6 +373,32 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, >> "Page has been migrated, last migrate reason: %s\n", >> migrate_reason_names[page_owner->last_migrate_reason]); >> >> +#ifdef CONFIG_MEMCG >> + /* >> + * Look for memcg information and print it out >> + */ >> + memcg_data = READ_ONCE(page->memcg_data); >> + if (memcg_data) { >> + struct mem_cgroup *memcg = page_memcg_check(page); >> + bool onlined; >> + char name[80]; >> + >> + if (memcg_data & MEMCG_DATA_OBJCGS) >> + SNPRINTF(kbuf, count, ret, err, "Slab cache page\n"); >> + >> + if (!memcg) >> + goto copy_out; >> + >> + onlined = (memcg->css.flags & CSS_ONLINE); >> + cgroup_name(memcg->css.cgroup, name, sizeof(name) - 1); >> + SNPRINTF(kbuf, count, ret, err, "Charged %sto %smemcg %s\n", > ^^^ > Extra specifier? > > Did this compile without warnings? Yes, there was no warning. Cheers, Longmna > Ira > >> + PageMemcgKmem(page) ? "(via objcg) " : "", >> + onlined ? "" : "offlined ", name); >> + } >> + >> +copy_out: >> +#endif >> + >> SNPRINTF(kbuf, count, ret, err, "\n"); >> >> if (copy_to_user(buf, kbuf, ret)) >> -- >> 2.27.0 >> >>
On Fri, Jan 28, 2022 at 04:31:07PM -0500, Waiman Long wrote: > On 1/28/22 16:22, Ira Weiny wrote: > > On Fri, Jan 28, 2022 at 02:56:42PM -0500, Waiman Long wrote: > > > It was found that a number of offlined memcgs were not freed because > > > they were pinned by some charged pages that were present. Even "echo > > > 1 > /proc/sys/vm/drop_caches" wasn't able to free those pages. These > > > offlined but not freed memcgs tend to increase in number over time with > > > the side effect that percpu memory consumption as shown in /proc/meminfo > > > also increases over time. > > > > > > In order to find out more information about those pages that pin > > > offlined memcgs, the page_owner feature is extended to dump memory > > > cgroup information especially whether the cgroup is offlined or not. > > > > > > Signed-off-by: Waiman Long <longman@redhat.com> > > > --- > > > mm/page_owner.c | 28 ++++++++++++++++++++++++++++ > > > 1 file changed, 28 insertions(+) > > > > > > diff --git a/mm/page_owner.c b/mm/page_owner.c > > > index c52ce9d6bc3b..e5d8c642296b 100644 > > > --- a/mm/page_owner.c > > > +++ b/mm/page_owner.c > > > @@ -10,6 +10,7 @@ > > > #include <linux/migrate.h> > > > #include <linux/stackdepot.h> > > > #include <linux/seq_file.h> > > > +#include <linux/memcontrol.h> > > > #include <linux/sched/clock.h> > > > #include "internal.h" > > > @@ -339,6 +340,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, > > > depot_stack_handle_t handle) > > > { > > > int ret = 0, pageblock_mt, page_mt; > > > + unsigned long __maybe_unused memcg_data; > > > char *kbuf; > > > count = min_t(size_t, count, PAGE_SIZE); > > > @@ -371,6 +373,32 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, > > > "Page has been migrated, last migrate reason: %s\n", > > > migrate_reason_names[page_owner->last_migrate_reason]); > > > +#ifdef CONFIG_MEMCG > > > + /* > > > + * Look for memcg information and print it out > > > + */ > > > + memcg_data = READ_ONCE(page->memcg_data); > > > + if (memcg_data) { > > > + struct mem_cgroup *memcg = page_memcg_check(page); > > > + bool onlined; > > > + char name[80]; > > > + > > > + if (memcg_data & MEMCG_DATA_OBJCGS) > > > + SNPRINTF(kbuf, count, ret, err, "Slab cache page\n"); > > > + > > > + if (!memcg) > > > + goto copy_out; > > > + > > > + onlined = (memcg->css.flags & CSS_ONLINE); > > > + cgroup_name(memcg->css.cgroup, name, sizeof(name) - 1); > > > + SNPRINTF(kbuf, count, ret, err, "Charged %sto %smemcg %s\n", > > ^^^ > > Extra specifier? > > > > Did this compile without warnings? > > Yes, there was no warning. But isn't that an extra specifier? Ira > > Cheers, > Longmna > > > Ira > > > > > + PageMemcgKmem(page) ? "(via objcg) " : "", > > > + onlined ? "" : "offlined ", name); > > > + } > > > + > > > +copy_out: > > > +#endif > > > + > > > SNPRINTF(kbuf, count, ret, err, "\n"); > > > if (copy_to_user(buf, kbuf, ret)) > > > -- > > > 2.27.0 > > > > > > >
On 1/28/22 16:48, Ira Weiny wrote: > On Fri, Jan 28, 2022 at 04:31:07PM -0500, Waiman Long wrote: >> On 1/28/22 16:22, Ira Weiny wrote: >>> On Fri, Jan 28, 2022 at 02:56:42PM -0500, Waiman Long wrote: >>>> It was found that a number of offlined memcgs were not freed because >>>> they were pinned by some charged pages that were present. Even "echo >>>> 1 > /proc/sys/vm/drop_caches" wasn't able to free those pages. These >>>> offlined but not freed memcgs tend to increase in number over time with >>>> the side effect that percpu memory consumption as shown in /proc/meminfo >>>> also increases over time. >>>> >>>> In order to find out more information about those pages that pin >>>> offlined memcgs, the page_owner feature is extended to dump memory >>>> cgroup information especially whether the cgroup is offlined or not. >>>> >>>> Signed-off-by: Waiman Long <longman@redhat.com> >>>> --- >>>> mm/page_owner.c | 28 ++++++++++++++++++++++++++++ >>>> 1 file changed, 28 insertions(+) >>>> >>>> diff --git a/mm/page_owner.c b/mm/page_owner.c >>>> index c52ce9d6bc3b..e5d8c642296b 100644 >>>> --- a/mm/page_owner.c >>>> +++ b/mm/page_owner.c >>>> @@ -10,6 +10,7 @@ >>>> #include <linux/migrate.h> >>>> #include <linux/stackdepot.h> >>>> #include <linux/seq_file.h> >>>> +#include <linux/memcontrol.h> >>>> #include <linux/sched/clock.h> >>>> #include "internal.h" >>>> @@ -339,6 +340,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, >>>> depot_stack_handle_t handle) >>>> { >>>> int ret = 0, pageblock_mt, page_mt; >>>> + unsigned long __maybe_unused memcg_data; >>>> char *kbuf; >>>> count = min_t(size_t, count, PAGE_SIZE); >>>> @@ -371,6 +373,32 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, >>>> "Page has been migrated, last migrate reason: %s\n", >>>> migrate_reason_names[page_owner->last_migrate_reason]); >>>> +#ifdef CONFIG_MEMCG >>>> + /* >>>> + * Look for memcg information and print it out >>>> + */ >>>> + memcg_data = READ_ONCE(page->memcg_data); >>>> + if (memcg_data) { >>>> + struct mem_cgroup *memcg = page_memcg_check(page); >>>> + bool onlined; >>>> + char name[80]; >>>> + >>>> + if (memcg_data & MEMCG_DATA_OBJCGS) >>>> + SNPRINTF(kbuf, count, ret, err, "Slab cache page\n"); >>>> + >>>> + if (!memcg) >>>> + goto copy_out; >>>> + >>>> + onlined = (memcg->css.flags & CSS_ONLINE); >>>> + cgroup_name(memcg->css.cgroup, name, sizeof(name) - 1); >>>> + SNPRINTF(kbuf, count, ret, err, "Charged %sto %smemcg %s\n", >>> ^^^ >>> Extra specifier? >>> >>> Did this compile without warnings? >> Yes, there was no warning. > But isn't that an extra specifier? There are 3 arguments to the format string that match the 3 "%s" in it: 1) PageMemcgKmem(page) ? "(via objcg) " : "" 2) onlined ? "" : "offlined 3) name Cheers, Longman
On Fri, Jan 28, 2022 at 10:35:22PM -0500, Waiman Long wrote: > On 1/28/22 16:48, Ira Weiny wrote: > > On Fri, Jan 28, 2022 at 04:31:07PM -0500, Waiman Long wrote: > > > On 1/28/22 16:22, Ira Weiny wrote: > > > > On Fri, Jan 28, 2022 at 02:56:42PM -0500, Waiman Long wrote: > > > > > It was found that a number of offlined memcgs were not freed because > > > > > they were pinned by some charged pages that were present. Even "echo > > > > > 1 > /proc/sys/vm/drop_caches" wasn't able to free those pages. These > > > > > offlined but not freed memcgs tend to increase in number over time with > > > > > the side effect that percpu memory consumption as shown in /proc/meminfo > > > > > also increases over time. > > > > > > > > > > In order to find out more information about those pages that pin > > > > > offlined memcgs, the page_owner feature is extended to dump memory > > > > > cgroup information especially whether the cgroup is offlined or not. > > > > > > > > > > Signed-off-by: Waiman Long <longman@redhat.com> > > > > > --- > > > > > mm/page_owner.c | 28 ++++++++++++++++++++++++++++ > > > > > 1 file changed, 28 insertions(+) > > > > > > > > > > diff --git a/mm/page_owner.c b/mm/page_owner.c > > > > > index c52ce9d6bc3b..e5d8c642296b 100644 > > > > > --- a/mm/page_owner.c > > > > > +++ b/mm/page_owner.c > > > > > @@ -10,6 +10,7 @@ > > > > > #include <linux/migrate.h> > > > > > #include <linux/stackdepot.h> > > > > > #include <linux/seq_file.h> > > > > > +#include <linux/memcontrol.h> > > > > > #include <linux/sched/clock.h> > > > > > #include "internal.h" > > > > > @@ -339,6 +340,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, > > > > > depot_stack_handle_t handle) > > > > > { > > > > > int ret = 0, pageblock_mt, page_mt; > > > > > + unsigned long __maybe_unused memcg_data; > > > > > char *kbuf; > > > > > count = min_t(size_t, count, PAGE_SIZE); > > > > > @@ -371,6 +373,32 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, > > > > > "Page has been migrated, last migrate reason: %s\n", > > > > > migrate_reason_names[page_owner->last_migrate_reason]); > > > > > +#ifdef CONFIG_MEMCG > > > > > + /* > > > > > + * Look for memcg information and print it out > > > > > + */ > > > > > + memcg_data = READ_ONCE(page->memcg_data); > > > > > + if (memcg_data) { > > > > > + struct mem_cgroup *memcg = page_memcg_check(page); > > > > > + bool onlined; > > > > > + char name[80]; > > > > > + > > > > > + if (memcg_data & MEMCG_DATA_OBJCGS) > > > > > + SNPRINTF(kbuf, count, ret, err, "Slab cache page\n"); > > > > > + > > > > > + if (!memcg) > > > > > + goto copy_out; > > > > > + > > > > > + onlined = (memcg->css.flags & CSS_ONLINE); > > > > > + cgroup_name(memcg->css.cgroup, name, sizeof(name) - 1); > > > > > + SNPRINTF(kbuf, count, ret, err, "Charged %sto %smemcg %s\n", > > > > ^^^ > > > > Extra specifier? > > > > > > > > Did this compile without warnings? > > > Yes, there was no warning. > > But isn't that an extra specifier? > > There are 3 arguments to the format string that match the 3 "%s" in it: > > 1) PageMemcgKmem(page) ? "(via objcg) " : "" > 2) onlined ? "" : "offlined > 3) name My apologies. My parsing of the ? statements was off. FWIW putting ', name' on the next line would make it more clear... But I see now... Sorry, Ira > > Cheers, > Longman >
diff --git a/mm/page_owner.c b/mm/page_owner.c index c52ce9d6bc3b..e5d8c642296b 100644 --- a/mm/page_owner.c +++ b/mm/page_owner.c @@ -10,6 +10,7 @@ #include <linux/migrate.h> #include <linux/stackdepot.h> #include <linux/seq_file.h> +#include <linux/memcontrol.h> #include <linux/sched/clock.h> #include "internal.h" @@ -339,6 +340,7 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, depot_stack_handle_t handle) { int ret = 0, pageblock_mt, page_mt; + unsigned long __maybe_unused memcg_data; char *kbuf; count = min_t(size_t, count, PAGE_SIZE); @@ -371,6 +373,32 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, "Page has been migrated, last migrate reason: %s\n", migrate_reason_names[page_owner->last_migrate_reason]); +#ifdef CONFIG_MEMCG + /* + * Look for memcg information and print it out + */ + memcg_data = READ_ONCE(page->memcg_data); + if (memcg_data) { + struct mem_cgroup *memcg = page_memcg_check(page); + bool onlined; + char name[80]; + + if (memcg_data & MEMCG_DATA_OBJCGS) + SNPRINTF(kbuf, count, ret, err, "Slab cache page\n"); + + if (!memcg) + goto copy_out; + + onlined = (memcg->css.flags & CSS_ONLINE); + cgroup_name(memcg->css.cgroup, name, sizeof(name) - 1); + SNPRINTF(kbuf, count, ret, err, "Charged %sto %smemcg %s\n", + PageMemcgKmem(page) ? "(via objcg) " : "", + onlined ? "" : "offlined ", name); + } + +copy_out: +#endif + SNPRINTF(kbuf, count, ret, err, "\n"); if (copy_to_user(buf, kbuf, ret))
It was found that a number of offlined memcgs were not freed because they were pinned by some charged pages that were present. Even "echo 1 > /proc/sys/vm/drop_caches" wasn't able to free those pages. These offlined but not freed memcgs tend to increase in number over time with the side effect that percpu memory consumption as shown in /proc/meminfo also increases over time. In order to find out more information about those pages that pin offlined memcgs, the page_owner feature is extended to dump memory cgroup information especially whether the cgroup is offlined or not. Signed-off-by: Waiman Long <longman@redhat.com> --- mm/page_owner.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)