lkml.org 
[lkml]   [2008]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH 1/3] cgroup: fix pre_destroy and semantics of css->refcnt
    KAMEZAWA Hiroyuki wrote:
    > On Tue, 02 Dec 2008 14:56:52 +0800
    > Li Zefan <lizf@cn.fujitsu.com> wrote:
    >
    >> KAMEZAWA Hiroyuki wrote:
    >>> On Tue, 02 Dec 2008 14:15:23 +0800
    >>> Li Zefan <lizf@cn.fujitsu.com> wrote:
    >>>
    >>>> KAMEZAWA Hiroyuki wrote:
    >>>>> Now, final check of refcnt is done after pre_destroy(), so rmdir() can fail
    >>>>> after pre_destroy().
    >>>>> memcg set mem->obsolete to be 1 at pre_destroy and this is buggy..
    >>>>>
    >>>>> Several ways to fix this can be considered. This is an idea.
    >>>>>
    >>>> I don't see what's the difference with css_under_removal() in this patch and
    >>>> cgroup_is_removed() which is currently available.
    >>>>
    >>>> CGRP_REMOVED flag is set in cgroup_rmdir() when it's confirmed that rmdir can
    >>>> be sucessfully performed.
    >>>>
    >>>> So mem->obsolete can be replaced with:
    >>>>
    >>>> bool mem_cgroup_is_obsolete(struct mem_cgroup *mem)
    >>>> {
    >>>> return cgroup_is_removed(mem->css.cgroup);
    >>>> }
    >>>>
    >>>> Or am I missing something?
    >>>>
    >>> Yes.
    >>> 1. "cgroup" and "css" object are different object.
    >>> 2. css object may not be freed at destroy() (as current memcg does.)
    >>>
    >>> Some of css objects cannot be freed even when there are no tasks because
    >>> of reference from some persistent object or temporal refcnt.
    >>>
    >> I just noticed mem_cgroup has its own refcnt now. The memcg code has changed
    >> dramatically that I don't catch up with it. Thx for the explanation.
    >>
    >> But I have another doubt:
    >>
    >> void mem_cgroup_uncharge_swapcache(struct page *page, swp_entry_t ent)
    >> {
    >> struct mem_cgroup *memcg;
    >>
    >> memcg = __mem_cgroup_uncharge_common(page,
    >> MEM_CGROUP_CHARGE_TYPE_SWAPOUT);
    >> /* record memcg information */
    >> if (do_swap_account && memcg) {
    >> swap_cgroup_record(ent, memcg);
    >> mem_cgroup_get(memcg);
    >> }
    >> }
    >>
    >> In the above code, is it possible that memcg is freed before mem_cgroup_get()
    >> increases memcg->refcnt?
    >>
    > Thank you for looking into. maybe possible.
    >
    > In this case,
    > 1. "the page" was belongs to memcg before uncharge().
    > 2. but it's not guaranteed that memcg is alive after uncharge.
    >
    > OK. maybe css_tryget() can change this to be
    > ==
    > rcu_read_lock();
    > memcg = __mem_cgroup_uncharge_common(page,
    > MEM_CGROUP_CHARGE_TYPE_SWAPOUT);
    > if (do_swap_account && memcg && css_tryget(&memcg->css)) {
    > swap_cgroup_record(ent, memcg);
    > mem_cgroup_get(memcg);
    > css_put(&memcg->css);
    > }
    > rcu_read_unlock();
    > ==
    > How about this ?
    >

    Seems OK for me. Another way to fix this is, don't call css_put() if we want
    to use the memcg returned from __mem_cgroup_uncharge_common(), I think this
    is more reasonable:

    --- a/mm/memcontrol.c.orig 2008-12-02 15:20:55.000000000 +0800
    +++ b/mm/memcontrol.c 2008-12-02 15:28:07.000000000 +0800
    @@ -1110,8 +1110,9 @@ void mem_cgroup_cancel_charge_swapin(str
    /*
    * uncharge if !page_mapped(page)
    */
    -static struct mem_cgroup *
    -__mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
    +static void
    +__mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype,
    + struct mem_cgroup **memcg)
    {
    struct page_cgroup *pc;
    struct mem_cgroup *mem = NULL;
    @@ -1163,13 +1164,16 @@ __mem_cgroup_uncharge_common(struct page
    mz = page_cgroup_zoneinfo(pc);
    unlock_page_cgroup(pc);

    - css_put(&mem->css);
    + /* don't dec refcnt, since the caller want to use this memcg */
    + if (memcg)
    + *memcg = mem;
    + else
    + css_put(&mem->css);

    - return mem;
    + return;

    unlock_out:
    unlock_page_cgroup(pc);
    - return NULL;
    }

    void mem_cgroup_uncharge_page(struct page *page)
    @@ -1197,12 +1201,13 @@ void mem_cgroup_uncharge_swapcache(struc
    {
    struct mem_cgroup *memcg;

    - memcg = __mem_cgroup_uncharge_common(page,
    - MEM_CGROUP_CHARGE_TYPE_SWAPOUT);
    + __mem_cgroup_uncharge_common(page,
    + MEM_CGROUP_CHARGE_TYPE_SWAPOUT, &memcg);
    /* record memcg information */
    if (do_swap_account && memcg) {
    swap_cgroup_record(ent, memcg);
    mem_cgroup_get(memcg);
    + css_put(&memcg->css);
    }
    }



    \
     
     \ /
      Last update: 2008-12-02 08:33    [W:0.028 / U:0.864 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site