Re: [BUGFIX][PATCH mmotm] memcg fix swap accounting leak (v2)

["KAMEZAWA Hiroyuki" <kamezawa.hiroyu@jp.fujitsu.com>]

Hugh Dickins said:
> On Sat, 13 Dec 2008, KAMEZAWA Hiroyuki wrote:
>> --- mmotm-2.6.28-Dec12.orig/mm/memory.c
>> +++ mmotm-2.6.28-Dec12/mm/memory.c
>>
>> -	mem_cgroup_commit_charge_swapin(page, ptr);
>>  	inc_mm_counter(mm, anon_rss);
>>  	pte = mk_pte(page, vma->vm_page_prot);
>>  	if (write_access && reuse_swap_page(page)) {
>>  		pte = maybe_mkwrite(pte_mkdirty(pte), vma);
>>  		write_access = 0;
>>  	}
>> -
>>  	flush_icache_page(vma, page);
>>  	set_pte_at(mm, address, page_table, pte);
>>  	page_add_anon_rmap(page, vma, address);
>> +	/* It's better to call commit-charge after rmap is established */
>> +	mem_cgroup_commit_charge_swapin(page, ptr);
>>
>>  	swap_free(entry);
>>  	if (vm_swap_full() || (vma->vm_flags & VM_LOCKED) ||
>> PageMlocked(page))
>
> That ordering is back to how it was before I adjusted it
> for reuse_swap_page()'s delete_from_swap_cache(), isn't it?
>
> So I don't understand how you've fixed the bug I hit (not an
> accounting imbalance but an oops or BUG, I forget) with this
> ordering, without making some other change elsewhere.
>
Ah, this is for fixing the bug by this order of calls.
==


> mem_cgroup_commit_charge_swapin calls swap_cgroup_record with
> bogus swp_entry_t 0, which appears to belong to swp_offset 0 of
> swp_type 0, but the ctrl->map for type 0 may have been freed
> ages ago (we do always start from 0, but maybe we swapped on
> type 1 and swapped off type 0 meanwhile).  I'm guessing that
> by looking at the code, not by retesting it, so I may have the
> details wrong; but I didn't reorder your code just for fun.
>
Ah, sorry. commit_charge_swapin() should chekc the page is still
SwapCache. Sorry. I'll update this in Monday.

> Perhaps your restored ordering works if you check PageSwapCache
> in mem_cgroup_commit_charge_swapin or check 0 in swap_cgroup_record,
> but I don't see that in yesterday's mmotm, nor in this patch.
>
yes. I'm wrong at that point.
I'll add PageSwapCache check to "commit" ops.

> (And I should admit, I've not even attempted to follow your
> accounting justification: I'll leave that to you memcg guys.)
>
> An alternative could be not to clear page->private when deleting
> from swap cache, that's only done for tidiness and to force notice
> of races like this; but I'd want a much stronger reason to change that.
>
Hmm, doesn't that change will add new unnecessary complex ?

> Or am I making this up?  As I say, I've not tested it this time around.
>
I'll revisit this Monday and think of swp_entry==0 problem.

Thank you for pointing out.

-Kame

> Hugh
>