(2012/04/19 8:33), Andrew Morton wrote:
> On Wed, 18 Apr 2012 11:21:55 -0700
> Ying Han <yinghan@xxxxxxxxxx> wrote:
>
>> We have the nr_mlock stat both in meminfo as well as vmstat system wide, this
>> patch adds the mlock field into per-memcg memory stat. The stat itself enhances
>> the metrics exported by memcg since the unevictable lru includes more than
>> mlock()'d page like SHM_LOCK'd.
>>
>> Why we need to count mlock'd pages while they are unevictable and we can not
>> do much on them anyway?
>>
>> This is true. The mlock stat I am proposing is more helpful for system admin
>> and kernel developer to understand the system workload. The same information
>> should be helpful to add into OOM log as well. Many times in the past that we
>> need to read the mlock stat from the per-container meminfo for different
>> reason. Afterall, we do have the ability to read the mlock from meminfo and
>> this patch fills the info in memcg.
>>
>>
>> ...
>>
>> static inline int is_mlocked_vma(struct vm_area_struct *vma, struct page *page)
>> {
>> + bool locked;
>> + unsigned long flags;
>> +
>> VM_BUG_ON(PageLRU(page));
>>
>> if (likely((vma->vm_flags & (VM_LOCKED | VM_SPECIAL)) != VM_LOCKED))
>> return 0;
>>
>> + mem_cgroup_begin_update_page_stat(page, &locked, &flags);
>> if (!TestSetPageMlocked(page)) {
>> inc_zone_page_state(page, NR_MLOCK);
>> + mem_cgroup_inc_page_stat(page, MEMCG_NR_MLOCK);
>> count_vm_event(UNEVICTABLE_PGMLOCKED);
>> }
>> + mem_cgroup_end_update_page_stat(page, &locked, &flags);
>> +
>> return 1;
>> }
>
> Unrelated to this patch: is_mlocked_vma() is misnamed. A function with
> that name should be a bool-returning test which has no side-effects.
>
>>
>> ...
>>
>> static void __free_pages_ok(struct page *page, unsigned int order)
>> {
>> unsigned long flags;
>> - int wasMlocked = __TestClearPageMlocked(page);
>> + bool locked;
>>
>> if (!free_pages_prepare(page, order))
>> return;
>>
>> local_irq_save(flags);
>> - if (unlikely(wasMlocked))
>> + mem_cgroup_begin_update_page_stat(page, &locked, &flags);
>
> hm, what's going on here. The page now has a zero refcount and is to
> be returned to the buddy. But mem_cgroup_begin_update_page_stat()
> assumes that the page still belongs to a memcg. I'd have thought that
> any page_cgroup backreferences would have been torn down by now?
>
>> + if (unlikely(__TestClearPageMlocked(page)))
>> free_page_mlock(page);
>
Ah, this is problem. Now, we have following code.
==
> struct lruvec *mem_cgroup_lru_add_list(struct zone *zone, struct page *page,
> enum lru_list lru)
> {
> struct mem_cgroup_per_zone *mz;
> struct mem_cgroup *memcg;
> struct page_cgroup *pc;
>
> if (mem_cgroup_disabled())
> return &zone->lruvec;
>
> pc = lookup_page_cgroup(page);
> memcg = pc->mem_cgroup;
>
> /*
> * Surreptitiously switch any uncharged page to root:
> * an uncharged page off lru does nothing to secure
> * its former mem_cgroup from sudden removal.
> *
> * Our caller holds lru_lock, and PageCgroupUsed is updated
> * under page_cgroup lock: between them, they make all uses
> * of pc->mem_cgroup safe.
> */
> if (!PageCgroupUsed(pc) && memcg != root_mem_cgroup)
> pc->mem_cgroup = memcg = root_mem_cgroup;
==
Then, accessing pc->mem_cgroup without checking PCG_USED bit is dangerous.
It may trigger #GP because of suddern removal of memcg or because of above
code, mis-accounting will happen... pc->mem_cgroup may be overwritten already.
Proposal from me is calling TestClearPageMlocked(page) via mem_cgroup_uncharge().
Like this.
==
mem_cgroup_charge_statistics(memcg, anon, -nr_pages);
/*
* Pages reach here when it's fully unmapped or dropped from file cache.
* we are under lock_page_cgroup() and have no race with memcg activities.
*/
if (unlikely(PageMlocked(page))) {
if (TestClearPageMlocked())
decrement counter.
}
ClearPageCgroupUsed(pc);
==
But please check performance impact...
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>