[PATCH] radix-tree: account nodes to memcg only if explicitly requested

[PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Vladimir Davydov]

Radix trees may be used not only for storing page cache pages, so
unconditionally accounting radix tree nodes to the current memory cgroup
is bad: if a radix tree node is used for storing data shared among
different cgroups we risk pinning dead memory cgroups forever. So let's
only account radix tree nodes if it was explicitly requested by passing
__GFP_ACCOUNT to INIT_RADIX_TREE. Currently, we only want to account
page cache entries, so mark mapping->page_tree so.

Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
---
 fs/inode.c       |  2 +-
 lib/radix-tree.c | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 559a9da25237..1d04dab5211c 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -345,7 +345,7 @@ EXPORT_SYMBOL(inc_nlink);
 void address_space_init_once(struct address_space *mapping)
 {
 	memset(mapping, 0, sizeof(*mapping));
-	INIT_RADIX_TREE(&mapping->page_tree, GFP_ATOMIC);
+	INIT_RADIX_TREE(&mapping->page_tree, GFP_ATOMIC | __GFP_ACCOUNT);
 	spin_lock_init(&mapping->tree_lock);
 	init_rwsem(&mapping->i_mmap_rwsem);
 	INIT_LIST_HEAD(&mapping->private_list);
diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 61b8fb529cef..1b7bf7314141 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -277,10 +277,11 @@ radix_tree_node_alloc(struct radix_tree_root *root)
 
 		/*
 		 * Even if the caller has preloaded, try to allocate from the
-		 * cache first for the new node to get accounted.
+		 * cache first for the new node to get accounted to the memory
+		 * cgroup.
 		 */
 		ret = kmem_cache_alloc(radix_tree_node_cachep,
-				       gfp_mask | __GFP_ACCOUNT | __GFP_NOWARN);
+				       gfp_mask | __GFP_NOWARN);
 		if (ret)
 			goto out;
 
@@ -303,8 +304,7 @@ radix_tree_node_alloc(struct radix_tree_root *root)
 		kmemleak_update_trace(ret);
 		goto out;
 	}
-	ret = kmem_cache_alloc(radix_tree_node_cachep,
-			       gfp_mask | __GFP_ACCOUNT);
+	ret = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask);
 out:
 	BUG_ON(radix_tree_is_internal_node(ret));
 	return ret;
@@ -351,6 +351,12 @@ static int __radix_tree_preload(gfp_t gfp_mask, int nr)
 	struct radix_tree_node *node;
 	int ret = -ENOMEM;
 
+	/*
+	 * Nodes preloaded by one cgroup can be be used by another cgroup, so
+	 * they should never be accounted to any particular memory cgroup.
+	 */
+	gfp_mask &= ~__GFP_ACCOUNT;
+
 	preempt_disable();
 	rtp = this_cpu_ptr(&radix_tree_preloads);
 	while (rtp->nr < nr) {
-- 
2.1.4

[PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Vladimir Davydov]

Radix trees may be used not only for storing page cache pages, so
unconditionally accounting radix tree nodes to the current memory cgroup
is bad: if a radix tree node is used for storing data shared among
different cgroups we risk pinning dead memory cgroups forever. So let's
only account radix tree nodes if it was explicitly requested by passing
__GFP_ACCOUNT to INIT_RADIX_TREE. Currently, we only want to account
page cache entries, so mark mapping->page_tree so.

Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
---
 fs/inode.c       |  2 +-
 lib/radix-tree.c | 14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 559a9da25237..1d04dab5211c 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -345,7 +345,7 @@ EXPORT_SYMBOL(inc_nlink);
 void address_space_init_once(struct address_space *mapping)
 {
 	memset(mapping, 0, sizeof(*mapping));
-	INIT_RADIX_TREE(&mapping->page_tree, GFP_ATOMIC);
+	INIT_RADIX_TREE(&mapping->page_tree, GFP_ATOMIC | __GFP_ACCOUNT);
 	spin_lock_init(&mapping->tree_lock);
 	init_rwsem(&mapping->i_mmap_rwsem);
 	INIT_LIST_HEAD(&mapping->private_list);
diff --git a/lib/radix-tree.c b/lib/radix-tree.c
index 61b8fb529cef..1b7bf7314141 100644
--- a/lib/radix-tree.c
+++ b/lib/radix-tree.c
@@ -277,10 +277,11 @@ radix_tree_node_alloc(struct radix_tree_root *root)
 
 		/*
 		 * Even if the caller has preloaded, try to allocate from the
-		 * cache first for the new node to get accounted.
+		 * cache first for the new node to get accounted to the memory
+		 * cgroup.
 		 */
 		ret = kmem_cache_alloc(radix_tree_node_cachep,
-				       gfp_mask | __GFP_ACCOUNT | __GFP_NOWARN);
+				       gfp_mask | __GFP_NOWARN);
 		if (ret)
 			goto out;
 
@@ -303,8 +304,7 @@ radix_tree_node_alloc(struct radix_tree_root *root)
 		kmemleak_update_trace(ret);
 		goto out;
 	}
-	ret = kmem_cache_alloc(radix_tree_node_cachep,
-			       gfp_mask | __GFP_ACCOUNT);
+	ret = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask);
 out:
 	BUG_ON(radix_tree_is_internal_node(ret));
 	return ret;
@@ -351,6 +351,12 @@ static int __radix_tree_preload(gfp_t gfp_mask, int nr)
 	struct radix_tree_node *node;
 	int ret = -ENOMEM;
 
+	/*
+	 * Nodes preloaded by one cgroup can be be used by another cgroup, so
+	 * they should never be accounted to any particular memory cgroup.
+	 */
+	gfp_mask &= ~__GFP_ACCOUNT;
+
 	preempt_disable();
 	rtp = this_cpu_ptr(&radix_tree_preloads);
 	while (rtp->nr < nr) {
-- 
2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Johannes Weiner]

On Mon, Aug 01, 2016 at 04:13:08PM +0300, Vladimir Davydov wrote:
> Radix trees may be used not only for storing page cache pages, so
> unconditionally accounting radix tree nodes to the current memory cgroup
> is bad: if a radix tree node is used for storing data shared among
> different cgroups we risk pinning dead memory cgroups forever. So let's
> only account radix tree nodes if it was explicitly requested by passing
> __GFP_ACCOUNT to INIT_RADIX_TREE. Currently, we only want to account
> page cache entries, so mark mapping->page_tree so.

Is this a theoretical fix, or did you actually run into problems? I
wouldn't expect any other radix tree node consumer in the kernel to
come anywhere close to the page cache, so I wonder why it matters.

> @@ -351,6 +351,12 @@ static int __radix_tree_preload(gfp_t gfp_mask, int nr)
>  	struct radix_tree_node *node;
>  	int ret = -ENOMEM;
>  
> +	/*
> +	 * Nodes preloaded by one cgroup can be be used by another cgroup, so
> +	 * they should never be accounted to any particular memory cgroup.
> +	 */
> +	gfp_mask &= ~__GFP_ACCOUNT;

But *all* page cache radix tree nodes are allocated from inside the
preload code, since the tree insertions need mapping->tree_lock. So
this would effectively disable accounting of the biggest radix tree
consumer in the kernel, no?

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Johannes Weiner]

On Mon, Aug 01, 2016 at 04:13:08PM +0300, Vladimir Davydov wrote:
> Radix trees may be used not only for storing page cache pages, so
> unconditionally accounting radix tree nodes to the current memory cgroup
> is bad: if a radix tree node is used for storing data shared among
> different cgroups we risk pinning dead memory cgroups forever. So let's
> only account radix tree nodes if it was explicitly requested by passing
> __GFP_ACCOUNT to INIT_RADIX_TREE. Currently, we only want to account
> page cache entries, so mark mapping->page_tree so.

Is this a theoretical fix, or did you actually run into problems? I
wouldn't expect any other radix tree node consumer in the kernel to
come anywhere close to the page cache, so I wonder why it matters.

> @@ -351,6 +351,12 @@ static int __radix_tree_preload(gfp_t gfp_mask, int nr)
>  	struct radix_tree_node *node;
>  	int ret = -ENOMEM;
>  
> +	/*
> +	 * Nodes preloaded by one cgroup can be be used by another cgroup, so
> +	 * they should never be accounted to any particular memory cgroup.
> +	 */
> +	gfp_mask &= ~__GFP_ACCOUNT;

But *all* page cache radix tree nodes are allocated from inside the
preload code, since the tree insertions need mapping->tree_lock. So
this would effectively disable accounting of the biggest radix tree
consumer in the kernel, no?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Vladimir Davydov]

On Mon, Aug 01, 2016 at 11:24:09AM -0400, Johannes Weiner wrote:
> On Mon, Aug 01, 2016 at 04:13:08PM +0300, Vladimir Davydov wrote:
> > Radix trees may be used not only for storing page cache pages, so
> > unconditionally accounting radix tree nodes to the current memory cgroup
> > is bad: if a radix tree node is used for storing data shared among
> > different cgroups we risk pinning dead memory cgroups forever. So let's
> > only account radix tree nodes if it was explicitly requested by passing
> > __GFP_ACCOUNT to INIT_RADIX_TREE. Currently, we only want to account
> > page cache entries, so mark mapping->page_tree so.
> 
> Is this a theoretical fix, or did you actually run into problems? I
> wouldn't expect any other radix tree node consumer in the kernel to
> come anywhere close to the page cache, so I wonder why it matters.

There are radix trees used for storing kernel data for different
cgroups, e.g. bdi->cgwb_tree. Nodes of such trees are shared among
different cgroups, so accounting a node to a particular memory cgroup
will pin the cgroup until all users of the node are gone which may never
happen. Although this can only result in slightly increased memory
consumption due to dangling offline memory cgroups and their kmem
caches, we'd better avoid it whenever possible. BTW this was one of the
arguments for switching to the white-list kmem accounting policy.

> 
> > @@ -351,6 +351,12 @@ static int __radix_tree_preload(gfp_t gfp_mask, int nr)
> >  	struct radix_tree_node *node;
> >  	int ret = -ENOMEM;
> >  
> > +	/*
> > +	 * Nodes preloaded by one cgroup can be be used by another cgroup, so
> > +	 * they should never be accounted to any particular memory cgroup.
> > +	 */
> > +	gfp_mask &= ~__GFP_ACCOUNT;
> 
> But *all* page cache radix tree nodes are allocated from inside the
> preload code, since the tree insertions need mapping->tree_lock. So
> this would effectively disable accounting of the biggest radix tree
> consumer in the kernel, no?

No, that's not how accounting of radix tree nodes works. We never
account preloaded nodes, because this could result in a node accounted
to one cgroup used by an unrelated cgroup. Instead we always try to
kmalloc a node on insertion falling back on preloads only if kmalloc
fails - see commit 58e698af4c634 ("radix-tree: account radix_tree_node
to memory cgroup").

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Vladimir Davydov]

On Mon, Aug 01, 2016 at 11:24:09AM -0400, Johannes Weiner wrote:
> On Mon, Aug 01, 2016 at 04:13:08PM +0300, Vladimir Davydov wrote:
> > Radix trees may be used not only for storing page cache pages, so
> > unconditionally accounting radix tree nodes to the current memory cgroup
> > is bad: if a radix tree node is used for storing data shared among
> > different cgroups we risk pinning dead memory cgroups forever. So let's
> > only account radix tree nodes if it was explicitly requested by passing
> > __GFP_ACCOUNT to INIT_RADIX_TREE. Currently, we only want to account
> > page cache entries, so mark mapping->page_tree so.
> 
> Is this a theoretical fix, or did you actually run into problems? I
> wouldn't expect any other radix tree node consumer in the kernel to
> come anywhere close to the page cache, so I wonder why it matters.

There are radix trees used for storing kernel data for different
cgroups, e.g. bdi->cgwb_tree. Nodes of such trees are shared among
different cgroups, so accounting a node to a particular memory cgroup
will pin the cgroup until all users of the node are gone which may never
happen. Although this can only result in slightly increased memory
consumption due to dangling offline memory cgroups and their kmem
caches, we'd better avoid it whenever possible. BTW this was one of the
arguments for switching to the white-list kmem accounting policy.

> 
> > @@ -351,6 +351,12 @@ static int __radix_tree_preload(gfp_t gfp_mask, int nr)
> >  	struct radix_tree_node *node;
> >  	int ret = -ENOMEM;
> >  
> > +	/*
> > +	 * Nodes preloaded by one cgroup can be be used by another cgroup, so
> > +	 * they should never be accounted to any particular memory cgroup.
> > +	 */
> > +	gfp_mask &= ~__GFP_ACCOUNT;
> 
> But *all* page cache radix tree nodes are allocated from inside the
> preload code, since the tree insertions need mapping->tree_lock. So
> this would effectively disable accounting of the biggest radix tree
> consumer in the kernel, no?

No, that's not how accounting of radix tree nodes works. We never
account preloaded nodes, because this could result in a node accounted
to one cgroup used by an unrelated cgroup. Instead we always try to
kmalloc a node on insertion falling back on preloads only if kmalloc
fails - see commit 58e698af4c634 ("radix-tree: account radix_tree_node
to memory cgroup").

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Johannes Weiner]

On Mon, Aug 01, 2016 at 07:06:05PM +0300, Vladimir Davydov wrote:
> On Mon, Aug 01, 2016 at 11:24:09AM -0400, Johannes Weiner wrote:
> > On Mon, Aug 01, 2016 at 04:13:08PM +0300, Vladimir Davydov wrote:
> > > @@ -351,6 +351,12 @@ static int __radix_tree_preload(gfp_t gfp_mask, int nr)
> > >  	struct radix_tree_node *node;
> > >  	int ret = -ENOMEM;
> > >  
> > > +	/*
> > > +	 * Nodes preloaded by one cgroup can be be used by another cgroup, so
> > > +	 * they should never be accounted to any particular memory cgroup.
> > > +	 */
> > > +	gfp_mask &= ~__GFP_ACCOUNT;
> > 
> > But *all* page cache radix tree nodes are allocated from inside the
> > preload code, since the tree insertions need mapping->tree_lock. So
> > this would effectively disable accounting of the biggest radix tree
> > consumer in the kernel, no?
> 
> No, that's not how accounting of radix tree nodes works. We never
> account preloaded nodes, because this could result in a node accounted
> to one cgroup used by an unrelated cgroup. Instead we always try to
> kmalloc a node on insertion falling back on preloads only if kmalloc
> fails - see commit 58e698af4c634 ("radix-tree: account radix_tree_node
> to memory cgroup").

You are right, I forgot we are doing this. The patch makes sense then.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Johannes Weiner]

On Mon, Aug 01, 2016 at 07:06:05PM +0300, Vladimir Davydov wrote:
> On Mon, Aug 01, 2016 at 11:24:09AM -0400, Johannes Weiner wrote:
> > On Mon, Aug 01, 2016 at 04:13:08PM +0300, Vladimir Davydov wrote:
> > > @@ -351,6 +351,12 @@ static int __radix_tree_preload(gfp_t gfp_mask, int nr)
> > >  	struct radix_tree_node *node;
> > >  	int ret = -ENOMEM;
> > >  
> > > +	/*
> > > +	 * Nodes preloaded by one cgroup can be be used by another cgroup, so
> > > +	 * they should never be accounted to any particular memory cgroup.
> > > +	 */
> > > +	gfp_mask &= ~__GFP_ACCOUNT;
> > 
> > But *all* page cache radix tree nodes are allocated from inside the
> > preload code, since the tree insertions need mapping->tree_lock. So
> > this would effectively disable accounting of the biggest radix tree
> > consumer in the kernel, no?
> 
> No, that's not how accounting of radix tree nodes works. We never
> account preloaded nodes, because this could result in a node accounted
> to one cgroup used by an unrelated cgroup. Instead we always try to
> kmalloc a node on insertion falling back on preloads only if kmalloc
> fails - see commit 58e698af4c634 ("radix-tree: account radix_tree_node
> to memory cgroup").

You are right, I forgot we are doing this. The patch makes sense then.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Michal Hocko]

On Mon 01-08-16 16:13:08, Vladimir Davydov wrote:
> Radix trees may be used not only for storing page cache pages, so
> unconditionally accounting radix tree nodes to the current memory cgroup
> is bad: if a radix tree node is used for storing data shared among
> different cgroups we risk pinning dead memory cgroups forever. So let's
> only account radix tree nodes if it was explicitly requested by passing
> __GFP_ACCOUNT to INIT_RADIX_TREE. Currently, we only want to account
> page cache entries, so mark mapping->page_tree so.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>

OK, the patch makes sense to me. Such a false sharing would be really
tedious to debug

Do we want to mark it for stable 4.6 to prevent from some pathological
issues. The patch is simple enough.

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  fs/inode.c       |  2 +-
>  lib/radix-tree.c | 14 ++++++++++----
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 559a9da25237..1d04dab5211c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -345,7 +345,7 @@ EXPORT_SYMBOL(inc_nlink);
>  void address_space_init_once(struct address_space *mapping)
>  {
>  	memset(mapping, 0, sizeof(*mapping));
> -	INIT_RADIX_TREE(&mapping->page_tree, GFP_ATOMIC);
> +	INIT_RADIX_TREE(&mapping->page_tree, GFP_ATOMIC | __GFP_ACCOUNT);
>  	spin_lock_init(&mapping->tree_lock);
>  	init_rwsem(&mapping->i_mmap_rwsem);
>  	INIT_LIST_HEAD(&mapping->private_list);
> diff --git a/lib/radix-tree.c b/lib/radix-tree.c
> index 61b8fb529cef..1b7bf7314141 100644
> --- a/lib/radix-tree.c
> +++ b/lib/radix-tree.c
> @@ -277,10 +277,11 @@ radix_tree_node_alloc(struct radix_tree_root *root)
>  
>  		/*
>  		 * Even if the caller has preloaded, try to allocate from the
> -		 * cache first for the new node to get accounted.
> +		 * cache first for the new node to get accounted to the memory
> +		 * cgroup.
>  		 */
>  		ret = kmem_cache_alloc(radix_tree_node_cachep,
> -				       gfp_mask | __GFP_ACCOUNT | __GFP_NOWARN);
> +				       gfp_mask | __GFP_NOWARN);
>  		if (ret)
>  			goto out;
>  
> @@ -303,8 +304,7 @@ radix_tree_node_alloc(struct radix_tree_root *root)
>  		kmemleak_update_trace(ret);
>  		goto out;
>  	}
> -	ret = kmem_cache_alloc(radix_tree_node_cachep,
> -			       gfp_mask | __GFP_ACCOUNT);
> +	ret = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask);
>  out:
>  	BUG_ON(radix_tree_is_internal_node(ret));
>  	return ret;
> @@ -351,6 +351,12 @@ static int __radix_tree_preload(gfp_t gfp_mask, int nr)
>  	struct radix_tree_node *node;
>  	int ret = -ENOMEM;
>  
> +	/*
> +	 * Nodes preloaded by one cgroup can be be used by another cgroup, so
> +	 * they should never be accounted to any particular memory cgroup.
> +	 */
> +	gfp_mask &= ~__GFP_ACCOUNT;
> +
>  	preempt_disable();
>  	rtp = this_cpu_ptr(&radix_tree_preloads);
>  	while (rtp->nr < nr) {
> -- 
> 2.1.4
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Michal Hocko]

On Mon 01-08-16 16:13:08, Vladimir Davydov wrote:
> Radix trees may be used not only for storing page cache pages, so
> unconditionally accounting radix tree nodes to the current memory cgroup
> is bad: if a radix tree node is used for storing data shared among
> different cgroups we risk pinning dead memory cgroups forever. So let's
> only account radix tree nodes if it was explicitly requested by passing
> __GFP_ACCOUNT to INIT_RADIX_TREE. Currently, we only want to account
> page cache entries, so mark mapping->page_tree so.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>

OK, the patch makes sense to me. Such a false sharing would be really
tedious to debug

Do we want to mark it for stable 4.6 to prevent from some pathological
issues. The patch is simple enough.

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  fs/inode.c       |  2 +-
>  lib/radix-tree.c | 14 ++++++++++----
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 559a9da25237..1d04dab5211c 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -345,7 +345,7 @@ EXPORT_SYMBOL(inc_nlink);
>  void address_space_init_once(struct address_space *mapping)
>  {
>  	memset(mapping, 0, sizeof(*mapping));
> -	INIT_RADIX_TREE(&mapping->page_tree, GFP_ATOMIC);
> +	INIT_RADIX_TREE(&mapping->page_tree, GFP_ATOMIC | __GFP_ACCOUNT);
>  	spin_lock_init(&mapping->tree_lock);
>  	init_rwsem(&mapping->i_mmap_rwsem);
>  	INIT_LIST_HEAD(&mapping->private_list);
> diff --git a/lib/radix-tree.c b/lib/radix-tree.c
> index 61b8fb529cef..1b7bf7314141 100644
> --- a/lib/radix-tree.c
> +++ b/lib/radix-tree.c
> @@ -277,10 +277,11 @@ radix_tree_node_alloc(struct radix_tree_root *root)
>  
>  		/*
>  		 * Even if the caller has preloaded, try to allocate from the
> -		 * cache first for the new node to get accounted.
> +		 * cache first for the new node to get accounted to the memory
> +		 * cgroup.
>  		 */
>  		ret = kmem_cache_alloc(radix_tree_node_cachep,
> -				       gfp_mask | __GFP_ACCOUNT | __GFP_NOWARN);
> +				       gfp_mask | __GFP_NOWARN);
>  		if (ret)
>  			goto out;
>  
> @@ -303,8 +304,7 @@ radix_tree_node_alloc(struct radix_tree_root *root)
>  		kmemleak_update_trace(ret);
>  		goto out;
>  	}
> -	ret = kmem_cache_alloc(radix_tree_node_cachep,
> -			       gfp_mask | __GFP_ACCOUNT);
> +	ret = kmem_cache_alloc(radix_tree_node_cachep, gfp_mask);
>  out:
>  	BUG_ON(radix_tree_is_internal_node(ret));
>  	return ret;
> @@ -351,6 +351,12 @@ static int __radix_tree_preload(gfp_t gfp_mask, int nr)
>  	struct radix_tree_node *node;
>  	int ret = -ENOMEM;
>  
> +	/*
> +	 * Nodes preloaded by one cgroup can be be used by another cgroup, so
> +	 * they should never be accounted to any particular memory cgroup.
> +	 */
> +	gfp_mask &= ~__GFP_ACCOUNT;
> +
>  	preempt_disable();
>  	rtp = this_cpu_ptr(&radix_tree_preloads);
>  	while (rtp->nr < nr) {
> -- 
> 2.1.4
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Vladimir Davydov]

On Tue, Aug 02, 2016 at 01:51:12PM +0200, Michal Hocko wrote:
> On Mon 01-08-16 16:13:08, Vladimir Davydov wrote:
> > Radix trees may be used not only for storing page cache pages, so
> > unconditionally accounting radix tree nodes to the current memory cgroup
> > is bad: if a radix tree node is used for storing data shared among
> > different cgroups we risk pinning dead memory cgroups forever. So let's
> > only account radix tree nodes if it was explicitly requested by passing
> > __GFP_ACCOUNT to INIT_RADIX_TREE. Currently, we only want to account
> > page cache entries, so mark mapping->page_tree so.
> > 
> > Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
> 
> OK, the patch makes sense to me. Such a false sharing would be really
> tedious to debug
> 
> Do we want to mark it for stable 4.6 to prevent from some pathological
> issues. The patch is simple enough.

Makes sense, expecially taking into account that kmemcg is enabled by
default now. I'll resend the patch for stable then.

> 
> Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Vladimir Davydov]

On Tue, Aug 02, 2016 at 01:51:12PM +0200, Michal Hocko wrote:
> On Mon 01-08-16 16:13:08, Vladimir Davydov wrote:
> > Radix trees may be used not only for storing page cache pages, so
> > unconditionally accounting radix tree nodes to the current memory cgroup
> > is bad: if a radix tree node is used for storing data shared among
> > different cgroups we risk pinning dead memory cgroups forever. So let's
> > only account radix tree nodes if it was explicitly requested by passing
> > __GFP_ACCOUNT to INIT_RADIX_TREE. Currently, we only want to account
> > page cache entries, so mark mapping->page_tree so.
> > 
> > Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
> 
> OK, the patch makes sense to me. Such a false sharing would be really
> tedious to debug
> 
> Do we want to mark it for stable 4.6 to prevent from some pathological
> issues. The patch is simple enough.

Makes sense, expecially taking into account that kmemcg is enabled by
default now. I'll resend the patch for stable then.

> 
> Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Michal Hocko]

On Tue 02-08-16 15:42:20, Vladimir Davydov wrote:
> On Tue, Aug 02, 2016 at 01:51:12PM +0200, Michal Hocko wrote:
> > On Mon 01-08-16 16:13:08, Vladimir Davydov wrote:
> > > Radix trees may be used not only for storing page cache pages, so
> > > unconditionally accounting radix tree nodes to the current memory cgroup
> > > is bad: if a radix tree node is used for storing data shared among
> > > different cgroups we risk pinning dead memory cgroups forever. So let's
> > > only account radix tree nodes if it was explicitly requested by passing
> > > __GFP_ACCOUNT to INIT_RADIX_TREE. Currently, we only want to account
> > > page cache entries, so mark mapping->page_tree so.
> > > 
> > > Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
> > 
> > OK, the patch makes sense to me. Such a false sharing would be really
> > tedious to debug
> > 
> > Do we want to mark it for stable 4.6 to prevent from some pathological
> > issues. The patch is simple enough.
> 
> Makes sense, expecially taking into account that kmemcg is enabled by
> default now.
>
> I'll resend the patch for stable then.

Maybe Andrew just want's to mark it for stable with
Fixes: 58e698af4c63 ("radix-tree: account radix_tree_node to memory cgroup")
 Cc: stable # 4.6

> > Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks!
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Michal Hocko]

On Tue 02-08-16 15:42:20, Vladimir Davydov wrote:
> On Tue, Aug 02, 2016 at 01:51:12PM +0200, Michal Hocko wrote:
> > On Mon 01-08-16 16:13:08, Vladimir Davydov wrote:
> > > Radix trees may be used not only for storing page cache pages, so
> > > unconditionally accounting radix tree nodes to the current memory cgroup
> > > is bad: if a radix tree node is used for storing data shared among
> > > different cgroups we risk pinning dead memory cgroups forever. So let's
> > > only account radix tree nodes if it was explicitly requested by passing
> > > __GFP_ACCOUNT to INIT_RADIX_TREE. Currently, we only want to account
> > > page cache entries, so mark mapping->page_tree so.
> > > 
> > > Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
> > 
> > OK, the patch makes sense to me. Such a false sharing would be really
> > tedious to debug
> > 
> > Do we want to mark it for stable 4.6 to prevent from some pathological
> > issues. The patch is simple enough.
> 
> Makes sense, expecially taking into account that kmemcg is enabled by
> default now.
>
> I'll resend the patch for stable then.

Maybe Andrew just want's to mark it for stable with
Fixes: 58e698af4c63 ("radix-tree: account radix_tree_node to memory cgroup")
 Cc: stable # 4.6

> > Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks!
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Andrew Morton]

On Tue, 2 Aug 2016 14:46:45 +0200 Michal Hocko <mhocko@kernel.org> wrote:

> Maybe Andrew just want's to mark it for stable with
> Fixes: 58e698af4c63 ("radix-tree: account radix_tree_node to memory cgroup")
>  Cc: stable # 4.6

Done.

Re: [PATCH] radix-tree: account nodes to memcg only if explicitly requested

[Andrew Morton]

On Tue, 2 Aug 2016 14:46:45 +0200 Michal Hocko <mhocko@kernel.org> wrote:

> Maybe Andrew just want's to mark it for stable with
> Fixes: 58e698af4c63 ("radix-tree: account radix_tree_node to memory cgroup")
>  Cc: stable # 4.6

Done.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>