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

[KAMEZAWA Hiroyuki]

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 a fix for the new bug by this order.
==
    try_charge()
    commit_charge()
    reuse_swap_page()
         -> delete_from_swapcache() -> uncharge_swapcache().
    increase mapcount here.
==
Because ucharge_swapcache() assumes following
  a. if mapcount==0, this swap cache is of no use and will be discarded.
  b. if mapcount >0, this swap cache is in use.
A charge commited by commit_charge() is discarded by reuse_swap_page().

By delaying commit (means checking flag of page_cgroup).
==
  try_charge()
  reuse_swap_page()
  commit_charge()
==
the leak of charge doesn't happen.
(reuse_swap_page() may drop page from swap-cache, but it's no probelm to
 commit. But as you say, this has swp_entry==0 bug.)

> 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.
>
> 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.
>
Ahhhh, sorry. ok, swp_entry==0 is valid...Sigh...
I'll revisit this and check how commit_charge() works.
I think checking PageSwapCache() is enough but if not, do somehing other.
(Maybe Nishimura's suggestion to pass swp_entry directly to commit_charge()
 is one way.)

> (And I should admit, I've not even attempted to follow your
> accounting justification: I'll leave that to you memcg guys.)
>
Sorry for complication ;(

> 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.
>
It seems that it  will add another complex or unexpected behavior..
I think I can do something workaround.

> Or am I making this up?  As I say, I've not tested it this time around.
>
I'll ask you if I found I can't do anything ;(

Thank you for pointing out!
I'll revisit this on Monday.

-Kame



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