In the previous patch, we know how to make the lruvec lock safe when the
LRU pages 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) do reparent
// 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 extracted the necessary 3 steps
in the memcg_reparent_objcgs().
memcg_reparent_objcgs(memcg)
1) lock
memcg_reparent_ops->lock(memcg, parent);
2) reparent
memcg_reparent_ops->reparent(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 <[email protected]>
---
include/linux/memcontrol.h | 11 +++++++++++
mm/memcontrol.c | 49 ++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 228263f2c82b..b12847b0be09 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -355,6 +355,17 @@ struct mem_cgroup {
/* WARNING: nodeinfo must be the last member here */
};
+struct memcg_reparent_ops {
+ struct list_head list;
+
+ /* Irq is disabled before calling those functions. */
+ void (*lock)(struct mem_cgroup *memcg, struct mem_cgroup *parent);
+ void (*unlock)(struct mem_cgroup *memcg, struct mem_cgroup *parent);
+ void (*reparent)(struct mem_cgroup *memcg, struct mem_cgroup *parent);
+};
+
+void __init register_memcg_repatent(struct memcg_reparent_ops *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 a48403e5999c..f88fe2f06f5b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -330,6 +330,41 @@ static struct obj_cgroup *obj_cgroup_alloc(void)
return objcg;
}
+static LIST_HEAD(reparent_ops_head);
+
+static void memcg_reparent_lock(struct mem_cgroup *memcg,
+ struct mem_cgroup *parent)
+{
+ struct memcg_reparent_ops *ops;
+
+ list_for_each_entry(ops, &reparent_ops_head, list)
+ ops->lock(memcg, parent);
+}
+
+static void memcg_reparent_unlock(struct mem_cgroup *memcg,
+ struct mem_cgroup *parent)
+{
+ struct memcg_reparent_ops *ops;
+
+ list_for_each_entry(ops, &reparent_ops_head, list)
+ ops->unlock(memcg, parent);
+}
+
+static void memcg_do_reparent(struct mem_cgroup *memcg,
+ struct mem_cgroup *parent)
+{
+ struct memcg_reparent_ops *ops;
+
+ list_for_each_entry(ops, &reparent_ops_head, list)
+ ops->reparent(memcg, parent);
+}
+
+void __init register_memcg_repatent(struct memcg_reparent_ops *ops)
+{
+ BUG_ON(!ops->lock || !ops->unlock || !ops->reparent);
+ list_add(&ops->list, &reparent_ops_head);
+}
+
static void memcg_reparent_objcgs(struct mem_cgroup *memcg)
{
struct obj_cgroup *objcg, *iter;
@@ -339,9 +374,13 @@ static void memcg_reparent_objcgs(struct mem_cgroup *memcg)
if (!parent)
parent = root_mem_cgroup;
+ local_irq_disable();
+
+ memcg_reparent_lock(memcg, parent);
+
objcg = rcu_replace_pointer(memcg->objcg, NULL, true);
- spin_lock_irq(&css_set_lock);
+ spin_lock(&css_set_lock);
/* 1) Ready to reparent active objcg. */
list_add(&objcg->list, &memcg->objcg_list);
@@ -351,7 +390,13 @@ static void memcg_reparent_objcgs(struct mem_cgroup *memcg)
/* 3) Move already reparented objcgs to the parent's list */
list_splice(&memcg->objcg_list, &parent->objcg_list);
- spin_unlock_irq(&css_set_lock);
+ spin_unlock(&css_set_lock);
+
+ memcg_do_reparent(memcg, parent);
+
+ memcg_reparent_unlock(memcg, parent);
+
+ local_irq_enable();
percpu_ref_kill(&objcg->refcnt);
}
--
2.11.0
On Wed, Apr 21, 2021 at 03:00:55PM +0800, Muchun Song wrote:
> In the previous patch, we know how to make the lruvec lock safe when the
> LRU pages 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) do reparent
> // 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 extracted the necessary 3 steps
> in the memcg_reparent_objcgs().
>
> memcg_reparent_objcgs(memcg)
> 1) lock
> memcg_reparent_ops->lock(memcg, parent);
>
> 2) reparent
> memcg_reparent_ops->reparent(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 <[email protected]>
> ---
> include/linux/memcontrol.h | 11 +++++++++++
> mm/memcontrol.c | 49 ++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 228263f2c82b..b12847b0be09 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -355,6 +355,17 @@ struct mem_cgroup {
> /* WARNING: nodeinfo must be the last member here */
> };
>
> +struct memcg_reparent_ops {
> + struct list_head list;
> +
> + /* Irq is disabled before calling those functions. */
> + void (*lock)(struct mem_cgroup *memcg, struct mem_cgroup *parent);
> + void (*unlock)(struct mem_cgroup *memcg, struct mem_cgroup *parent);
> + void (*reparent)(struct mem_cgroup *memcg, struct mem_cgroup *parent);
> +};
> +
> +void __init register_memcg_repatent(struct memcg_reparent_ops *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 a48403e5999c..f88fe2f06f5b 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -330,6 +330,41 @@ static struct obj_cgroup *obj_cgroup_alloc(void)
> return objcg;
> }
>
> +static LIST_HEAD(reparent_ops_head);
Because this list is completely static, why not make a build-time initialized
array instead?
I guess it's a more canonical way of solving problems like this.
The proposed API looks good to me.
On Wed, May 26, 2021 at 1:46 AM Roman Gushchin <[email protected]> wrote:
>
> On Wed, Apr 21, 2021 at 03:00:55PM +0800, Muchun Song wrote:
> > In the previous patch, we know how to make the lruvec lock safe when the
> > LRU pages 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) do reparent
> > // 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 extracted the necessary 3 steps
> > in the memcg_reparent_objcgs().
> >
> > memcg_reparent_objcgs(memcg)
> > 1) lock
> > memcg_reparent_ops->lock(memcg, parent);
> >
> > 2) reparent
> > memcg_reparent_ops->reparent(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 <[email protected]>
> > ---
> > include/linux/memcontrol.h | 11 +++++++++++
> > mm/memcontrol.c | 49 ++++++++++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 58 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index 228263f2c82b..b12847b0be09 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -355,6 +355,17 @@ struct mem_cgroup {
> > /* WARNING: nodeinfo must be the last member here */
> > };
> >
> > +struct memcg_reparent_ops {
> > + struct list_head list;
> > +
> > + /* Irq is disabled before calling those functions. */
> > + void (*lock)(struct mem_cgroup *memcg, struct mem_cgroup *parent);
> > + void (*unlock)(struct mem_cgroup *memcg, struct mem_cgroup *parent);
> > + void (*reparent)(struct mem_cgroup *memcg, struct mem_cgroup *parent);
> > +};
> > +
> > +void __init register_memcg_repatent(struct memcg_reparent_ops *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 a48403e5999c..f88fe2f06f5b 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -330,6 +330,41 @@ static struct obj_cgroup *obj_cgroup_alloc(void)
> > return objcg;
> > }
> >
> > +static LIST_HEAD(reparent_ops_head);
>
> Because this list is completely static, why not make a build-time initialized
> array instead?
I didn't think of using an array before. The first idea that popped out
was a list. But you remind me of the array. I'd love to replace it with
the array.
Thanks, Roman.
> I guess it's a more canonical way of solving problems like this.
> The proposed API looks good to me.