Messages in this thread | | | Date | Sat, 7 Nov 2009 03:44:35 +0900 (JST) | Subject | Re: [PATCH 2/2] memcg : rewrite percpu countings with new interfaces | From | "KAMEZAWA Hiroyuki" <> |
| |
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
| |