lkml.org 
[lkml]   [2008]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC][PATCH 2/2] memcg: hardwall hierarhcy for memcg
    On Wed, 4 Jun 2008 01:59:12 -0700
    "Paul Menage" <menage@google.com> wrote:

    > On Tue, Jun 3, 2008 at 10:03 PM, KAMEZAWA Hiroyuki
    > <kamezawa.hiroyu@jp.fujitsu.com> wrote:
    > > @@ -792,6 +798,89 @@ int mem_cgroup_shrink_usage(struct mm_st
    > > }
    > >
    > > /*
    > > + * Memory Controller hierarchy support.
    > > + */
    > > +
    > > +/*
    > > + * shrink usage to be res->usage + val < res->limit.
    > > + */
    > > +
    > > +int memcg_shrink_val(struct res_counter *cnt, unsigned long long val)
    > > +{
    > > + struct mem_cgroup *memcg = container_of(cnt, struct mem_cgroup, res);
    > > + unsigned long flags;
    > > + int ret = 1;
    > > + int progress = 1;
    > > +
    > > +retry:
    > > + spin_lock_irqsave(&cnt->lock, flags);
    > > + /* Need to shrink ? */
    > > + if (cnt->usage + val <= cnt->limit)
    > > + ret = 0;
    > > + spin_unlock_irqrestore(&cnt->lock, flags);
    >
    > Can't this logic be in res_counter itself? I.e. the callback can
    > assume that some shrinking needs to be done, and should just do it and
    > return. The res_counter can handle retrying if necessary.
    >
    Hmm ok. Maybe All I have to do is to define "What the callback has to do"
    and to move this check interface to res_counter.


    > > +/*
    > > + * For Hard Wall Hierarchy.
    > > + */
    > > +
    > > +int mem_cgroup_resize_callback(struct res_counter *cnt,
    > > + unsigned long long val, int what)
    > > +{
    > > + unsigned long flags, borrow;
    > > + unsigned long long diffs;
    > > + int ret = 0;
    > > +
    > > + BUG_ON(what != RES_LIMIT);
    > > +
    > > + /* Is this under hierarchy ? */
    > > + if (!cnt->parent) {
    > > + spin_lock_irqsave(&cnt->lock, flags);
    > > + cnt->limit = val;
    > > + spin_unlock_irqrestore(&cnt->lock, flags);
    > > + return 0;
    > > + }
    > > +
    > > + spin_lock_irqsave(&cnt->lock, flags);
    > > + if (val > cnt->limit) {
    > > + diffs = val - cnt->limit;
    > > + borrow = 1;
    > > + } else {
    > > + diffs = cnt->limit - val;
    > > + borrow = 0;
    > > + }
    > > + spin_unlock_irqrestore(&cnt->lock, flags);
    > > +
    > > + if (borrow)
    > > + ret = res_counter_move_resource(cnt,diffs,
    > > + memcg_shrink_val,
    > > + MEM_CGROUP_RECLAIM_RETRIES);
    > > + else
    > > + ret = res_counter_return_resource(cnt, diffs,
    > > + memcg_shrink_val,
    > > + MEM_CGROUP_RECLAIM_RETRIES);
    > > + return ret;
    > > +}
    >
    > Again, a lot of this function seems like generic logic that should be
    > in res_counter. The only bit that's memory specific is the
    > memcg_shrink_val, and maybe that could just be passed when creating
    > the res_counter. Perhaps we should have a res_counter_ops structure
    > with operations like "parse" for parsing strings into numbers
    > (currently called "write_strategy") and "reclaim" for trying to shrink
    > the usage.
    >
    ok, will try.


    > > @@ -896,11 +987,44 @@ static ssize_t mem_cgroup_write(struct c
    > > struct file *file, const char __user *userbuf,
    > > size_t nbytes, loff_t *ppos)
    > > {
    > > - return res_counter_write(&mem_cgroup_from_cont(cont)->res,
    > > + struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
    > > +
    > > + if (cft->private != RES_LIMIT
    > > + || !cont->parent
    > > + || memcg->hierarchy_model == MEMCG_NO_HIERARCHY)
    >
    > The res_counter already knows whether it has a parent, so these checks
    > shouldn't be necessary.
    >
    ok, will check in res_counter itself.

    > > @@ -1096,6 +1238,12 @@ static void mem_cgroup_destroy(struct cg
    > > int node;
    > > struct mem_cgroup *mem = mem_cgroup_from_cont(cont);
    > >
    > > + if (cont->parent &&
    > > + mem->hierarchy_model == MEMCG_HARDWALL_HIERARCHY) {
    > > + /* we did what we can...just returns what we borrow */
    > > + res_counter_return_resource(&mem->res, -1, NULL, 0);
    > > + }
    > > +
    >
    > Should we also re-account any remaining child usage to the parent?
    >
    When this is called, there are no process in this group. Then, remaining
    resources in this level is
    - file cache
    - swap cache (if shared)
    - shmem

    And the biggest usage will be "file cache".
    So, I don't think it's necessary to move child's usage to the parent,
    in hurry. But maybe shmem is worth to be moved.

    I'd like to revisit this when I implements "usage move at task move"
    logic. (currenty, memory usage doesn't move to new cgroup at task_attach.)

    It will help me to implement the logic "move remaining usage to the parent"
    in clean way.

    Thanks,
    -Kame











    \
     
     \ /
      Last update: 2008-06-04 11:23    [W:3.028 / U:1.612 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site