| Message ID | 20220530074919.46352-9-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Use obj_cgroup APIs to charge the LRU pages | expand |
On Mon, May 30, 2022 at 03:49:16PM +0800, Muchun Song wrote: > In the previous patch, we know how to make the lruvec lock safe when LRU > pages are reparented. We should do something like following. > > memcg_reparent_objcgs(memcg) > 1) lock > // lruvec belongs to memcg and lruvec_parent belongs to parent memcg. > spin_lock(&lruvec->lru_lock); > spin_lock(&lruvec_parent->lru_lock); > > 2) relocate from current memcg to its parent > // Move all the pages from the lruvec list to the parent lruvec list. > > 3) unlock > spin_unlock(&lruvec_parent->lru_lock); > spin_unlock(&lruvec->lru_lock); > > Apart from the page lruvec lock, the deferred split queue lock (THP only) > also needs to do something similar. So we extract the necessary three steps > in the memcg_reparent_objcgs(). > > memcg_reparent_objcgs(memcg) > 1) lock > memcg_reparent_ops->lock(memcg, parent); > > 2) relocate > memcg_reparent_ops->relocate(memcg, reparent); > > 3) unlock > memcg_reparent_ops->unlock(memcg, reparent); > > Now there are two different locks (e.g. lruvec lock and deferred split > queue lock) need to use this infrastructure. In the next patch, we will > use those APIs to make those locks safe when the LRU pages reparented. > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> I've mixed feelings about this: it looks nice, but maybe too nice. I wonder if it's better to open-code it. Not very confident, I wonder what others are thinking. 1) Because the lock callback is first called for all ops, then relocate, then unlock, implicit lock dependencies are created. Now it depends on the order of elements in the memcg_reparent_ops array, which isn't very obvious. 2) Unlikely there will be a lot of new ops added in the future. The code looks correct though. Thanks!
On Sun, Jun 19, 2022 at 12:47:39PM -0700, Roman Gushchin wrote: > On Mon, May 30, 2022 at 03:49:16PM +0800, Muchun Song wrote: > > In the previous patch, we know how to make the lruvec lock safe when LRU > > pages are reparented. We should do something like following. > > > > memcg_reparent_objcgs(memcg) > > 1) lock > > // lruvec belongs to memcg and lruvec_parent belongs to parent memcg. > > spin_lock(&lruvec->lru_lock); > > spin_lock(&lruvec_parent->lru_lock); > > > > 2) relocate from current memcg to its parent > > // Move all the pages from the lruvec list to the parent lruvec list. > > > > 3) unlock > > spin_unlock(&lruvec_parent->lru_lock); > > spin_unlock(&lruvec->lru_lock); > > > > Apart from the page lruvec lock, the deferred split queue lock (THP only) > > also needs to do something similar. So we extract the necessary three steps > > in the memcg_reparent_objcgs(). > > > > memcg_reparent_objcgs(memcg) > > 1) lock > > memcg_reparent_ops->lock(memcg, parent); > > > > 2) relocate > > memcg_reparent_ops->relocate(memcg, reparent); > > > > 3) unlock > > memcg_reparent_ops->unlock(memcg, reparent); > > > > Now there are two different locks (e.g. lruvec lock and deferred split > > queue lock) need to use this infrastructure. In the next patch, we will > > use those APIs to make those locks safe when the LRU pages reparented. > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > I've mixed feelings about this: it looks nice, but maybe too nice. I wonder > if it's better to open-code it. Not very confident, I wonder what others are > thinking. > I also thought about this. Open-code is not simplified than this since memcg_reparent_ops can be used for 3 locks which simplifies code a lot. I also want to hear others' thoughts on this. > 1) Because the lock callback is first called for all ops, then relocate, then > unlock, implicit lock dependencies are created. Now it depends on the order > of elements in the memcg_reparent_ops array, which isn't very obvious. Maybe we can add some comments explaining the lock dependency depends on the element order in array. > 2) Unlikely there will be a lot of new ops added in the future. > Yep. I think so. Thanks. > The code looks correct though. > > Thanks! >
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 16464116f94a..c2ac98a0ece4 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -347,6 +347,26 @@ struct mem_cgroup { struct mem_cgroup_per_node *nodeinfo[]; }; +struct memcg_reparent_ops { + /* + * Note that interrupt is disabled before calling those callbacks, + * so the interrupt should remain disabled when leaving those callbacks. + */ + void (*lock)(struct mem_cgroup *src, struct mem_cgroup *dst); + void (*relocate)(struct mem_cgroup *src, struct mem_cgroup *dst); + void (*unlock)(struct mem_cgroup *src, struct mem_cgroup *dst); +}; + +#define DEFINE_MEMCG_REPARENT_OPS(name) \ + const struct memcg_reparent_ops memcg_##name##_reparent_ops = { \ + .lock = name##_reparent_lock, \ + .relocate = name##_reparent_relocate, \ + .unlock = name##_reparent_unlock, \ + } + +#define DECLARE_MEMCG_REPARENT_OPS(name) \ + extern const struct memcg_reparent_ops memcg_##name##_reparent_ops + /* * size of first charge trial. "32" comes from vmscan.c's magic value. * TODO: maybe necessary to use big numbers in big irons. diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 4cc392741753..059188eeb80c 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -337,24 +337,60 @@ static struct obj_cgroup *obj_cgroup_alloc(void) return objcg; } -static void memcg_reparent_objcgs(struct mem_cgroup *memcg) +static void objcg_reparent_lock(struct mem_cgroup *src, struct mem_cgroup *dst) +{ + spin_lock(&objcg_lock); +} + +static void objcg_reparent_relocate(struct mem_cgroup *src, struct mem_cgroup *dst) { struct obj_cgroup *objcg, *iter; - struct mem_cgroup *parent = parent_mem_cgroup(memcg); - objcg = rcu_replace_pointer(memcg->objcg, NULL, true); + objcg = rcu_replace_pointer(src->objcg, NULL, true); + /* 1) Ready to reparent active objcg. */ + list_add(&objcg->list, &src->objcg_list); + /* 2) Reparent active objcg and already reparented objcgs to dst. */ + list_for_each_entry(iter, &src->objcg_list, list) + WRITE_ONCE(iter->memcg, dst); + /* 3) Move already reparented objcgs to the dst's list */ + list_splice(&src->objcg_list, &dst->objcg_list); +} + +static void objcg_reparent_unlock(struct mem_cgroup *src, struct mem_cgroup *dst) +{ + spin_unlock(&objcg_lock); +} - spin_lock_irq(&objcg_lock); +static DEFINE_MEMCG_REPARENT_OPS(objcg); - /* 1) Ready to reparent active objcg. */ - list_add(&objcg->list, &memcg->objcg_list); - /* 2) Reparent active objcg and already reparented objcgs to parent. */ - list_for_each_entry(iter, &memcg->objcg_list, list) - WRITE_ONCE(iter->memcg, parent); - /* 3) Move already reparented objcgs to the parent's list */ - list_splice(&memcg->objcg_list, &parent->objcg_list); - - spin_unlock_irq(&objcg_lock); +static const struct memcg_reparent_ops *memcg_reparent_ops[] = { + &memcg_objcg_reparent_ops, +}; + +#define DEFINE_MEMCG_REPARENT_FUNC(phase) \ + static void memcg_reparent_##phase(struct mem_cgroup *src, \ + struct mem_cgroup *dst) \ + { \ + int i; \ + \ + for (i = 0; i < ARRAY_SIZE(memcg_reparent_ops); i++) \ + memcg_reparent_ops[i]->phase(src, dst); \ + } + +DEFINE_MEMCG_REPARENT_FUNC(lock) +DEFINE_MEMCG_REPARENT_FUNC(relocate) +DEFINE_MEMCG_REPARENT_FUNC(unlock) + +static void memcg_reparent_objcgs(struct mem_cgroup *src) +{ + struct mem_cgroup *dst = parent_mem_cgroup(src); + struct obj_cgroup *objcg = rcu_dereference_protected(src->objcg, true); + + local_irq_disable(); + memcg_reparent_lock(src, dst); + memcg_reparent_relocate(src, dst); + memcg_reparent_unlock(src, dst); + local_irq_enable(); percpu_ref_kill(&objcg->refcnt); }
In the previous patch, we know how to make the lruvec lock safe when LRU pages are reparented. We should do something like following. memcg_reparent_objcgs(memcg) 1) lock // lruvec belongs to memcg and lruvec_parent belongs to parent memcg. spin_lock(&lruvec->lru_lock); spin_lock(&lruvec_parent->lru_lock); 2) relocate from current memcg to its parent // Move all the pages from the lruvec list to the parent lruvec list. 3) unlock spin_unlock(&lruvec_parent->lru_lock); spin_unlock(&lruvec->lru_lock); Apart from the page lruvec lock, the deferred split queue lock (THP only) also needs to do something similar. So we extract the necessary three steps in the memcg_reparent_objcgs(). memcg_reparent_objcgs(memcg) 1) lock memcg_reparent_ops->lock(memcg, parent); 2) relocate memcg_reparent_ops->relocate(memcg, reparent); 3) unlock memcg_reparent_ops->unlock(memcg, reparent); Now there are two different locks (e.g. lruvec lock and deferred split queue lock) need to use this infrastructure. In the next patch, we will use those APIs to make those locks safe when the LRU pages reparented. Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- include/linux/memcontrol.h | 20 +++++++++++++++ mm/memcontrol.c | 62 ++++++++++++++++++++++++++++++++++++---------- 2 files changed, 69 insertions(+), 13 deletions(-)