lkml.org 
[lkml]   [2010]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2][memcg+dirtylimit] Fix overwriting global vm dirty limit setting by memcg (Re: [PATCH v3 00/11] memcg: per cgroup dirty page accounting
    Date
    KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> writes:

    > Fixed one here.
    > ==
    > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    >
    > Now, at calculating dirty limit, vm_dirty_param() is called.
    > This function returns dirty-limit related parameters considering
    > memory cgroup settings.
    >
    > Now, assume that vm_dirty_bytes=100M (global dirty limit) and
    > memory cgroup has 1G of pages and 40 dirty_ratio, dirtyable memory is
    > 500MB.
    >
    > In this case, global_dirty_limits will consider dirty_limt as
    > 500 *0.4 = 200MB. This is bad...memory cgroup is not back door.
    >
    > This patch limits the return value of vm_dirty_param() considring
    > global settings.
    >
    > Changelog:
    > - fixed an argument "mem" int to u64
    > - fixed to use global available memory to cap memcg's value.
    >
    > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
    > ---
    > include/linux/memcontrol.h | 5 +++--
    > mm/memcontrol.c | 30 +++++++++++++++++++++++++++++-
    > mm/page-writeback.c | 3 ++-
    > 3 files changed, 34 insertions(+), 4 deletions(-)
    >
    > Index: dirty_limit_new/mm/memcontrol.c
    > ===================================================================
    > --- dirty_limit_new.orig/mm/memcontrol.c
    > +++ dirty_limit_new/mm/memcontrol.c
    > @@ -1171,9 +1171,11 @@ static void __mem_cgroup_dirty_param(str
    > * can be moved after our access and writeback tends to take long time. At
    > * least, "memcg" will not be freed while holding rcu_read_lock().
    > */
    > -void vm_dirty_param(struct vm_dirty_param *param)
    > +void vm_dirty_param(struct vm_dirty_param *param,
    > + u64 mem, u64 global)
    > {
    > struct mem_cgroup *memcg;
    > + u64 limit, bglimit;
    >
    > if (mem_cgroup_disabled()) {
    > global_vm_dirty_param(param);
    > @@ -1183,6 +1185,32 @@ void vm_dirty_param(struct vm_dirty_para
    > rcu_read_lock();
    > memcg = mem_cgroup_from_task(current);
    > __mem_cgroup_dirty_param(param, memcg);
    > + /*
    > + * A limitation under memory cgroup is under global vm, too.
    > + */
    > + if (vm_dirty_ratio)
    > + limit = global * vm_dirty_ratio / 100;
    > + else
    > + limit = vm_dirty_bytes;
    > + if (param->dirty_ratio) {
    > + param->dirty_bytes = mem * param->dirty_ratio / 100;
    > + param->dirty_ratio = 0;
    > + }
    > + if (param->dirty_bytes > limit)
    > + param->dirty_bytes = limit;
    > +
    > + if (dirty_background_ratio)
    > + bglimit = global * dirty_background_ratio / 100;
    > + else
    > + bglimit = dirty_background_bytes;
    > +
    > + if (param->dirty_background_ratio) {
    > + param->dirty_background_bytes =
    > + mem * param->dirty_background_ratio / 100;
    > + param->dirty_background_ratio = 0;
    > + }
    > + if (param->dirty_background_bytes > bglimit)
    > + param->dirty_background_bytes = bglimit;
    > rcu_read_unlock();
    > }
    >
    > Index: dirty_limit_new/include/linux/memcontrol.h
    > ===================================================================
    > --- dirty_limit_new.orig/include/linux/memcontrol.h
    > +++ dirty_limit_new/include/linux/memcontrol.h
    > @@ -171,7 +171,7 @@ static inline void mem_cgroup_dec_page_s
    > }
    >
    > bool mem_cgroup_has_dirty_limit(void);
    > -void vm_dirty_param(struct vm_dirty_param *param);
    > +void vm_dirty_param(struct vm_dirty_param *param, u64 mem, u64 global);
    > s64 mem_cgroup_page_stat(enum mem_cgroup_nr_pages_item item);
    >
    > unsigned long mem_cgroup_soft_limit_reclaim(struct zone *zone, int order,
    > @@ -360,7 +360,8 @@ static inline bool mem_cgroup_has_dirty_
    > return false;
    > }
    >
    > -static inline void vm_dirty_param(struct vm_dirty_param *param)
    > +static inline void vm_dirty_param(struct vm_dirty_param *param,
    > + u64 mem, u64 global)
    > {
    > global_vm_dirty_param(param);
    > }
    > Index: dirty_limit_new/mm/page-writeback.c
    > ===================================================================
    > --- dirty_limit_new.orig/mm/page-writeback.c
    > +++ dirty_limit_new/mm/page-writeback.c
    > @@ -466,7 +466,8 @@ void global_dirty_limits(unsigned long *
    > struct task_struct *tsk;
    > struct vm_dirty_param dirty_param;
    >
    > - vm_dirty_param(&dirty_param);
    > + vm_dirty_param(&dirty_param,
    > + available_memory, global_dirtyable_memory());
    >
    > if (dirty_param.dirty_bytes)
    > dirty = DIV_ROUND_UP(dirty_param.dirty_bytes, PAGE_SIZE);

    I think there is a problem with the patch above. In the patch
    vm_dirty_param() sets param->dirty_[background_]bytes to the smallest
    limits considering the memcg and global limits. Assuming the current
    task is in a memcg, then the memcg dirty (not system-wide) usage is
    always compared to the selected limits (which may be per-memcg or
    system). The problem is that if:
    a) per-memcg dirty limit is smaller than system then vm_dirty_param()
    will select per-memcg dirty limit, and
    b) per-memcg dirty usage is well below memcg dirty limit, and
    b) system usage is at system limit
    Then the above patch will not trigger writeback. Example with two
    memcg:
    sys
    B C

    limit usage
    sys 10 10
    B 7 6
    C 5 4

    If B wants to grow, the system will exceed system limit of 10 and should
    be throttled. However, the smaller limit (7) will be selected and
    applied to memcg usage (6), which indicates no need to throttle, so the
    system could get as bad as:

    limit usage
    sys 10 12
    B 7 7
    C 5 5

    In this case the system usage exceeds the system limit because each
    the per-memcg checks see no per-memcg problems.

    To solve this I propose we create a new structure to aggregate both
    dirty limit and usage data:
    struct dirty_limits {
    unsigned long dirty_thresh;
    unsigned long background_thresh;
    unsigned long nr_reclaimable;
    unsigned long nr_writeback;
    };

    global_dirty_limits() would then query both the global and memcg limits
    and dirty usage of one that is closest to its limit. This change makes
    global_dirty_limits() look like:

    void global_dirty_limits(struct dirty_limits *limits)
    {
    unsigned long background;
    unsigned long dirty;
    unsigned long nr_reclaimable;
    unsigned long nr_writeback;
    unsigned long available_memory = determine_dirtyable_memory();
    struct task_struct *tsk;

    if (vm_dirty_bytes)
    dirty = DIV_ROUND_UP(vm_dirty_bytes, PAGE_SIZE);
    else
    dirty = (vm_dirty_ratio * available_memory) / 100;

    if (dirty_background_bytes)
    background = DIV_ROUND_UP(dirty_background_bytes, PAGE_SIZE);
    else
    background = (dirty_background_ratio * available_memory) / 100;

    nr_reclaimable = global_page_state(NR_FILE_DIRTY) +
    global_page_state(NR_UNSTABLE_NFS);
    nr_writeback = global_page_state(NR_WRITEBACK);

    if (mem_cgroup_dirty_limits(available_memory, limits) &&
    dirty_available(limits->dirty_thresh, limits->nr_reclaimable,
    limits->nr_writeback) <
    dirty_available(dirty, nr_reclaimable, nr_writeback)) {
    dirty = min(dirty, limits->dirty_thresh);
    background = min(background, limits->background_thresh);
    } else {
    limits->nr_reclaimable = nr_reclaimable;
    limits->nr_writeback = nr_writeback;
    }

    if (background >= dirty)
    background = dirty / 2;
    tsk = current;
    if (tsk->flags & PF_LESS_THROTTLE || rt_task(tsk)) {
    background += background / 4;
    dirty += dirty / 4;
    }
    limits->background_thresh = background;
    limits->dirty_thresh = dirty;
    }

    Because this approach considered both memcg and system limits, the
    problem described above is avoided.

    I have this change integrated into the memcg dirty limit series (-v3 was
    the last post; v4 is almost ready with this change). I will post -v4
    with this approach is there is no strong objection.

    --
    Greg


    \
     
     \ /
      Last update: 2010-10-24 20:47    [W:0.032 / U:60.228 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site