Re: [PATCH 2/4] [BUGFIX] dont set USED bit on tail pages

[Hiroyuki Kamezawa <kamezawa.hiroyuki@gmail.com>]

Thank you for review.

2011/1/14 Johannes Weiner <hannes@cmpxchg.org>:
> On Fri, Jan 14, 2011 at 07:09:09PM +0900, KAMEZAWA Hiroyuki wrote:
>> --- mmotm-0107.orig/mm/memcontrol.c
>> +++ mmotm-0107/mm/memcontrol.c
>
>> @@ -2154,6 +2139,23 @@ static void __mem_cgroup_commit_charge(s
>>        */
>>       memcg_check_events(mem, pc->page);
>>  }
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +/*
>> + * Because tail pages are not mared as "used", set it. We're under
>
> marked
>
will fix.

>> + * compund_lock and don't need to take care of races.
>> + * Statistics are updated properly at charging. We just mark Used bits.
>> + */
>> +void mem_cgroup_split_huge_fixup(struct page *head, struct page *tail)
>> +{
>> +     struct page_cgroup *hpc = lookup_page_cgroup(head);
>> +     struct page_cgroup *tpc = lookup_page_cgroup(tail);
>
> I have trouble reading the code fluently with those names as they are
> just very similar random letter sequences.  Could you rename them so
> that they're better to discriminate?  headpc and tailpc perhaps?
>
ok. I'll use headpc,tailpc.

>> +     tpc->mem_cgroup = hpc->mem_cgroup;
>> +     smp_wmb(); /* see __commit_charge() */
>> +     SetPageCgroupUsed(tpc);
>> +     VM_BUG_ON(PageCgroupCache(hpc));
>
> Right now, this would be a bug due to other circumstances, but this
> function does not require the page to be anon to function correctly,
> does it?

No.

>  I don't think we should encode a made up dependency here.
>
Ok, I'll remove BUG_ON.


>> @@ -2602,8 +2603,7 @@ __mem_cgroup_uncharge_common(struct page
>>               break;
>>       }
>>
>> -     for (i = 0; i < count; i++)
>> -             mem_cgroup_charge_statistics(mem, file, -1);
>> +     mem_cgroup_charge_statistics(mem, file, -count);
>
> Pass PageCgroupCache(pc) instead, ditch the `file' variable?
>
will do.

>>       ClearPageCgroupUsed(pc);
>>       /*
>> Index: mmotm-0107/include/linux/memcontrol.h
>> ===================================================================
>> --- mmotm-0107.orig/include/linux/memcontrol.h
>> +++ mmotm-0107/include/linux/memcontrol.h
>> @@ -146,6 +146,10 @@ unsigned long mem_cgroup_soft_limit_recl
>>                                               gfp_t gfp_mask);
>>  u64 mem_cgroup_get_limit(struct mem_cgroup *mem);
>>
>> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> +void mem_cgroup_split_huge_fixup(struct page *head, struct page *tail);
>> +#endif
>> +
>>  #else /* CONFIG_CGROUP_MEM_RES_CTLR */
>>  struct mem_cgroup;
>>
>> Index: mmotm-0107/mm/huge_memory.c
>> ===================================================================
>> --- mmotm-0107.orig/mm/huge_memory.c
>> +++ mmotm-0107/mm/huge_memory.c
>> @@ -1203,6 +1203,8 @@ static void __split_huge_page_refcount(s
>>               BUG_ON(!PageDirty(page_tail));
>>               BUG_ON(!PageSwapBacked(page_tail));
>>
>> +             mem_cgroup_split_huge_fixup(page, page_tail);
>
> You need to provide a dummy for non-memcg configurations.

Ahhh, yes. I'll add one.

Thanks,
-Kame