[Devel] Re: [PATCH 07/10] memcg: add dirty limits to mem_cgroup
Greg Thelen
gthelen at google.com
Mon Oct 11 17:24:21 PDT 2010
KAMEZAWA Hiroyuki <kamezawa.hiroyu at jp.fujitsu.com> writes:
> On Wed, 06 Oct 2010 17:27:13 -0700
> Greg Thelen <gthelen at google.com> wrote:
>
>> KAMEZAWA Hiroyuki <kamezawa.hiroyu at jp.fujitsu.com> writes:
>>
>> > On Tue, 05 Oct 2010 12:00:17 -0700
>> > Greg Thelen <gthelen at google.com> wrote:
>> >
>> >> Andrea Righi <arighi at develer.com> writes:
>> >>
>> >> > On Sun, Oct 03, 2010 at 11:58:02PM -0700, Greg Thelen wrote:
>> >> >> Extend mem_cgroup to contain dirty page limits. Also add routines
>> >> >> allowing the kernel to query the dirty usage of a memcg.
>> >> >>
>> >> >> These interfaces not used by the kernel yet. A subsequent commit
>> >> >> will add kernel calls to utilize these new routines.
>> >> >
>> >> > A small note below.
>> >> >
>> >> >>
>> >> >> Signed-off-by: Greg Thelen <gthelen at google.com>
>> >> >> Signed-off-by: Andrea Righi <arighi at develer.com>
>> >> >> ---
>> >> >> include/linux/memcontrol.h | 44 +++++++++++
>> >> >> mm/memcontrol.c | 180 +++++++++++++++++++++++++++++++++++++++++++-
>> >> >> 2 files changed, 223 insertions(+), 1 deletions(-)
>> >> >>
>> >> >> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> >> >> index 6303da1..dc8952d 100644
>> >> >> --- a/include/linux/memcontrol.h
>> >> >> +++ b/include/linux/memcontrol.h
>> >> >> @@ -19,6 +19,7 @@
>> >> >>
>> >> >> #ifndef _LINUX_MEMCONTROL_H
>> >> >> #define _LINUX_MEMCONTROL_H
>> >> >> +#include <linux/writeback.h>
>> >> >> #include <linux/cgroup.h>
>> >> >> struct mem_cgroup;
>> >> >> struct page_cgroup;
>> >> >> @@ -33,6 +34,30 @@ enum mem_cgroup_write_page_stat_item {
>> >> >> MEMCG_NR_FILE_UNSTABLE_NFS, /* # of NFS unstable pages */
>> >> >> };
>> >> >>
>> >> >> +/* Cgroup memory statistics items exported to the kernel */
>> >> >> +enum mem_cgroup_read_page_stat_item {
>> >> >> + MEMCG_NR_DIRTYABLE_PAGES,
>> >> >> + MEMCG_NR_RECLAIM_PAGES,
>> >> >> + MEMCG_NR_WRITEBACK,
>> >> >> + MEMCG_NR_DIRTY_WRITEBACK_PAGES,
>> >> >> +};
>> >> >> +
>> >> >> +/* Dirty memory parameters */
>> >> >> +struct vm_dirty_param {
>> >> >> + int dirty_ratio;
>> >> >> + int dirty_background_ratio;
>> >> >> + unsigned long dirty_bytes;
>> >> >> + unsigned long dirty_background_bytes;
>> >> >> +};
>> >> >> +
>> >> >> +static inline void get_global_vm_dirty_param(struct vm_dirty_param *param)
>> >> >> +{
>> >> >> + param->dirty_ratio = vm_dirty_ratio;
>> >> >> + param->dirty_bytes = vm_dirty_bytes;
>> >> >> + param->dirty_background_ratio = dirty_background_ratio;
>> >> >> + param->dirty_background_bytes = dirty_background_bytes;
>> >> >> +}
>> >> >> +
>> >> >> extern unsigned long mem_cgroup_isolate_pages(unsigned long nr_to_scan,
>> >> >> struct list_head *dst,
>> >> >> unsigned long *scanned, int order,
>> >> >> @@ -145,6 +170,10 @@ static inline void mem_cgroup_dec_page_stat(struct page *page,
>> >> >> mem_cgroup_update_page_stat(page, idx, -1);
>> >> >> }
>> >> >>
>> >> >> +bool mem_cgroup_has_dirty_limit(void);
>> >> >> +void get_vm_dirty_param(struct vm_dirty_param *param);
>> >> >> +s64 mem_cgroup_page_stat(enum mem_cgroup_read_page_stat_item item);
>> >> >> +
>> >> >> unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order,
>> >> >> gfp_t gfp_mask);
>> >> >> u64 mem_cgroup_get_limit(struct mem_cgroup *mem);
>> >> >> @@ -326,6 +355,21 @@ static inline void mem_cgroup_dec_page_stat(struct page *page,
>> >> >> {
>> >> >> }
>> >> >>
>> >> >> +static inline bool mem_cgroup_has_dirty_limit(void)
>> >> >> +{
>> >> >> + return false;
>> >> >> +}
>> >> >> +
>> >> >> +static inline void get_vm_dirty_param(struct vm_dirty_param *param)
>> >> >> +{
>> >> >> + get_global_vm_dirty_param(param);
>> >> >> +}
>> >> >> +
>> >> >> +static inline s64 mem_cgroup_page_stat(enum mem_cgroup_read_page_stat_item item)
>> >> >> +{
>> >> >> + return -ENOSYS;
>> >> >> +}
>> >> >> +
>> >> >> static inline
>> >> >> unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order,
>> >> >> gfp_t gfp_mask)
>> >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> >> >> index f40839f..6ec2625 100644
>> >> >> --- a/mm/memcontrol.c
>> >> >> +++ b/mm/memcontrol.c
>> >> >> @@ -233,6 +233,10 @@ struct mem_cgroup {
>> >> >> atomic_t refcnt;
>> >> >>
>> >> >> unsigned int swappiness;
>> >> >> +
>> >> >> + /* control memory cgroup dirty pages */
>> >> >> + struct vm_dirty_param dirty_param;
>> >> >> +
>> >> >> /* OOM-Killer disable */
>> >> >> int oom_kill_disable;
>> >> >>
>> >> >> @@ -1132,6 +1136,172 @@ static unsigned int get_swappiness(struct mem_cgroup *memcg)
>> >> >> return swappiness;
>> >> >> }
>> >> >>
>> >> >> +/*
>> >> >> + * Returns a snapshot of the current dirty limits which is not synchronized with
>> >> >> + * the routines that change the dirty limits. If this routine races with an
>> >> >> + * update to the dirty bytes/ratio value, then the caller must handle the case
>> >> >> + * where both dirty_[background_]_ratio and _bytes are set.
>> >> >> + */
>> >> >> +static void __mem_cgroup_get_dirty_param(struct vm_dirty_param *param,
>> >> >> + struct mem_cgroup *mem)
>> >> >> +{
>> >> >> + if (mem && !mem_cgroup_is_root(mem)) {
>> >> >> + param->dirty_ratio = mem->dirty_param.dirty_ratio;
>> >> >> + param->dirty_bytes = mem->dirty_param.dirty_bytes;
>> >> >> + param->dirty_background_ratio =
>> >> >> + mem->dirty_param.dirty_background_ratio;
>> >> >> + param->dirty_background_bytes =
>> >> >> + mem->dirty_param.dirty_background_bytes;
>> >> >> + } else {
>> >> >> + get_global_vm_dirty_param(param);
>> >> >> + }
>> >> >> +}
>> >> >> +
>> >> >> +/*
>> >> >> + * Get dirty memory parameters of the current memcg or global values (if memory
>> >> >> + * cgroups are disabled or querying the root cgroup).
>> >> >> + */
>> >> >> +void get_vm_dirty_param(struct vm_dirty_param *param)
>> >> >> +{
>> >> >> + struct mem_cgroup *memcg;
>> >> >> +
>> >> >> + if (mem_cgroup_disabled()) {
>> >> >> + get_global_vm_dirty_param(param);
>> >> >> + return;
>> >> >> + }
>> >> >> +
>> >> >> + /*
>> >> >> + * It's possible that "current" may be moved to other cgroup while we
>> >> >> + * access cgroup. But precise check is meaningless because the task can
>> >> >> + * be moved after our access and writeback tends to take long time. At
>> >> >> + * least, "memcg" will not be freed under rcu_read_lock().
>> >> >> + */
>> >> >> + rcu_read_lock();
>> >> >> + memcg = mem_cgroup_from_task(current);
>> >> >> + __mem_cgroup_get_dirty_param(param, memcg);
>> >> >> + rcu_read_unlock();
>> >> >> +}
>> >> >> +
>> >> >> +/*
>> >> >> + * Check if current memcg has local dirty limits. Return true if the current
>> >> >> + * memory cgroup has local dirty memory settings.
>> >> >> + */
>> >> >> +bool mem_cgroup_has_dirty_limit(void)
>> >> >> +{
>> >> >> + struct mem_cgroup *mem;
>> >> >> +
>> >> >> + if (mem_cgroup_disabled())
>> >> >> + return false;
>> >> >> +
>> >> >> + mem = mem_cgroup_from_task(current);
>> >> >> + return mem && !mem_cgroup_is_root(mem);
>> >> >> +}
>> >> >
>> >> > We only check the pointer without dereferencing it, so this is probably
>> >> > ok, but maybe this is safer:
>> >> >
>> >> > bool mem_cgroup_has_dirty_limit(void)
>> >> > {
>> >> > struct mem_cgroup *mem;
>> >> > bool ret;
>> >> >
>> >> > if (mem_cgroup_disabled())
>> >> > return false;
>> >> >
>> >> > rcu_read_lock();
>> >> > mem = mem_cgroup_from_task(current);
>> >> > ret = mem && !mem_cgroup_is_root(mem);
>> >> > rcu_read_unlock();
>> >> >
>> >> > return ret;
>> >> > }
>> >> >
>> >> > rcu_read_lock() should be held in mem_cgroup_from_task(), otherwise
>> >> > lockdep could detect this as an error.
>> >> >
>> >> > Thanks,
>> >> > -Andrea
>> >>
>> >> Good suggestion. I agree that lockdep might catch this. There are some
>> >> unrelated debug_locks failures (even without my patches) that I worked
>> >> around to get lockdep to complain about this one. I applied your
>> >> suggested fix and lockdep was happy. I will incorporate this fix into
>> >> the next revision of the patch series.
>> >>
>> >
>> > Hmm, considering other parts, shouldn't we define mem_cgroup_from_task
>> > as macro ?
>> >
>> > Thanks,
>> > -Kame
>>
>> Is your motivation to increase performance with the same functionality?
>> If so, then would a 'static inline' be performance equivalent to a
>> preprocessor macro yet be safer to use?
>>
> Ah, if lockdep finds this as bug, I think other parts will hit this,
> too. like this.
>> static struct mem_cgroup *try_get_mem_cgroup_from_mm(struct mm_struct *mm)
>> {
>> struct mem_cgroup *mem = NULL;
>>
>> if (!mm)
>> return NULL;
>> /*
>> * Because we have no locks, mm->owner's may be being moved to other
>> * cgroup. We use css_tryget() here even if this looks
>> * pessimistic (rather than adding locks here).
>> */
>> rcu_read_lock();
>> do {
>> mem = mem_cgroup_from_task(rcu_dereference(mm->owner));
>> if (unlikely(!mem))
>> break;
>> } while (!css_tryget(&mem->css));
>> rcu_read_unlock();
>> return mem;
>> }
mem_cgroup_from_task() calls task_subsys_state() calls
task_subsys_state_check(). task_subsys_state_check() will be happy if
rcu_read_lock is held.
I don't think that this will fail lockdep, because rcu_read_lock_held()
is true when calling mem_cgroup_from_task() within
try_get_mem_cgroup_from_mm()..
> mem_cgroup_from_task() is designed to be used as this.
> If dqefined as macro, I think it will not be catched.
I do not understand how making mem_cgroup_from_task() a macro will
change its behavior wrt. to lockdep assertion checking. I assume that
as a macro mem_cgroup_from_task() would still call task_subsys_state(),
which requires either:
a) rcu read lock held
b) task->alloc_lock held
c) cgroup lock held
>> Maybe it makes more sense to find a way to perform this check in
>> mem_cgroup_has_dirty_limit() without needing to grab the rcu lock. I
>> think this lock grab is unneeded. I am still collecting performance
>> data, but suspect that this may be making the code slower than it needs
>> to be.
>>
>
> Hmm. css_set[] itself is freed by RCU..what idea to remove rcu_read_lock() do
> you have ? Adding some flags ?
It seems like a shame to need a lock to determine if current is in the
root cgroup. Especially given that as soon as
mem_cgroup_has_dirty_limit() returns, the task could be moved
in-to/out-of the root cgroup thereby invaliding the answer. So the
answer is just a sample that may be wrong. But I think you are correct.
We will need the rcu read lock in mem_cgroup_has_dirty_limit().
> Ah...I noticed that you should do
>
> mem = mem_cgroup_from_task(current->mm->owner);
>
> to check has_dirty_limit...
What are the cases where current->mm->owner->cgroups !=
current->cgroups?
I was hoping to avoid having add even more logic into
mem_cgroup_has_dirty_limit() to handle the case where current->mm is
NULL.
Presumably the newly proposed vm_dirty_param(),
mem_cgroup_has_dirty_limit(), and mem_cgroup_page_stat() routines all
need to use the same logic. I assume they should all be consistently
using current->mm->owner or current.
--
Greg
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers
More information about the Devel
mailing list