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    [from the cache]
©2003-2011 Jasper Spaans