linux-kernel - Re: [RFC][PATCH] memcg: avoid THP split in task migration

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 29 Feb 2012 04:50:20 -0500
From:	Naoya Horiguchi <n-horiguchi@...jp.nec.com>
To:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Cc:	Naoya Horiguchi <n-horiguchi@...jp.nec.com>, linux-mm@...ck.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Daisuke Nishimura <nishimura@....nes.nec.co.jp>,
	Hillf Danton <dhillf@...il.com>, linux-kernel@...r.kernel.org
Subject: Re: [RFC][PATCH] memcg: avoid THP split in task migration

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@...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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/