[RFC] memcg usage_in_bytes does not account file mapped and slab memory

[RFC] memcg usage_in_bytes does not account file mapped and slab memory

[Anton Vorontsov]

... and thus is useless for low memory notifications.

Hi all!

While working on userspace low memory killer daemon (a supposed
substitution for the kernel low memory killer, i.e.
drivers/staging/android/lowmemorykiller.c), I noticed that current
cgroups memory notifications aren't suitable for such a daemon.

Suppose we want to install a notification when free memory drops below
8 MB. Logically (taking memory hotplug aside), using current usage_in_bytes
notifications we would install an event on 'total_ram - 8MB' threshold.

But as usage_in_bytes doesn't account file mapped memory and memory
used by kernel slab, the formula won't work.

Currently I use the following patch that makes things going:

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 228d646..c8abdc5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3812,6 +3812,9 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
 
        val = mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_CACHE);
        val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_RSS);
+       val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
+       val += global_page_state(NR_SLAB_RECLAIMABLE);
+       val += global_page_state(NR_SLAB_UNRECLAIMABLE);


But here are some questions:

1. Is there any particular reason we don't currently account file mapped
   memory in usage_in_bytes?

   To me, MEM_CGROUP_STAT_FILE_MAPPED hunk seems logical even if we
   don't use it for lowmemory notifications.

   Plus, it seems that FILE_MAPPED _is_ accounted for the non-root
   cgroups, so I guess it's clearly a bug for the root memcg?

2. As for NR_SLAB_RECLAIMABLE and NR_SLAB_UNRECLAIMABLE, it seems that
   these numbers are only applicable for the root memcg.
   I'm not sure that usage_in_bytes semantics should actually account
   these, but I tend to think that we should.

All in all, not accounting both 1. and 2. looks like bugs to me.

But if for some reason we don't want to change usage_in_bytes, should
I just go ahead and implement a new cftype (say free_in_bytes), which
would account free memory as total_ram - cache - rss - mapped - slab,
with ability to install notifiers? That way we would also could solve
memory hotplug issue in the kernel, so that userland won't need to
bother with reinstalling lowmemory notifiers when memory added/removed.

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

[RFC] memcg usage_in_bytes does not account file mapped and slab memory

[Anton Vorontsov]

... and thus is useless for low memory notifications.

Hi all!

While working on userspace low memory killer daemon (a supposed
substitution for the kernel low memory killer, i.e.
drivers/staging/android/lowmemorykiller.c), I noticed that current
cgroups memory notifications aren't suitable for such a daemon.

Suppose we want to install a notification when free memory drops below
8 MB. Logically (taking memory hotplug aside), using current usage_in_bytes
notifications we would install an event on 'total_ram - 8MB' threshold.

But as usage_in_bytes doesn't account file mapped memory and memory
used by kernel slab, the formula won't work.

Currently I use the following patch that makes things going:

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 228d646..c8abdc5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3812,6 +3812,9 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
 
        val = mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_CACHE);
        val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_RSS);
+       val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
+       val += global_page_state(NR_SLAB_RECLAIMABLE);
+       val += global_page_state(NR_SLAB_UNRECLAIMABLE);


But here are some questions:

1. Is there any particular reason we don't currently account file mapped
   memory in usage_in_bytes?

   To me, MEM_CGROUP_STAT_FILE_MAPPED hunk seems logical even if we
   don't use it for lowmemory notifications.

   Plus, it seems that FILE_MAPPED _is_ accounted for the non-root
   cgroups, so I guess it's clearly a bug for the root memcg?

2. As for NR_SLAB_RECLAIMABLE and NR_SLAB_UNRECLAIMABLE, it seems that
   these numbers are only applicable for the root memcg.
   I'm not sure that usage_in_bytes semantics should actually account
   these, but I tend to think that we should.

All in all, not accounting both 1. and 2. looks like bugs to me.

But if for some reason we don't want to change usage_in_bytes, should
I just go ahead and implement a new cftype (say free_in_bytes), which
would account free memory as total_ram - cache - rss - mapped - slab,
with ability to install notifiers? That way we would also could solve
memory hotplug issue in the kernel, so that userland won't need to
bother with reinstalling lowmemory notifiers when memory added/removed.

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[RFC] memcg usage_in_bytes does not account file mapped and slab memory

[Anton Vorontsov]

... and thus is useless for low memory notifications.

Hi all!

While working on userspace low memory killer daemon (a supposed
substitution for the kernel low memory killer, i.e.
drivers/staging/android/lowmemorykiller.c), I noticed that current
cgroups memory notifications aren't suitable for such a daemon.

Suppose we want to install a notification when free memory drops below
8 MB. Logically (taking memory hotplug aside), using current usage_in_bytes
notifications we would install an event on 'total_ram - 8MB' threshold.

But as usage_in_bytes doesn't account file mapped memory and memory
used by kernel slab, the formula won't work.

Currently I use the following patch that makes things going:

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 228d646..c8abdc5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3812,6 +3812,9 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
 
        val = mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_CACHE);
        val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_RSS);
+       val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
+       val += global_page_state(NR_SLAB_RECLAIMABLE);
+       val += global_page_state(NR_SLAB_UNRECLAIMABLE);


But here are some questions:

1. Is there any particular reason we don't currently account file mapped
   memory in usage_in_bytes?

   To me, MEM_CGROUP_STAT_FILE_MAPPED hunk seems logical even if we
   don't use it for lowmemory notifications.

   Plus, it seems that FILE_MAPPED _is_ accounted for the non-root
   cgroups, so I guess it's clearly a bug for the root memcg?

2. As for NR_SLAB_RECLAIMABLE and NR_SLAB_UNRECLAIMABLE, it seems that
   these numbers are only applicable for the root memcg.
   I'm not sure that usage_in_bytes semantics should actually account
   these, but I tend to think that we should.

All in all, not accounting both 1. and 2. looks like bugs to me.

But if for some reason we don't want to change usage_in_bytes, should
I just go ahead and implement a new cftype (say free_in_bytes), which
would account free memory as total_ram - cache - rss - mapped - slab,
with ability to install notifiers? That way we would also could solve
memory hotplug issue in the kernel, so that userland won't need to
bother with reinstalling lowmemory notifiers when memory added/removed.

Thanks!

-- 
Anton Vorontsov
Email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Re: [RFC] memcg usage_in_bytes does not account file mapped and slab memory

[KAMEZAWA Hiroyuki]

On Fri, 2 Mar 2012 20:27:53 +0400
Anton Vorontsov <anton.vorontsov@linaro.org> wrote:

> ... and thus is useless for low memory notifications.
> 
> Hi all!
> 
> While working on userspace low memory killer daemon (a supposed
> substitution for the kernel low memory killer, i.e.
> drivers/staging/android/lowmemorykiller.c), I noticed that current
> cgroups memory notifications aren't suitable for such a daemon.
> 
> Suppose we want to install a notification when free memory drops below
> 8 MB. Logically (taking memory hotplug aside), using current usage_in_bytes
> notifications we would install an event on 'total_ram - 8MB' threshold.
> 
> But as usage_in_bytes doesn't account file mapped memory and memory
> used by kernel slab, the formula won't work.
> 
> Currently I use the following patch that makes things going:
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 228d646..c8abdc5 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3812,6 +3812,9 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
>  
>         val = mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_CACHE);
>         val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_RSS);
> +       val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
> +       val += global_page_state(NR_SLAB_RECLAIMABLE);
> +       val += global_page_state(NR_SLAB_UNRECLAIMABLE);
> 
> 
> But here are some questions:
> 
> 1. Is there any particular reason we don't currently account file mapped
>    memory in usage_in_bytes?
> 

CACHE includes all file caches. Why do you think FILE_MAPPED is not included in CACHE ?


>    To me, MEM_CGROUP_STAT_FILE_MAPPED hunk seems logical even if we
>    don't use it for lowmemory notifications.
> 
>    Plus, it seems that FILE_MAPPED _is_ accounted for the non-root
>    cgroups, so I guess it's clearly a bug for the root memcg?
> 
> 2. As for NR_SLAB_RECLAIMABLE and NR_SLAB_UNRECLAIMABLE, it seems that
>    these numbers are only applicable for the root memcg.
>    I'm not sure that usage_in_bytes semantics should actually account
>    these, but I tend to think that we should.
> 

Now, SLAB is not accounted by memcg at all.
See memifo if necessary.

> All in all, not accounting both 1. and 2. looks like bugs to me.
> 

It's spec. not bug. If you want to see slab status in memcg's file,
Please add kernel memory accounting feature. There has been already 2 proposals.
Check them and comment.

Thanks,
-Kame

Re: [RFC] memcg usage_in_bytes does not account file mapped and slab memory

[KAMEZAWA Hiroyuki]

On Fri, 2 Mar 2012 20:27:53 +0400
Anton Vorontsov <anton.vorontsov@linaro.org> wrote:

> ... and thus is useless for low memory notifications.
> 
> Hi all!
> 
> While working on userspace low memory killer daemon (a supposed
> substitution for the kernel low memory killer, i.e.
> drivers/staging/android/lowmemorykiller.c), I noticed that current
> cgroups memory notifications aren't suitable for such a daemon.
> 
> Suppose we want to install a notification when free memory drops below
> 8 MB. Logically (taking memory hotplug aside), using current usage_in_bytes
> notifications we would install an event on 'total_ram - 8MB' threshold.
> 
> But as usage_in_bytes doesn't account file mapped memory and memory
> used by kernel slab, the formula won't work.
> 
> Currently I use the following patch that makes things going:
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 228d646..c8abdc5 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3812,6 +3812,9 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
>  
>         val = mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_CACHE);
>         val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_RSS);
> +       val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
> +       val += global_page_state(NR_SLAB_RECLAIMABLE);
> +       val += global_page_state(NR_SLAB_UNRECLAIMABLE);
> 
> 
> But here are some questions:
> 
> 1. Is there any particular reason we don't currently account file mapped
>    memory in usage_in_bytes?
> 

CACHE includes all file caches. Why do you think FILE_MAPPED is not included in CACHE ?


>    To me, MEM_CGROUP_STAT_FILE_MAPPED hunk seems logical even if we
>    don't use it for lowmemory notifications.
> 
>    Plus, it seems that FILE_MAPPED _is_ accounted for the non-root
>    cgroups, so I guess it's clearly a bug for the root memcg?
> 
> 2. As for NR_SLAB_RECLAIMABLE and NR_SLAB_UNRECLAIMABLE, it seems that
>    these numbers are only applicable for the root memcg.
>    I'm not sure that usage_in_bytes semantics should actually account
>    these, but I tend to think that we should.
> 

Now, SLAB is not accounted by memcg at all.
See memifo if necessary.

> All in all, not accounting both 1. and 2. looks like bugs to me.
> 

It's spec. not bug. If you want to see slab status in memcg's file,
Please add kernel memory accounting feature. There has been already 2 proposals.
Check them and comment.

Thanks,
-Kame

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH RFC] memcg: MEMCG_NR_FILE_MAPPED should update _STAT_CACHE as well

[Anton Vorontsov]

...otherwise the we're getting the wrong numbers in usage_in_bytes.

On Mon, Mar 05, 2012 at 09:19:34AM +0900, KAMEZAWA Hiroyuki wrote:
[...]
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 228d646..c8abdc5 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -3812,6 +3812,9 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
> >  
> >         val = mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_CACHE);
> >         val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_RSS);
> > +       val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
> > 
> > 1. Is there any particular reason we don't currently account file mapped
> >    memory in usage_in_bytes?
> > 
> >    To me, MEM_CGROUP_STAT_FILE_MAPPED hunk seems logical even if we
> >    don't use it for lowmemory notifications.
> > 
> >    Plus, it seems that FILE_MAPPED _is_ accounted for the non-root
> >    cgroups, so I guess it's clearly a bug for the root memcg?
> 
> CACHE includes all file caches. Why do you think FILE_MAPPED is not included in CACHE ?

There were tons of changes in the memcg lately, but I believe the issue
is still there.

For example, looking into this code flow:

-> page_add_file_rmap() (mm/rmap.c)
 -> mem_cgroup_inc_page_stat(page, MEMCG_NR_FILE_MAPPED) (include/linux/memcontrol.h)
  -> void mem_cgroup_update_page_stat(page, MEMCG_NR_FILE_MAPPED, 1) (mm/memcontrol.c)

And then:

void mem_cgroup_update_page_stat(struct page *page,
                                 enum mem_cgroup_page_stat_item idx, int val)
{
        ...
        switch (idx) {
        case MEMCG_NR_FILE_MAPPED:
                idx = MEM_CGROUP_STAT_FILE_MAPPED;
                break;
        default:
                BUG();
        }

        this_cpu_add(memcg->stat->count[idx], val);
        ...
}

So, clearly, this function only bothers updating _FILE_MAPPED only,
leaving _CACHE alone.

If you're saying that _CACHE meant to include _FILE_MAPPED, then
I guess the patch down below would be a proper fix then... Otherwise
we need to be consistent on stats reporting, and either fall-back
to my original fix (in mem_cgroup_usage()), or think about doing it
some other way...

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---

The patch is against current -next.

Thanks,

 mm/memcontrol.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 884e936..760ecf5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1958,6 +1958,8 @@ void mem_cgroup_update_page_stat(struct page *page,
 
 	switch (idx) {
 	case MEMCG_NR_FILE_MAPPED:
+		idx = MEM_CGROUP_STAT_CACHE;
+		this_cpu_add(memcg->stat->count[idx], val);
 		idx = MEM_CGROUP_STAT_FILE_MAPPED;
 		break;
 	default:
-- 
1.7.9.2

[PATCH RFC] memcg: MEMCG_NR_FILE_MAPPED should update _STAT_CACHE as well

[Anton Vorontsov]

...otherwise the we're getting the wrong numbers in usage_in_bytes.

On Mon, Mar 05, 2012 at 09:19:34AM +0900, KAMEZAWA Hiroyuki wrote:
[...]
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 228d646..c8abdc5 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -3812,6 +3812,9 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
> >  
> >         val = mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_CACHE);
> >         val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_RSS);
> > +       val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
> > 
> > 1. Is there any particular reason we don't currently account file mapped
> >    memory in usage_in_bytes?
> > 
> >    To me, MEM_CGROUP_STAT_FILE_MAPPED hunk seems logical even if we
> >    don't use it for lowmemory notifications.
> > 
> >    Plus, it seems that FILE_MAPPED _is_ accounted for the non-root
> >    cgroups, so I guess it's clearly a bug for the root memcg?
> 
> CACHE includes all file caches. Why do you think FILE_MAPPED is not included in CACHE ?

There were tons of changes in the memcg lately, but I believe the issue
is still there.

For example, looking into this code flow:

-> page_add_file_rmap() (mm/rmap.c)
 -> mem_cgroup_inc_page_stat(page, MEMCG_NR_FILE_MAPPED) (include/linux/memcontrol.h)
  -> void mem_cgroup_update_page_stat(page, MEMCG_NR_FILE_MAPPED, 1) (mm/memcontrol.c)

And then:

void mem_cgroup_update_page_stat(struct page *page,
                                 enum mem_cgroup_page_stat_item idx, int val)
{
        ...
        switch (idx) {
        case MEMCG_NR_FILE_MAPPED:
                idx = MEM_CGROUP_STAT_FILE_MAPPED;
                break;
        default:
                BUG();
        }

        this_cpu_add(memcg->stat->count[idx], val);
        ...
}

So, clearly, this function only bothers updating _FILE_MAPPED only,
leaving _CACHE alone.

If you're saying that _CACHE meant to include _FILE_MAPPED, then
I guess the patch down below would be a proper fix then... Otherwise
we need to be consistent on stats reporting, and either fall-back
to my original fix (in mem_cgroup_usage()), or think about doing it
some other way...

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---

The patch is against current -next.

Thanks,

 mm/memcontrol.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 884e936..760ecf5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1958,6 +1958,8 @@ void mem_cgroup_update_page_stat(struct page *page,
 
 	switch (idx) {
 	case MEMCG_NR_FILE_MAPPED:
+		idx = MEM_CGROUP_STAT_CACHE;
+		this_cpu_add(memcg->stat->count[idx], val);
 		idx = MEM_CGROUP_STAT_FILE_MAPPED;
 		break;
 	default:
-- 
1.7.9.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH RFC] memcg: MEMCG_NR_FILE_MAPPED should update _STAT_CACHE as well

[Anton Vorontsov]

...otherwise the we're getting the wrong numbers in usage_in_bytes.

On Mon, Mar 05, 2012 at 09:19:34AM +0900, KAMEZAWA Hiroyuki wrote:
[...]
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 228d646..c8abdc5 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -3812,6 +3812,9 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
> >  
> >         val = mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_CACHE);
> >         val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_RSS);
> > +       val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
> > 
> > 1. Is there any particular reason we don't currently account file mapped
> >    memory in usage_in_bytes?
> > 
> >    To me, MEM_CGROUP_STAT_FILE_MAPPED hunk seems logical even if we
> >    don't use it for lowmemory notifications.
> > 
> >    Plus, it seems that FILE_MAPPED _is_ accounted for the non-root
> >    cgroups, so I guess it's clearly a bug for the root memcg?
> 
> CACHE includes all file caches. Why do you think FILE_MAPPED is not included in CACHE ?

There were tons of changes in the memcg lately, but I believe the issue
is still there.

For example, looking into this code flow:

-> page_add_file_rmap() (mm/rmap.c)
 -> mem_cgroup_inc_page_stat(page, MEMCG_NR_FILE_MAPPED) (include/linux/memcontrol.h)
  -> void mem_cgroup_update_page_stat(page, MEMCG_NR_FILE_MAPPED, 1) (mm/memcontrol.c)

And then:

void mem_cgroup_update_page_stat(struct page *page,
                                 enum mem_cgroup_page_stat_item idx, int val)
{
        ...
        switch (idx) {
        case MEMCG_NR_FILE_MAPPED:
                idx = MEM_CGROUP_STAT_FILE_MAPPED;
                break;
        default:
                BUG();
        }

        this_cpu_add(memcg->stat->count[idx], val);
        ...
}

So, clearly, this function only bothers updating _FILE_MAPPED only,
leaving _CACHE alone.

If you're saying that _CACHE meant to include _FILE_MAPPED, then
I guess the patch down below would be a proper fix then... Otherwise
we need to be consistent on stats reporting, and either fall-back
to my original fix (in mem_cgroup_usage()), or think about doing it
some other way...

Signed-off-by: Anton Vorontsov <anton.vorontsov-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---

The patch is against current -next.

Thanks,

 mm/memcontrol.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 884e936..760ecf5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1958,6 +1958,8 @@ void mem_cgroup_update_page_stat(struct page *page,
 
 	switch (idx) {
 	case MEMCG_NR_FILE_MAPPED:
+		idx = MEM_CGROUP_STAT_CACHE;
+		this_cpu_add(memcg->stat->count[idx], val);
 		idx = MEM_CGROUP_STAT_FILE_MAPPED;
 		break;
 	default:
-- 
1.7.9.2

Re: [PATCH RFC] memcg: MEMCG_NR_FILE_MAPPED should update _STAT_CACHE as well

[KAMEZAWA Hiroyuki]

(2012/04/23 17:28), Anton Vorontsov wrote:

> ...otherwise the we're getting the wrong numbers in usage_in_bytes.
> 
> On Mon, Mar 05, 2012 at 09:19:34AM +0900, KAMEZAWA Hiroyuki wrote:
> [...]
>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>> index 228d646..c8abdc5 100644
>>> --- a/mm/memcontrol.c
>>> +++ b/mm/memcontrol.c
>>> @@ -3812,6 +3812,9 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
>>>  
>>>         val = mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_CACHE);
>>>         val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_RSS);
>>> +       val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
>>>
>>> 1. Is there any particular reason we don't currently account file mapped
>>>    memory in usage_in_bytes?
>>>
>>>    To me, MEM_CGROUP_STAT_FILE_MAPPED hunk seems logical even if we
>>>    don't use it for lowmemory notifications.
>>>
>>>    Plus, it seems that FILE_MAPPED _is_ accounted for the non-root
>>>    cgroups, so I guess it's clearly a bug for the root memcg?
>>
>> CACHE includes all file caches. Why do you think FILE_MAPPED is not included in CACHE ?
> 
> There were tons of changes in the memcg lately, but I believe the issue
> is still there.
> 
> For example, looking into this code flow:
> 
> -> page_add_file_rmap() (mm/rmap.c)
>  -> mem_cgroup_inc_page_stat(page, MEMCG_NR_FILE_MAPPED) (include/linux/memcontrol.h)
>   -> void mem_cgroup_update_page_stat(page, MEMCG_NR_FILE_MAPPED, 1) (mm/memcontrol.c)
> 
> And then:
> 
> void mem_cgroup_update_page_stat(struct page *page,
>                                  enum mem_cgroup_page_stat_item idx, int val)
> {
>         ...
>         switch (idx) {
>         case MEMCG_NR_FILE_MAPPED:
>                 idx = MEM_CGROUP_STAT_FILE_MAPPED;
>                 break;
>         default:
>                 BUG();
>         }
> 
>         this_cpu_add(memcg->stat->count[idx], val);
>         ...
> }
> 
> So, clearly, this function only bothers updating _FILE_MAPPED only,
> leaving _CACHE alone.
> 
> If you're saying that _CACHE meant to include _FILE_MAPPED, then
> I guess the patch down below would be a proper fix then... Otherwise
> we need to be consistent on stats reporting, and either fall-back
> to my original fix (in mem_cgroup_usage()), or think about doing it
> some other way...
> 


NACK.
CACHE is updated at charge()/uncharge()...inserting/removing page cache to radix-tree.

Thanks,
-Kame


> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
> ---
> 
> The patch is against current -next.
> 
> Thanks,
> 
>  mm/memcontrol.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 884e936..760ecf5 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1958,6 +1958,8 @@ void mem_cgroup_update_page_stat(struct page *page,
>  
>  	switch (idx) {
>  	case MEMCG_NR_FILE_MAPPED:
> +		idx = MEM_CGROUP_STAT_CACHE;
> +		this_cpu_add(memcg->stat->count[idx], val);
>  		idx = MEM_CGROUP_STAT_FILE_MAPPED;
>  		break;
>  	default:

Re: [PATCH RFC] memcg: MEMCG_NR_FILE_MAPPED should update _STAT_CACHE as well

[KAMEZAWA Hiroyuki]

(2012/04/23 17:28), Anton Vorontsov wrote:

> ...otherwise the we're getting the wrong numbers in usage_in_bytes.
> 
> On Mon, Mar 05, 2012 at 09:19:34AM +0900, KAMEZAWA Hiroyuki wrote:
> [...]
>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>> index 228d646..c8abdc5 100644
>>> --- a/mm/memcontrol.c
>>> +++ b/mm/memcontrol.c
>>> @@ -3812,6 +3812,9 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
>>>  
>>>         val = mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_CACHE);
>>>         val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_RSS);
>>> +       val += mem_cgroup_recursive_stat(memcg, MEM_CGROUP_STAT_FILE_MAPPED);
>>>
>>> 1. Is there any particular reason we don't currently account file mapped
>>>    memory in usage_in_bytes?
>>>
>>>    To me, MEM_CGROUP_STAT_FILE_MAPPED hunk seems logical even if we
>>>    don't use it for lowmemory notifications.
>>>
>>>    Plus, it seems that FILE_MAPPED _is_ accounted for the non-root
>>>    cgroups, so I guess it's clearly a bug for the root memcg?
>>
>> CACHE includes all file caches. Why do you think FILE_MAPPED is not included in CACHE ?
> 
> There were tons of changes in the memcg lately, but I believe the issue
> is still there.
> 
> For example, looking into this code flow:
> 
> -> page_add_file_rmap() (mm/rmap.c)
>  -> mem_cgroup_inc_page_stat(page, MEMCG_NR_FILE_MAPPED) (include/linux/memcontrol.h)
>   -> void mem_cgroup_update_page_stat(page, MEMCG_NR_FILE_MAPPED, 1) (mm/memcontrol.c)
> 
> And then:
> 
> void mem_cgroup_update_page_stat(struct page *page,
>                                  enum mem_cgroup_page_stat_item idx, int val)
> {
>         ...
>         switch (idx) {
>         case MEMCG_NR_FILE_MAPPED:
>                 idx = MEM_CGROUP_STAT_FILE_MAPPED;
>                 break;
>         default:
>                 BUG();
>         }
> 
>         this_cpu_add(memcg->stat->count[idx], val);
>         ...
> }
> 
> So, clearly, this function only bothers updating _FILE_MAPPED only,
> leaving _CACHE alone.
> 
> If you're saying that _CACHE meant to include _FILE_MAPPED, then
> I guess the patch down below would be a proper fix then... Otherwise
> we need to be consistent on stats reporting, and either fall-back
> to my original fix (in mem_cgroup_usage()), or think about doing it
> some other way...
> 


NACK.
CACHE is updated at charge()/uncharge()...inserting/removing page cache to radix-tree.

Thanks,
-Kame


> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
> ---
> 
> The patch is against current -next.
> 
> Thanks,
> 
>  mm/memcontrol.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 884e936..760ecf5 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -1958,6 +1958,8 @@ void mem_cgroup_update_page_stat(struct page *page,
>  
>  	switch (idx) {
>  	case MEMCG_NR_FILE_MAPPED:
> +		idx = MEM_CGROUP_STAT_CACHE;
> +		this_cpu_add(memcg->stat->count[idx], val);
>  		idx = MEM_CGROUP_STAT_FILE_MAPPED;
>  		break;
>  	default:



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH RFC] memcg: MEMCG_NR_FILE_MAPPED should update _STAT_CACHE as well

[Anton Vorontsov]

On Mon, Apr 23, 2012 at 05:35:12PM +0900, KAMEZAWA Hiroyuki wrote:
[...]
> > For example, looking into this code flow:
> > 
> > -> page_add_file_rmap() (mm/rmap.c)
> >  -> mem_cgroup_inc_page_stat(page, MEMCG_NR_FILE_MAPPED) (include/linux/memcontrol.h)
> >   -> void mem_cgroup_update_page_stat(page, MEMCG_NR_FILE_MAPPED, 1) (mm/memcontrol.c)
> > 
> > And then:
> > 
> > void mem_cgroup_update_page_stat(struct page *page,
> >                                  enum mem_cgroup_page_stat_item idx, int val)
> > {
> >         ...
> >         switch (idx) {
> >         case MEMCG_NR_FILE_MAPPED:
> >                 idx = MEM_CGROUP_STAT_FILE_MAPPED;
> >                 break;
> >         default:
> >                 BUG();
> >         }
> > 
> >         this_cpu_add(memcg->stat->count[idx], val);
> >         ...
> > }
> > 
> > So, clearly, this function only bothers updating _FILE_MAPPED only,
> > leaving _CACHE alone.
[...]
> 
> NACK.
> CACHE is updated at charge()/uncharge()...inserting/removing page cache to radix-tree.

Interesting; true, we have charge/uncharge in __do_fault()/do_wp_page
and friends. So, we seem to update FILE_MAPPED in the rmap via
cgroup_dec/inc_page_stat, and CACHE is updated via charge/uncharge. Hm.

The code in memory.c is full of if/else ifs, and I wonder if there's 
some discrepancy in there, but briefly looking it looks fine. The
code looks correct indeed, but I'm getting the wrong stats. :-/

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH RFC] memcg: MEMCG_NR_FILE_MAPPED should update _STAT_CACHE as well

[Anton Vorontsov]

On Mon, Apr 23, 2012 at 05:35:12PM +0900, KAMEZAWA Hiroyuki wrote:
[...]
> > For example, looking into this code flow:
> > 
> > -> page_add_file_rmap() (mm/rmap.c)
> >  -> mem_cgroup_inc_page_stat(page, MEMCG_NR_FILE_MAPPED) (include/linux/memcontrol.h)
> >   -> void mem_cgroup_update_page_stat(page, MEMCG_NR_FILE_MAPPED, 1) (mm/memcontrol.c)
> > 
> > And then:
> > 
> > void mem_cgroup_update_page_stat(struct page *page,
> >                                  enum mem_cgroup_page_stat_item idx, int val)
> > {
> >         ...
> >         switch (idx) {
> >         case MEMCG_NR_FILE_MAPPED:
> >                 idx = MEM_CGROUP_STAT_FILE_MAPPED;
> >                 break;
> >         default:
> >                 BUG();
> >         }
> > 
> >         this_cpu_add(memcg->stat->count[idx], val);
> >         ...
> > }
> > 
> > So, clearly, this function only bothers updating _FILE_MAPPED only,
> > leaving _CACHE alone.
[...]
> 
> NACK.
> CACHE is updated at charge()/uncharge()...inserting/removing page cache to radix-tree.

Interesting; true, we have charge/uncharge in __do_fault()/do_wp_page
and friends. So, we seem to update FILE_MAPPED in the rmap via
cgroup_dec/inc_page_stat, and CACHE is updated via charge/uncharge. Hm.

The code in memory.c is full of if/else ifs, and I wonder if there's 
some discrepancy in there, but briefly looking it looks fine. The
code looks correct indeed, but I'm getting the wrong stats. :-/

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>