2011/1/14 Johannes Weiner <hannes@xxxxxxxxxxx>:
> On Fri, Jan 14, 2011 at 07:06:44PM +0900, KAMEZAWA Hiroyuki wrote:
>> mem_cgroup_charge_staistics() was designed for charging a page but
>> now, we have transparent hugepage. To fix problems (in following patch)
>> it's required to change the function to get the number of pages
>> as its arguments.
>>
>> The new function gets following as argument.
>> - type of page rather than 'pc'
>> - size of page which is accounted.
>>
>> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
>
> I agree with the patch in general, below are only a few nitpicks.
>
Thanks, I think details should be updated, too.
>> --- mmotm-0107.orig/mm/memcontrol.c
>> +++ mmotm-0107/mm/memcontrol.c
>> @@ -600,23 +600,23 @@ static void mem_cgroup_swap_statistics(s
>> }
>>
>> static void mem_cgroup_charge_statistics(struct mem_cgroup *mem,
>> - struct page_cgroup *pc,
>> - bool charge)
>> + bool file,
>> + int pages)
>
> I think 'nr_pages' would be a better name. This makes me think of a
> 'struct page *[]'.
>
ok.
>> {
>> - int val = (charge) ? 1 : -1;
>> -
>> preempt_disable();
>>
>> - if (PageCgroupCache(pc))
>> - __this_cpu_add(mem->stat->count[MEM_CGROUP_STAT_CACHE], val);
>> + if (file)
>> + __this_cpu_add(mem->stat->count[MEM_CGROUP_STAT_CACHE], pages);
>> else
>> - __this_cpu_add(mem->stat->count[MEM_CGROUP_STAT_RSS], val);
>> + __this_cpu_add(mem->stat->count[MEM_CGROUP_STAT_RSS], pages);
>>
>> - if (charge)
>> + /* pagein of a big page is an event. So, ignore page size */
>> + if (pages > 0)
>> __this_cpu_inc(mem->stat->count[MEM_CGROUP_STAT_PGPGIN_COUNT]);
>> else
>> __this_cpu_inc(mem->stat->count[MEM_CGROUP_STAT_PGPGOUT_COUNT]);
>> - __this_cpu_inc(mem->stat->count[MEM_CGROUP_EVENTS]);
>> +
>> + __this_cpu_add(mem->stat->count[MEM_CGROUP_EVENTS], pages);
>>
>> preempt_enable();
>> }
>> @@ -2092,6 +2092,7 @@ static void ____mem_cgroup_commit_charge
>> struct page_cgroup *pc,
>> enum charge_type ctype)
>> {
>> + bool file = false;
>> pc->mem_cgroup = mem;
>> /*
>> * We access a page_cgroup asynchronously without lock_page_cgroup().
>> @@ -2106,6 +2107,7 @@ static void ____mem_cgroup_commit_charge
>> case MEM_CGROUP_CHARGE_TYPE_SHMEM:
>> SetPageCgroupCache(pc);
>> SetPageCgroupUsed(pc);
>> + file = true;
>> break;
>> case MEM_CGROUP_CHARGE_TYPE_MAPPED:
>> ClearPageCgroupCache(pc);
>> @@ -2115,7 +2117,7 @@ static void ____mem_cgroup_commit_charge
>> break;
>> }
>>
>> - mem_cgroup_charge_statistics(mem, pc, true);
>> + mem_cgroup_charge_statistics(mem, file, 1);
>
> The extra local variable is a bit awkward, since there are already
> several sources of this information (ctype and pc->flags).
>
> Could you keep it like the other sites, just pass PageCgroupCache()
> here as well?
>
Ok.
>> @@ -2186,14 +2188,14 @@ static void __mem_cgroup_move_account(st
>> __this_cpu_inc(to->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
>> preempt_enable();
>> }
>> - mem_cgroup_charge_statistics(from, pc, false);
>> + mem_cgroup_charge_statistics(from, PageCgroupCache(pc), -1);
>> if (uncharge)
>> /* This is not "cancel", but cancel_charge does all we need. */
>> mem_cgroup_cancel_charge(from, PAGE_SIZE);
>>
>> /* caller should have done css_get */
>> pc->mem_cgroup = to;
>> - mem_cgroup_charge_statistics(to, pc, true);
>> + mem_cgroup_charge_statistics(to, PageCgroupCache(pc), 1);
>> /*
>> * We charges against "to" which may not have any tasks. Then, "to"
>> * can be under rmdir(). But in current implementation, caller of
>> @@ -2551,6 +2553,7 @@ __mem_cgroup_uncharge_common(struct page
>> struct page_cgroup *pc;
>> struct mem_cgroup *mem = NULL;
>> int page_size = PAGE_SIZE;
>> + bool file = false;
>>
>> if (mem_cgroup_disabled())
>> return NULL;
>> @@ -2578,6 +2581,9 @@ __mem_cgroup_uncharge_common(struct page
>> if (!PageCgroupUsed(pc))
>> goto unlock_out;
>>
>> + if (PageCgroupCache(pc))
>> + file = true;
>> +
>> switch (ctype) {
>> case MEM_CGROUP_CHARGE_TYPE_MAPPED:
>> case MEM_CGROUP_CHARGE_TYPE_DROP:
>> @@ -2597,7 +2603,7 @@ __mem_cgroup_uncharge_common(struct page
>> }
>>
>> for (i = 0; i < count; i++)
>> - mem_cgroup_charge_statistics(mem, pc + i, false);
>> + mem_cgroup_charge_statistics(mem, file, -1);
>
> I see you get rid of this loop in the next patch, anyway. Can you
> just use PageCgroupCache() instead of the extra variable?
will do.
Thanks,
-Kame
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href