lkml.org 
[lkml]   [2011]   [Sep]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [patch 01/11] mm: memcg: consolidate hierarchy iteration primitives
    Hi,

    On Mon 12-09-11 12:57:18, Johannes Weiner wrote:
    > Memory control groups are currently bolted onto the side of
    > traditional memory management in places where better integration would
    > be preferrable. To reclaim memory, for example, memory control groups
    > maintain their own LRU list and reclaim strategy aside from the global
    > per-zone LRU list reclaim. But an extra list head for each existing
    > page frame is expensive and maintaining it requires additional code.
    >
    > This patchset disables the global per-zone LRU lists on memory cgroup
    > configurations and converts all its users to operate on the per-memory
    > cgroup lists instead. As LRU pages are then exclusively on one list,
    > this saves two list pointers for each page frame in the system:
    >
    > page_cgroup array size with 4G physical memory
    >
    > vanilla: [ 0.000000] allocated 31457280 bytes of page_cgroup
    > patched: [ 0.000000] allocated 15728640 bytes of page_cgroup
    >
    > At the same time, system performance for various workloads is
    > unaffected:
    >
    > 100G sparse file cat, 4G physical memory, 10 runs, to test for code
    > bloat in the traditional LRU handling and kswapd & direct reclaim
    > paths, without/with the memory controller configured in
    >
    > vanilla: 71.603(0.207) seconds
    > patched: 71.640(0.156) seconds
    >
    > vanilla: 79.558(0.288) seconds
    > patched: 77.233(0.147) seconds
    >
    > 100G sparse file cat in 1G memory cgroup, 10 runs, to test for code
    > bloat in the traditional memory cgroup LRU handling and reclaim path
    >
    > vanilla: 96.844(0.281) seconds
    > patched: 94.454(0.311) seconds
    >
    > 4 unlimited memcgs running kbuild -j32 each, 4G physical memory, 500M
    > swap on SSD, 10 runs, to test for regressions in kswapd & direct
    > reclaim using per-memcg LRU lists with multiple memcgs and multiple
    > allocators within each memcg
    >
    > vanilla: 717.722(1.440) seconds [ 69720.100(11600.835) majfaults ]
    > patched: 714.106(2.313) seconds [ 71109.300(14886.186) majfaults ]
    >
    > 16 unlimited memcgs running kbuild, 1900M hierarchical limit, 500M
    > swap on SSD, 10 runs, to test for regressions in hierarchical memcg
    > setups
    >
    > vanilla: 2742.058(1.992) seconds [ 26479.600(1736.737) majfaults ]
    > patched: 2743.267(1.214) seconds [ 27240.700(1076.063) majfaults ]

    I guess you want to have this in the first patch to have it for
    reference once it gets to the tree, right? I have no objections but it
    seems unrelated to the patch and so it might be confusing a bit. I
    haven't seen other patches in the series so there is probably no better
    place to put this.

    >
    > This patch:
    >
    > There are currently two different implementations of iterating over a
    > memory cgroup hierarchy tree.
    >
    > Consolidate them into one worker function and base the convenience
    > looping-macros on top of it.
    >
    > Signed-off-by: Johannes Weiner <jweiner@redhat.com>

    Looks mostly good. There is just one issue I spotted and I guess we
    want some comments. After the issue is fixed:
    Reviewed-by: Michal Hocko <mhocko@suse.cz>

    > ---
    > mm/memcontrol.c | 196 ++++++++++++++++++++----------------------------------
    > 1 files changed, 73 insertions(+), 123 deletions(-)

    Nice diet.

    >
    > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
    > index b76011a..912c7c7 100644
    > --- a/mm/memcontrol.c
    > +++ b/mm/memcontrol.c
    > @@ -781,83 +781,75 @@ struct mem_cgroup *try_get_mem_cgroup_from_mm(struct mm_struct *mm)
    > return memcg;
    > }
    >
    > -/* The caller has to guarantee "mem" exists before calling this */

    Shouldn't we have a similar comment that we have to keep a reference to
    root if non-NULL. A mention about remember parameter and what is it used
    for (hierarchical reclaim) would be helpful as well.

    /*
    * Find a next cgroup under the hierarchy tree with the given root (or
    * root_mem_cgroup if NULL) starting from the given prev (iterator)
    * position and releasing a reference to it. Start from the root if
    * iterator is NULL.
    * Ignore iterator position if remember is true and follow with the
    * last_scanned_child instead and remember the new value (used during
    * hierarchical reclaim).
    * Caller is supposed to grab a reference to the root (if non NULL) before
    * it calls us for the first time.
    *
    * Returns a cgroup with increased reference count (except for the root)
    * or NULL if there are no more groups to visit.
    *
    * Use for_each_mem_cgroup_tree and for_each_mem_cgroup instead and
    * mem_cgroup_iter_break for the final clean up if you are using this
    * function directly.
    */
    > -static struct mem_cgroup *mem_cgroup_start_loop(struct mem_cgroup *memcg)
    > +static struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root,
    > + struct mem_cgroup *prev,
    > + bool remember)
    [...]
    > @@ -1656,7 +1611,7 @@ static int mem_cgroup_hierarchical_reclaim(struct mem_cgroup *root_memcg,
    > unsigned long reclaim_options,
    > unsigned long *total_scanned)
    > {
    > - struct mem_cgroup *victim;
    > + struct mem_cgroup *victim = NULL;
    > int ret, total = 0;
    > int loop = 0;
    > bool noswap = reclaim_options & MEM_CGROUP_RECLAIM_NOSWAP;
    > @@ -1672,8 +1627,8 @@ static int mem_cgroup_hierarchical_reclaim(struct mem_cgroup *root_memcg,
    > noswap = true;
    >
    > while (1) {
    > - victim = mem_cgroup_select_victim(root_memcg);
    > - if (victim == root_memcg) {
    > + victim = mem_cgroup_iter(root_memcg, victim, true);
    > + if (!victim) {
    > loop++;
    > /*
    > * We are not draining per cpu cached charges during
    > @@ -1689,10 +1644,8 @@ static int mem_cgroup_hierarchical_reclaim(struct mem_cgroup *root_memcg,
    > * anything, it might because there are
    > * no reclaimable pages under this hierarchy
    > */
    > - if (!check_soft || !total) {
    > - css_put(&victim->css);
    > + if (!check_soft || !total)
    > break;
    > - }
    > /*
    > * We want to do more targeted reclaim.
    > * excess >> 2 is not to excessive so as to
    > @@ -1700,15 +1653,13 @@ static int mem_cgroup_hierarchical_reclaim(struct mem_cgroup *root_memcg,
    > * coming back to reclaim from this cgroup
    > */
    > if (total >= (excess >> 2) ||
    > - (loop > MEM_CGROUP_MAX_RECLAIM_LOOPS)) {
    > - css_put(&victim->css);
    > + (loop > MEM_CGROUP_MAX_RECLAIM_LOOPS))
    > break;
    > - }
    > }
    > + continue;
    > }
    > if (!mem_cgroup_reclaimable(victim, noswap)) {
    > /* this cgroup's local usage == 0 */
    > - css_put(&victim->css);
    > continue;
    > }
    > /* we use swappiness of local cgroup */
    > @@ -1719,21 +1670,21 @@ static int mem_cgroup_hierarchical_reclaim(struct mem_cgroup *root_memcg,
    > } else
    > ret = try_to_free_mem_cgroup_pages(victim, gfp_mask,
    > noswap);
    > - css_put(&victim->css);
    > /*
    > * At shrinking usage, we can't check we should stop here or
    > * reclaim more. It's depends on callers. last_scanned_child
    > * will work enough for keeping fairness under tree.
    > */
    > if (shrink)
    > - return ret;
    > + break;

    Hmm, we are returning total but it doesn't get set to ret for shrinking
    case so we are alway returning 0. You want to move the line bellow up.

    > total += ret;
    > if (check_soft) {
    > if (!res_counter_soft_limit_excess(&root_memcg->res))
    > - return total;
    > + break;
    > } else if (mem_cgroup_margin(root_memcg))
    > - return total;
    > + break;
    > }
    > + mem_cgroup_iter_break(root_memcg, victim);
    > return total;
    > }

    [...]

    Thanks
    --
    Michal Hocko
    SUSE Labs
    SUSE LINUX s.r.o.
    Lihovarska 1060/12
    190 00 Praha 9
    Czech Republic


    \
     
     \ /
      Last update: 2011-09-19 14:55    [W:0.038 / U:39.012 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site