lkml.org 
[lkml]   [2011]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [patch] Revert "memcg: add memory.vmscan_stat"
    From
    On Tue, Aug 30, 2011 at 1:42 AM, Johannes Weiner <jweiner@redhat.com> wrote:
    > On Tue, Aug 30, 2011 at 04:20:50PM +0900, KAMEZAWA Hiroyuki wrote:
    >> On Tue, 30 Aug 2011 09:04:24 +0200
    >> Johannes Weiner <jweiner@redhat.com> wrote:
    >>
    >> > On Tue, Aug 30, 2011 at 10:12:33AM +0900, KAMEZAWA Hiroyuki wrote:
    >> > > @@ -1710,11 +1711,18 @@ static void mem_cgroup_record_scanstat(s
    >> > >   spin_lock(&memcg->scanstat.lock);
    >> > >   __mem_cgroup_record_scanstat(memcg->scanstat.stats[context], rec);
    >> > >   spin_unlock(&memcg->scanstat.lock);
    >> > > -
    >> > > - memcg = rec->root;
    >> > > - spin_lock(&memcg->scanstat.lock);
    >> > > - __mem_cgroup_record_scanstat(memcg->scanstat.rootstats[context], rec);
    >> > > - spin_unlock(&memcg->scanstat.lock);
    >> > > + cgroup = memcg->css.cgroup;
    >> > > + do {
    >> > > +         spin_lock(&memcg->scanstat.lock);
    >> > > +         __mem_cgroup_record_scanstat(
    >> > > +                 memcg->scanstat.hierarchy_stats[context], rec);
    >> > > +         spin_unlock(&memcg->scanstat.lock);
    >> > > +         if (!cgroup->parent)
    >> > > +                 break;
    >> > > +         cgroup = cgroup->parent;
    >> > > +         memcg = mem_cgroup_from_cont(cgroup);
    >> > > + } while (memcg->use_hierarchy && memcg != rec->root);
    >> >
    >> > Okay, so this looks correct, but it sums up all parents after each
    >> > memcg scanned, which could have a performance impact.  Usually,
    >> > hierarchy statistics are only summed up when a user reads them.
    >> >
    >> Hmm. But sum-at-read doesn't work.
    >>
    >> Assume 3 cgroups in a hierarchy.
    >>
    >>       A
    >>        /
    >>       B
    >>      /
    >>     C
    >>
    >> C's scan contains 3 causes.
    >>       C's scan caused by limit of A.
    >>       C's scan caused by limit of B.
    >>       C's scan caused by limit of C.
    >>
    >> If we make hierarchy sum at read, we think
    >>       B's scan_stat = B's scan_stat + C's scan_stat
    >> But in precice, this is
    >>
    >>       B's scan_stat = B's scan_stat caused by B +
    >>                       B's scan_stat caused by A +
    >>                       C's scan_stat caused by C +
    >>                       C's scan_stat caused by B +
    >>                       C's scan_stat caused by A.
    >>
    >> In orignal version.
    >>       B's scan_stat = B's scan_stat caused by B +
    >>                       C's scan_stat caused by B +
    >>
    >> After this patch,
    >>       B's scan_stat = B's scan_stat caused by B +
    >>                       B's scan_stat caused by A +
    >>                       C's scan_stat caused by C +
    >>                       C's scan_stat caused by B +
    >>                       C's scan_stat caused by A.
    >>
    >> Hmm...removing hierarchy part completely seems fine to me.
    >
    > I see.
    >
    > You want to look at A and see whether its limit was responsible for
    > reclaim scans in any children.  IMO, that is asking the question
    > backwards.  Instead, there is a cgroup under reclaim and one wants to
    > find out the cause for that.  Not the other way round.
    >
    > In my original proposal I suggested differentiating reclaim caused by
    > internal pressure (due to own limit) and reclaim caused by
    > external/hierarchical pressure (due to limits from parents).
    >
    > If you want to find out why C is under reclaim, look at its reclaim
    > statistics.  If the _limit numbers are high, C's limit is the problem.
    > If the _hierarchical numbers are high, the problem is B, A, or
    > physical memory, so you check B for _limit and _hierarchical as well,
    > then move on to A.
    >
    > Implementing this would be as easy as passing not only the memcg to
    > scan (victim) to the reclaim code, but also the memcg /causing/ the
    > reclaim (root_mem):
    >
    >        root_mem == victim -> account to victim as _limit
    >        root_mem != victim -> account to victim as _hierarchical
    >
    > This would make things much simpler and more natural, both the code
    > and the way of tracking down a problem, IMO.

    This is pretty much the stats I am currently using for debugging the
    reclaim patches. For example:

    scanned_pages_by_system 0
    scanned_pages_by_system_under_hierarchy 50989

    scanned_pages_by_limit 0
    scanned_pages_by_limit_under_hierarchy 0

    "_system" is count under global reclaim, and "_limit" is count under
    per-memcg reclaim.
    "_under_hiearchy" is set if memcg is not the one triggering pressure.

    So in the previous example:

    > A (root)
    > /
    > B
    > /
    > C

    For cgroup C:
    scanned_pages_by_system:
    scanned_pages_by_system_under_hierarchy: # of pages scanned under
    global memory pressure

    scanned_pages_by_limit: # of pages scanned while C hits the limit
    scanned_pages_by_limit_under_hierarchy: # of pages scanned while B
    hits the limit

    --Ying

    >
    >> > I don't get why this has to be done completely different from the way
    >> > we usually do things, without any justification, whatsoever.
    >> >
    >> > Why do you want to pass a recording structure down the reclaim stack?
    >>
    >> Just for reducing number of passed variables.
    >
    > It's still sitting on bottom of the reclaim stack the whole time.
    >
    > With my proposal, you would only need to pass the extra root_mem
    > pointer.
    >
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-09-01 08:07    [W:0.039 / U:1.696 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site