lkml.org 
[lkml]   [2011]   [Jun]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 2/8] mm: memcg-aware global reclaim
    A few small nitpicks:

    > +struct mem_cgroup *mem_cgroup_hierarchy_walk(struct mem_cgroup *root,
    > + struct mem_cgroup *prev)
    > +{
    > + struct mem_cgroup *mem;
    > +
    > + if (mem_cgroup_disabled())
    > + return NULL;
    > +
    > + if (!root)
    > + root = root_mem_cgroup;
    > + /*
    > + * Even without hierarchy explicitely enabled in the root
    > + * memcg, it is the ultimate parent of all memcgs.
    > + */
    > + if (!(root == root_mem_cgroup || root->use_hierarchy))
    > + return root;

    The logic here reads a bit weird, why not simply:

    /*
    * Even without hierarchy explicitely enabled in the root
    * memcg, it is the ultimate parent of all memcgs.
    */
    if (!root || root == root_mem_cgroup)
    return root_mem_cgroup;
    if (root->use_hierarchy)
    return root;


    > /*
    > * This is a basic per-zone page freer. Used by both kswapd and direct reclaim.
    > */
    > -static void shrink_zone(int priority, struct zone *zone,
    > - struct scan_control *sc)
    > +static void do_shrink_zone(int priority, struct zone *zone,
    > + struct scan_control *sc)

    It actually is the per-memcg shrinker now, and thus should be called
    shrink_memcg.

    > + sc->mem_cgroup = mem;
    > + do_shrink_zone(priority, zone, sc);

    Any passing the mem_cgroup explicitly instead of hiding it in the
    scan_control would make that much more obvious. If there's a good
    reason to pass it in the structure the same probably applies to the
    zone and priority, too.

    Shouldn't we also have a non-cgroups stub of shrink_zone to directly
    call do_shrink_zone/shrink_memcg with a NULL memcg and thus optimize
    the whole loop away for it?



    \
     
     \ /
      Last update: 2011-06-07 14:27    [W:0.057 / U:28.520 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site