lkml.org 
[lkml]   [2008]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [BUGFIX][PATCH mmotm] memcg fix swap accounting leak (v2)
From
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





\
 
 \ /
  Last update: 2008-12-13 11:41    [W:0.066 / U:0.576 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site