lkml.org 
[lkml]   [2008]   [Jun]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC 5/5] Memory controller soft limit reclaim on contention
    Paul Menage wrote:
    > On Fri, Jun 27, 2008 at 8:19 AM, Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
    >> +/*
    >> + * Create a heap of memory controller structures. The heap is reverse
    >> + * sorted by size. This heap is used for implementing soft limits. Our
    >> + * current heap implementation does not allow dynamic heap updates, but
    >> + * eventually, the costliest controller (over it's soft limit should
    >
    > it's -> its
    >

    Yes

    >> + old_mem = heap_insert(&mem_cgroup_heap, mem,
    >> + HEAP_REP_LEAF);
    >> + mem->on_heap = 1;
    >> + if (old_mem)
    >> + old_mem->on_heap = 0;
    >
    > Maybe a comment here that mem might == old_mem?
    >
    >> + * When the soft limit is exceeded, look through the heap and start
    >> + * reclaiming from all groups over thier soft limit
    >
    > thier -> their
    >

    Will fix

    >> + if (!res_counter_check_under_soft_limit(&mem->res)) {
    >> + /*
    >> + * The current task might already be over it's soft
    >> + * limit and trying to aggressively grow. We check to
    >> + * see if it the memory group associated with the
    >> + * current task is on the heap when the current group
    >> + * is over it's soft limit. If not, we add it
    >> + */
    >> + if (!mem->on_heap) {
    >> + struct mem_cgroup *old_mem;
    >> +
    >> + old_mem = heap_insert(&mem_cgroup_heap, mem,
    >> + HEAP_REP_LEAF);
    >> + mem->on_heap = 1;
    >> + if (old_mem)
    >> + old_mem->on_heap = 0;
    >> + }
    >> + }
    >
    > This and the other similar code for adding to the heap should be
    > refactored into a separate function.
    >

    OK, I can look into that.

    >> +static int mem_cgroup_compare_soft_limits(void *p1, void *p2)
    >> +{
    >> + struct mem_cgroup *mem1 = (struct mem_cgroup *)p1;
    >> + struct mem_cgroup *mem2 = (struct mem_cgroup *)p2;
    >> + unsigned long long delta1, delta2;
    >> +
    >> + delta1 = res_counter_soft_limit_delta(&mem1->res);
    >> + delta2 = res_counter_soft_limit_delta(&mem2->res);
    >> +
    >> + return delta1 > delta2;
    >> +}
    >
    > This isn't a valid comparator, since it isn't a constant function of
    > its two input pointers - calling mem_cgroup_compare_soft_limits(m1,
    > m2) can give different results at different times. So your heap
    > invariant will become invalid over time.
    >
    > I think if you want to do this, you're going to need to periodically
    > take a snapshot of each cgroup's excess and use that snapshot in the
    > comparator; whenever you update the snapshots, you'll need to restore
    > the heap invariant.
    >

    I'll fix it by taking snapshots only before inserting an element into the heap
    (I think I responded to this one in another email, but missed out on the typos).

    --
    Warm Regards,
    Balbir Singh
    Linux Technology Center
    IBM, ISTL


    \
     
     \ /
      Last update: 2008-06-30 05:45    [W:0.026 / U:29.876 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site