lkml.org 
[lkml]   [2012]   [Feb]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [RFC][PATCH] memcg: avoid THP split in task migration
    Date
    Hi,

    On Wed, Feb 29, 2012 at 09:28:59AM +0900, KAMEZAWA Hiroyuki wrote:
    > On Tue, 28 Feb 2012 16:12:32 -0500
    > Naoya Horiguchi <n-horiguchi@ah.jp.nec.com> wrote:
    ...
    > > @@ -5378,16 +5420,38 @@ static int mem_cgroup_move_charge_pte_range(pmd_t *pmd,
    > > struct vm_area_struct *vma = walk->private;
    > > pte_t *pte;
    > > spinlock_t *ptl;
    > > + int type;
    > > + union mc_target target;
    > > + struct page *page;
    > > + struct page_cgroup *pc;
    > > +
    > > + if (pmd_trans_huge_lock(pmd, vma) == 1) {
    > > + if (!mc.precharge)
    > > + return 0;
    > > + type = is_target_huge_pmd_for_mc(vma, addr, *pmd, &target);
    > > + if (type == MC_TARGET_PAGE) {
    > > + page = target.page;
    > > + if (!isolate_lru_page(page)) {
    > > + pc = lookup_page_cgroup(page);
    >
    > Here is a diffuclut point. Please see mem_cgroup_split_huge_fixup(). It splits
    > updates memcg's status of splitted pages under lru_lock and compound_lock
    > but not under mm->page_table_lock.

    OK, I rethink locking.
    mem_cgroup_move_account() also states that the caller should hold compound_lock(),
    so I should follow that.

    > Looking into split_huge_page()
    >
    > split_huge_page() # take anon_vma lock
    > __split_huge_page()
    > __split_huge_page_refcount() # take lru_lock, compound_lock.
    > mem_cgroup_split_huge_fixup()
    > __split_huge_page_map() # take page table lock.

    I'm afraid this callchain is not correct.
    Page table lock seems to be taken before we enter the main split work.

    split_huge_page
    take anon_vma lock
    __split_huge_page
    __split_huge_page_splitting
    lock page_table_lock <--- *1
    page_check_address_pmd
    unlock page_table_lock
    __split_huge_page_refcount
    lock lru_lock
    compound_lock
    mem_cgroup_split_huge_fixup
    compound_unlock
    unlock lru_lock
    __split_huge_page_map
    lock page_table_lock
    ... some work
    unlock page_table_lock
    unlock anon_vma lock

    > I'm not fully sure but IIUC, pmd_trans_huge_lock() just guarantees a huge page "map"
    > never goes out. To avoid page splitting itself, compound_lock() is required, I think.
    >
    > So, the lock here should be
    >
    > page = target.page;
    > isolate_lru_page(page);
    > flags = compound_lock_irqsave(page);

    I think the race between task migration and thp split does not happen
    because of 2 reasons:

    - when we enter the if-block, there is no concurrent thp splitting
    (note that pmd_trans_huge_lock() returns 1 only if the thp is not
    under splitting,)

    - if another thread runs into split_huge_page() just after we entered
    this if-block, the thread waits for page table lock to be unlocked
    in __split_huge_page_splitting() (shown *1 above.) At this point,
    the thp has not been split yet.

    But I think it's OK to add compound_lock to meet the requisition of
    mem_cgroup_move_account().

    >
    >
    > > + if (!mem_cgroup_move_account(page, HPAGE_PMD_NR,
    > > + pc, mc.from, mc.to,
    > > + false)) {
    > > + mc.precharge -= HPAGE_PMD_NR;
    > > + mc.moved_charge += HPAGE_PMD_NR;
    > > + }
    >
    > Here is PageTransHuge() is checked in mem_cgroup_move_account() and if !PageTransHuge(),
    > the function returns -EBUSY.

    If the above explanation is correct, PageTransHuge() should always be
    true here, so BUG_ON(!PageTransHuge()) looks suitable for me.

    > I'm not sure but....it's not worth to retry (but add a comment as FIXME later!)

    I agree.
    For regular size pages, retrying means that we run out of mc.precharge
    before addr reaches to end.
    But mem_cgroup_move_charge_pte_range() runs over a pmd in a single call and
    addr reaches to end only one call of mem_cgroup_move_account() for thp.
    So it makes no sense to retry.

    > compound_unlock_irqrestore(page);
    >
    > I may miss something, please check carefully, again.

    OK.

    Thanks,
    Naoya


    \
     
     \ /
      Last update: 2012-02-29 10:53    [W:0.027 / U:0.180 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site