lkml.org 
[lkml]   [2009]   [Nov]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/2] memcg : rewrite percpu countings with new interfaces
From
Christoph Lameter wrote:
> On Fri, 6 Nov 2009, KAMEZAWA Hiroyuki wrote:
>> - __mem_cgroup_stat_reset_safe(cpustat, MEMCG_EVENTS);
>> + __this_cpu_write(mem->cpustat->count[MEMCG_EVENTS], 0);
>> ret = true;
>> }
>> - put_cpu();
>> return ret;
>
> If you want to use the __this_cpu_xx versions then you need to manage
> preempt on your own.
>
Ah, I see. I understand I haven't understood.

> You need to keep preempt_disable/enable here because otherwise the per
> cpu variable zeroed may be on a different cpu than the per cpu variable
> where you got the value from.
>
Thank you. I think I can do well in the next version.


>> +static s64 mem_cgroup_read_stat(struct mem_cgroup *mem,
>> + enum mem_cgroup_stat_index idx)
>> +{
>> + struct mem_cgroup_stat_cpu *cstat;
>> + int cpu;
>> + s64 ret = 0;
>> +
>> + for_each_possible_cpu(cpu) {
>> + cstat = per_cpu_ptr(mem->cpustat, cpu);
>> + ret += cstat->count[idx];
>> + }
>
> == ret += per_cpu(mem->cpustat->cstat->count[idx], cpu)
>
Hmm, Hmm. Will use that.

>> static void mem_cgroup_swap_statistics(struct mem_cgroup *mem,
>> bool charge)
>> {
>> int val = (charge) ? 1 : -1;
>> - struct mem_cgroup_stat *stat = &mem->stat;
>> - struct mem_cgroup_stat_cpu *cpustat;
>> - int cpu = get_cpu();
>>
>> - cpustat = &stat->cpustat[cpu];
>> - __mem_cgroup_stat_add_safe(cpustat, MEMCG_NR_SWAP, val);
>> - put_cpu();
>> + __this_cpu_add(mem->cpustat->count[MEMCG_NR_SWAP], val);
>> }
>
> You do not disable preempt on your own so you have to use
>
> this_cpu_add()
>
> There is no difference between __this_cpu_add and this_cpu_add on x86 but
> they will differ on platforms that do not have atomic per cpu
> instructions. The fallback for this_cpu_add is to protect the add with
> preempt_disable()/enable. The fallback fro __this_cpu_add is just to rely
> on the caller to ensure that preempt is disabled somehow.
>
Ok.


>> - /*
>> - * Preemption is already disabled, we don't need get_cpu()
>> - */
>> - cpu = smp_processor_id();
>> - stat = &mem->stat;
>> - cpustat = &stat->cpustat[cpu];
>> -
>> - __mem_cgroup_stat_add_safe(cpustat, MEMCG_NR_FILE_MAPPED, val);
>> + __this_cpu_add(mem->cpustat->count[MEMCG_NR_FILE_MAPPED], val);
>
> Remove __
>
>
>> @@ -1650,16 +1597,11 @@ static int mem_cgroup_move_account(struc
>>
>> page = pc->page;
>> if (page_mapped(page) && !PageAnon(page)) {
>> - cpu = smp_processor_id();
>> /* Update mapped_file data for mem_cgroup "from" */
>> - stat = &from->stat;
>> - cpustat = &stat->cpustat[cpu];
>> - __mem_cgroup_stat_add_safe(cpustat, MEMCG_NR_FILE_MAPPED, -1);
>> + __this_cpu_dec(from->cpustat->count[MEMCG_NR_FILE_MAPPED]);
>
> You can keep it here since the context already has preempt disabled it
> seems.
>
Thank you for kindly review.

Regards,
-Kame





\
 
 \ /
  Last update: 2009-11-06 19:47    [from the cache]
©2003-2011 Jasper Spaans