(2012/04/20 14:57), Ying Han wrote:
> On Thu, Apr 19, 2012 at 5:37 PM, KAMEZAWA Hiroyuki
> <kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
>> (2012/04/19 22:12), Johannes Weiner wrote:
>>> Plus this code runs for ALL uncharges, the unlikely() and preliminary
>>> flag testing don't make it okay. It's bad that we have this in the
>>> allocator, but at least it would be good to hook into that branch and
>>> not add another one.
>>>
>>> pc->mem_cgroup stays intact after the uncharge. Could we make the
>>> memcg removal path wait on the mlock counter to drop to zero instead
>>> and otherwise keep Ying's version?
>>>
>>
>>
>> handling problem in ->destroy() path ? Hmm, it will work against use-after-free.
>
>> But accounting problem which may be caused by mem_cgroup_lru_add_list() cannot
>> be handled, which overwrites pc->mem_cgroup.
>
> Kame, can you clarify that? What the mem_cgroup_lru_add_list() has
> anything to do w/ this problem?
>
It overwrites pc->mem_cgroup. Then, Assume a task in cgroup "A".
1. page is charged. pc->mem_cgroup = A + Used bit.
2. page is set Mlocked. A's mlock-counter += 1
3. page is uncharged - Used bit.
4. page is added to lru pc->mem_cgroup = root
5. page is freed root's mlock-coutner -=1,
Then, A's mlock-counter +1, root's mlock-counter -1 IF free_pages()
really handle mlocked pages...
>>
>> But hm, is this too slow ?...
>> ==
>> mem_cgroup_uncharge_common()
>> {
>> ....
>> if (PageSwapCache(page) || PageMlocked(page))
>> return NULL;
>> }
>>
>> page_alloc.c::
>>
>> static inline void free_page_mlock(struct page *page)
>> {
>>
>> __dec_zone_page_state(page, NR_MLOCK);
>> __count_vm_event(UNEVICTABLE_MLOCKFREED);
>>
>> mem_cgroup_uncharge_page(page);
>> }
>> ==
>>
>> BTW, at reading code briefly....why we have hooks in free_page() ?
>>
>> It seems do_munmap() and exit_mmap() calls munlock_vma_pages_all().
>> So, it seems all vmas which has VM_MLOCKED are checked before freeing.
>> vmscan never frees mlocked pages, I think.
>>
>> Any other path to free mlocked pages without munlock ?
>
> I found this commit which introduced the hook in the freeing path,
> however I couldn't get more details why it was introduced from the
> commit description
>
> commit 985737cf2ea096ea946aed82c7484d40defc71a8
> Author: Lee Schermerhorn <lee.schermerhorn@xxxxxx>
> Date: Sat Oct 18 20:26:53 2008 -0700
>
> mlock: count attempts to free mlocked page
>
> Allow free of mlock()ed pages. This shouldn't happen, but during
> developement, it occasionally did.
>
> This patch allows us to survive that condition, while keeping the
> statistics and events correct for debug.
>
>> I feel freeing Mlocked page is a cause of problems.
>
Sigh...."This shouldn't happen"!!!!!
How about adding warning to free_page() path and remove your current hook ?
Thanks,
-Kame
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>