memcg: dirty pages accounting and limiting infrastructure
in [Kernel]
|
Prev: [PATCH -tip 5/9] perf probe: Use elfutils-libdw for analyzing debuginfo
Next: [announce] gujin GPL bootloader version 2.8
From: Andrea Righi on 22 Feb 2010 13:10 On Mon, Feb 22, 2010 at 09:22:42AM +0900, KAMEZAWA Hiroyuki wrote: [snip] > > +static unsigned long get_dirty_bytes(struct mem_cgroup *memcg) > > +{ > > + struct cgroup *cgrp = memcg->css.cgroup; > > + unsigned long dirty_bytes; > > + > > + /* root ? */ > > + if (cgrp->parent == NULL) > > + return vm_dirty_bytes; > > We have mem_cgroup_is_root() macro. > > > + > > + spin_lock(&memcg->reclaim_param_lock); > > + dirty_bytes = memcg->dirty_bytes; > > + spin_unlock(&memcg->reclaim_param_lock); > > + > > + return dirty_bytes; > > +} > Hmm...do we need spinlock ? You use "unsigned long", then, read-write > is always atomic if not read-modify-write. I think I simply copy&paste the memcg->swappiness case. But I agree, read-write should be atomic. -Andrea -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: David Rientjes on 22 Feb 2010 16:30 On Mon, 22 Feb 2010, Andrea Righi wrote: > > Hmm...do we need spinlock ? You use "unsigned long", then, read-write > > is always atomic if not read-modify-write. > > I think I simply copy&paste the memcg->swappiness case. But I agree, > read-write should be atomic. > We don't need memcg->reclaim_param_lock in get_swappiness() or mem_cgroup_get_reclaim_priority(). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andrea Righi on 23 Feb 2010 04:30 On Mon, Feb 22, 2010 at 09:44:42PM +0530, Balbir Singh wrote: [snip] > > +void mem_cgroup_charge_dirty(struct page *page, > > + enum zone_stat_item idx, int charge) > > +{ > > + struct mem_cgroup *mem; > > + struct mem_cgroup_stat_cpu *cpustat; > > + unsigned long flags; > > + int cpu; > > + > > + if (mem_cgroup_disabled()) > > + return; > > + /* Translate the zone_stat_item into a mem_cgroup_stat_index */ > > + switch (idx) { > > + case NR_FILE_DIRTY: > > + idx = MEM_CGROUP_STAT_FILE_DIRTY; > > + break; > > + case NR_WRITEBACK: > > + idx = MEM_CGROUP_STAT_WRITEBACK; > > + break; > > + case NR_WRITEBACK_TEMP: > > + idx = MEM_CGROUP_STAT_WRITEBACK_TEMP; > > + break; > > + case NR_UNSTABLE_NFS: > > + idx = MEM_CGROUP_STAT_UNSTABLE_NFS; > > + break; > > + default: > > + return; > > + } > > + /* Charge the memory cgroup statistics */ > > + mem = get_mem_cgroup_from_page(page); > > + if (!mem) { > > + mem = root_mem_cgroup; > > + css_get(&mem->css); > > + } > > + > > + local_irq_save(flags); > > + cpu = get_cpu(); > > Kamezawa is in the process of changing these, so you might want to > look at and integrate with those patches when they are ready. OK, I'll rebase the patch to -mm. Are those changes already included in mmotm? Thanks, -Andrea -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andrea Righi on 23 Feb 2010 05:00 On Mon, Feb 22, 2010 at 02:31:13PM -0500, Vivek Goyal wrote: > On Mon, Feb 22, 2010 at 09:22:42AM +0900, KAMEZAWA Hiroyuki wrote: > > [..] > > > +static int mem_cgroup_dirty_bytes_write(struct cgroup *cgrp, struct cftype *cft, > > > + u64 val) > > > +{ > > > + struct mem_cgroup *memcg = mem_cgroup_from_cont(cgrp); > > > + struct mem_cgroup *parent; > > > + > > > + if (cgrp->parent == NULL) > > > + return -EINVAL; > > > + > > > + parent = mem_cgroup_from_cont(cgrp->parent); > > > + > > > + cgroup_lock(); > > > + > > > + /* If under hierarchy, only empty-root can set this value */ > > > + if ((parent->use_hierarchy) || > > > + (memcg->use_hierarchy && !list_empty(&cgrp->children))) { > > > + cgroup_unlock(); > > > + return -EINVAL; > > > + } > > > > Okay, then, only hierarchy root can set the value. > > Please descirbe this kind of important things in patch description. > > > > Hi Andrea, > > Why can only root of the hierarchy set set dirty_bytes value? In this > case, a child cgroup's amount of dirty pages will be controlled by > dirty_ratio? I'm rewriting the patch to correctly handle hierarchy. The hierarchy design is completely broken in this patch. -Andrea -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Andrea Righi on 23 Feb 2010 07:00
On Mon, Feb 22, 2010 at 01:07:32PM -0500, Vivek Goyal wrote: > > > > +unsigned long mem_cgroup_dirty_bytes(void) > > > > +{ > > > > + struct mem_cgroup *memcg; > > > > + unsigned long dirty_bytes; > > > > + > > > > + if (mem_cgroup_disabled()) > > > > + return vm_dirty_bytes; > > > > + > > > > + rcu_read_lock(); > > > > + memcg = mem_cgroup_from_task(current); > > > > + if (memcg == NULL) > > > > + dirty_bytes = vm_dirty_bytes; > > > > + else > > > > + dirty_bytes = get_dirty_bytes(memcg); > > > > + rcu_read_unlock(); > > > > > > The rcu_read_lock() isn't protecting anything here. > > > > Right! > > Are we not protecting "memcg" pointer using rcu here? Vivek, you are right: mem_cgroup_from_task() -> task_subsys_state() -> rcu_dereference() So, this *must* be RCU protected. Thanks! -Andrea -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |