lkml.org 
[lkml]   [2012]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 09/11] memcg: propagate kmem limiting information to children
    Feeling like a nit pervert but..

    On Mon, Jun 25, 2012 at 06:15:26PM +0400, Glauber Costa wrote:
    > @@ -287,7 +287,11 @@ struct mem_cgroup {
    > * Should the accounting and control be hierarchical, per subtree?
    > */
    > bool use_hierarchy;
    > - bool kmem_accounted;
    > + /*
    > + * bit0: accounted by this cgroup
    > + * bit1: accounted by a parent.
    > + */
    > + volatile unsigned long kmem_accounted;

    Is the volatile declaration really necessary? Why is it necessary?
    Why no comment explaining it?

    > +#ifdef CONFIG_CGROUP_MEM_RES_CTLR_KMEM
    > +static void mem_cgroup_update_kmem_limit(struct mem_cgroup *memcg, u64 val)
    > +{
    > + struct mem_cgroup *iter;
    > +
    > + mutex_lock(&set_limit_mutex);
    > + if (!test_and_set_bit(KMEM_ACCOUNTED_THIS, &memcg->kmem_accounted) &&
    > + val != RESOURCE_MAX) {
    > +
    > + /*
    > + * Once enabled, can't be disabled. We could in theory
    > + * disable it if we haven't yet created any caches, or
    > + * if we can shrink them all to death.
    > + *
    > + * But it is not worth the trouble
    > + */
    > + static_key_slow_inc(&mem_cgroup_kmem_enabled_key);
    > +
    > + if (!memcg->use_hierarchy)
    > + goto out;
    > +
    > + for_each_mem_cgroup_tree(iter, memcg) {
    > + if (iter == memcg)
    > + continue;
    > + set_bit(KMEM_ACCOUNTED_PARENT, &iter->kmem_accounted);
    > + }
    > +
    > + } else if (test_and_clear_bit(KMEM_ACCOUNTED_THIS, &memcg->kmem_accounted)
    > + && val == RESOURCE_MAX) {
    > +
    > + if (!memcg->use_hierarchy)
    > + goto out;
    > +
    > + for_each_mem_cgroup_tree(iter, memcg) {
    > + struct mem_cgroup *parent;

    Blank line between decl and body please.

    > + if (iter == memcg)
    > + continue;
    > + /*
    > + * We should only have our parent bit cleared if none of
    > + * ouri parents are accounted. The transversal order of

    ^ type

    > + * our iter function forces us to always look at the
    > + * parents.

    Also, it's okay here but the text filling in comments and patch
    descriptions tend to be quite inconsistent. If you're on emacs, alt-q
    is your friend and I'm sure vim can do text filling pretty nicely too.

    > + */
    > + parent = parent_mem_cgroup(iter);
    > + while (parent && (parent != memcg)) {
    > + if (test_bit(KMEM_ACCOUNTED_THIS, &parent->kmem_accounted))
    > + goto noclear;
    > +
    > + parent = parent_mem_cgroup(parent);
    > + }

    Better written in for (;;)? Also, if we're breaking on parent ==
    memcg, can we ever hit NULL parent in the above loop?

    > + clear_bit(KMEM_ACCOUNTED_PARENT, &iter->kmem_accounted);
    > +noclear:
    > + continue;
    > + }
    > + }
    > +out:
    > + mutex_unlock(&set_limit_mutex);

    Can we please branch on val != RECOURSE_MAX first? I'm not even sure
    whether the above conditionals are correct. If the user updates an
    existing kmem limit, the first test_and_set_bit() returns non-zero, so
    the code proceeds onto clearing KMEM_ACCOUNTED_THIS, which succeeds
    but val == RESOURCE_MAX fails so it doesn't do anything. If the user
    changes it again, it will set ACCOUNTED_THIS again. So, changing an
    existing kmem limit toggles KMEM_ACCOUNTED_THIS, which just seems
    wacky to me.

    Thanks.

    --
    tejun


    \
     
     \ /
      Last update: 2012-06-25 21:21    [W:0.028 / U:32.084 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site