Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1454906 > unrolled thread

Re: [PATCH v2 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 1/3] mm: memcontrol: fix swap counter leak on swapout  from offline cgroup Michal Hocko <mhocko@kernel.org> - 2016-08-02 18:20 +0200
    Re: [PATCH v2 1/3] mm: memcontrol: fix swap counter leak on swapout  from offline cgroup Johannes Weiner <hannes@cmpxchg.org> - 2016-08-02 19:40 +0200
      Re: [PATCH v2 1/3] mm: memcontrol: fix swap counter leak on swapout  from offline cgroup Michal Hocko <mhocko@kernel.org> - 2016-08-02 22:50 +0200
    Re: [PATCH v2 1/3] mm: memcontrol: fix swap counter leak on swapout  from offline cgroup Michal Hocko <mhocko@kernel.org> - 2016-08-03 13:30 +0200
      Re: [PATCH v2 1/3] mm: memcontrol: fix swap counter leak on swapout  from offline cgroup Michal Hocko <mhocko@kernel.org> - 2016-08-03 14:10 +0200
      Re: [PATCH v2 1/3] mm: memcontrol: fix swap counter leak on swapout  from offline cgroup Johannes Weiner <hannes@cmpxchg.org> - 2016-08-03 16:20 +0200

#1454906 — Re: [PATCH v2 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup

On Tue 02-08-16 18:00:48, Vladimir Davydov wrote:
> An offline memory cgroup might have anonymous memory or shmem left
> charged to it and no swap. Since only swap entries pin the id of an
> offline cgroup, such a cgroup will have no id and so an attempt to
> swapout its anon/shmem will not store memory cgroup info in the swap
> cgroup map. As a result, memcg->swap or memcg->memsw will never get
> uncharged from it and any of its ascendants.
> 
> Fix this by always charging swapout to the first ancestor cgroup that
> hasn't released its id yet.
> 
> Fixes: 73f576c04b941 ("mm: memcontrol: fix cgroup creation failure after many small jobs")
> Signed-off-by: Vladimir Davydov <vdavydov@virtuozzo.com>
> Cc: <stable@vger.kernel.org>	[3.19+]
> ---
> Changes in v2:
>  - handle !use_hierarchy case properly (Michal)
> 
>  mm/memcontrol.c | 38 ++++++++++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 3be791afd372..4ae12effe347 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4036,6 +4036,24 @@ static void mem_cgroup_id_get(struct mem_cgroup *memcg)
>  	atomic_inc(&memcg->id.ref);
>  }
>  
> +static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
> +{
> +	while (!atomic_inc_not_zero(&memcg->id.ref)) {
> +		/*
> +		 * The root cgroup cannot be destroyed, so it's refcount must
> +		 * always be >= 1.
> +		 */
> +		if (memcg == root_mem_cgroup) {
> +			VM_BUG_ON(1);
> +			break;
> +		}

why not simply VM_BUG_ON(memcg == root_mem_cgroup)?

> +		memcg = parent_mem_cgroup(memcg);
> +		if (!memcg)
> +			memcg = root_mem_cgroup;
> +	}
> +	return memcg;
> +}
> +
>  static void mem_cgroup_id_put(struct mem_cgroup *memcg)
>  {
>  	if (atomic_dec_and_test(&memcg->id.ref)) {
> @@ -5752,7 +5770,7 @@ subsys_initcall(mem_cgroup_init);
>   */
>  void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
>  {
> -	struct mem_cgroup *memcg;
> +	struct mem_cgroup *memcg, *swap_memcg;
>  	unsigned short oldid;
>  
>  	VM_BUG_ON_PAGE(PageLRU(page), page);
> @@ -5767,15 +5785,20 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
>  	if (!memcg)
>  		return;
>  
> -	mem_cgroup_id_get(memcg);
> -	oldid = swap_cgroup_record(entry, mem_cgroup_id(memcg));
> +	swap_memcg = mem_cgroup_id_get_active(memcg);
> +	oldid = swap_cgroup_record(entry, mem_cgroup_id(swap_memcg));
>  	VM_BUG_ON_PAGE(oldid, page);
> -	mem_cgroup_swap_statistics(memcg, true);
> +	mem_cgroup_swap_statistics(swap_memcg, true);
>  
>  	page->mem_cgroup = NULL;
>  
>  	if (!mem_cgroup_is_root(memcg))
>  		page_counter_uncharge(&memcg->memory, 1);
> +	if (memcg != swap_memcg) {
> +		if (!mem_cgroup_is_root(swap_memcg))
> +			page_counter_charge(&swap_memcg->memsw, 1);
> +		page_counter_uncharge(&memcg->memsw, 1);
> +	}
>  
>  	/*
>  	 * Interrupts should be disabled here because the caller holds the

The resulting code is a weird mixture of memcg and swap_memcg usage
which is really confusing and error prone. Do we really have to do
uncharge on an already offline memcg?
-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1455152

On Tue, Aug 02, 2016 at 06:00:26PM +0200, Michal Hocko wrote:
> On Tue 02-08-16 18:00:48, Vladimir Davydov wrote:
> > @@ -5767,15 +5785,20 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
> >  	if (!memcg)
> >  		return;
> >  
> > -	mem_cgroup_id_get(memcg);
> > -	oldid = swap_cgroup_record(entry, mem_cgroup_id(memcg));
> > +	swap_memcg = mem_cgroup_id_get_active(memcg);
> > +	oldid = swap_cgroup_record(entry, mem_cgroup_id(swap_memcg));
> >  	VM_BUG_ON_PAGE(oldid, page);
> > -	mem_cgroup_swap_statistics(memcg, true);
> > +	mem_cgroup_swap_statistics(swap_memcg, true);
> >  
> >  	page->mem_cgroup = NULL;
> >  
> >  	if (!mem_cgroup_is_root(memcg))
> >  		page_counter_uncharge(&memcg->memory, 1);
> > +	if (memcg != swap_memcg) {
> > +		if (!mem_cgroup_is_root(swap_memcg))
> > +			page_counter_charge(&swap_memcg->memsw, 1);
> > +		page_counter_uncharge(&memcg->memsw, 1);
> > +	}
> >  
> >  	/*
> >  	 * Interrupts should be disabled here because the caller holds the
> 
> The resulting code is a weird mixture of memcg and swap_memcg usage
> which is really confusing and error prone. Do we really have to do
> uncharge on an already offline memcg?

The charge is recursive and includes swap_memcg, i.e. live groups, so
the uncharge is necessary. I don't think the code is too bad, though?
swap_memcg is the target that is being charged for swap, memcg is the
origin group from which we swap out. Seems pretty straightforward...?

But maybe a comment above the memcg != swap_memcg check would be nice:

/*
 * In case the memcg owning these pages has been offlined and doesn't
 * have an ID allocated to it anymore, charge the closest online
 * ancestor for the swap instead and transfer the memory+swap charge.
 */

Thinking about it, mem_cgroup_id_get_active() is a little strange; the
term we use throughout the cgroup code is "online". It might be good
to rename this mem_cgroup_id_get_online().

Thanks

[toc] | [prev] | [next] | [standalone]

#1455463

On Tue 02-08-16 13:33:37, Johannes Weiner wrote:
> On Tue, Aug 02, 2016 at 06:00:26PM +0200, Michal Hocko wrote:
> > On Tue 02-08-16 18:00:48, Vladimir Davydov wrote:
> > > @@ -5767,15 +5785,20 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
> > >  	if (!memcg)
> > >  		return;
> > >  
> > > -	mem_cgroup_id_get(memcg);
> > > -	oldid = swap_cgroup_record(entry, mem_cgroup_id(memcg));
> > > +	swap_memcg = mem_cgroup_id_get_active(memcg);
> > > +	oldid = swap_cgroup_record(entry, mem_cgroup_id(swap_memcg));
> > >  	VM_BUG_ON_PAGE(oldid, page);
> > > -	mem_cgroup_swap_statistics(memcg, true);
> > > +	mem_cgroup_swap_statistics(swap_memcg, true);
> > >  
> > >  	page->mem_cgroup = NULL;
> > >  
> > >  	if (!mem_cgroup_is_root(memcg))
> > >  		page_counter_uncharge(&memcg->memory, 1);
> > > +	if (memcg != swap_memcg) {
> > > +		if (!mem_cgroup_is_root(swap_memcg))
> > > +			page_counter_charge(&swap_memcg->memsw, 1);
> > > +		page_counter_uncharge(&memcg->memsw, 1);
> > > +	}
> > >  
> > >  	/*
> > >  	 * Interrupts should be disabled here because the caller holds the
> > 
> > The resulting code is a weird mixture of memcg and swap_memcg usage
> > which is really confusing and error prone. Do we really have to do
> > uncharge on an already offline memcg?
> 
> The charge is recursive and includes swap_memcg, i.e. live groups, so
> the uncharge is necessary.

Hmm, the charge is recursive, alraight, but then I see only see only
small sympathy for
               if (!mem_cgroup_is_root(swap_memcg))
                       page_counter_charge(&swap_memcg->memsw, 1);
               page_counter_uncharge(&memcg->memsw, 1);

we first charge up the hierarchy just to uncharge the same balance from
the lower. So the end result should be same, right? The only reason
would be that we uncharge the lower layer as well. I do not remember
details, but I do not remember we would be checking counters being 0 on
exit.
But it is quite late and my brain is quite burnt so I might miss
something easily. So whatever small style issues, I think the patch
is correct and feel free to add

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

I just think we can make this easier and more straightforward. See the
diff below (not even compile tested - just for an illustration).

> I don't think the code is too bad, though?
> swap_memcg is the target that is being charged for swap, memcg is the
> origin group from which we swap out. Seems pretty straightforward...?
> 
> But maybe a comment above the memcg != swap_memcg check would be nice:
> 
> /*
>  * In case the memcg owning these pages has been offlined and doesn't
>  * have an ID allocated to it anymore, charge the closest online
>  * ancestor for the swap instead and transfer the memory+swap charge.
>  */

comment would be definitely helpful.
 
> Thinking about it, mem_cgroup_id_get_active() is a little strange; the
> term we use throughout the cgroup code is "online". It might be good
> to rename this mem_cgroup_id_get_online().

yes, that would be better, imho

---
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b6ac01d2b908..66868b2a4c8c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5819,6 +5819,14 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 	VM_BUG_ON_PAGE(PageLRU(page), page);
 	VM_BUG_ON_PAGE(page_count(page), page);
 
+	/*
+	 * Interrupts should be disabled here because the caller holds the
+	 * mapping->tree_lock lock which is taken with interrupts-off. It is
+	 * important here to have the interrupts disabled because it is the
+	 * only synchronisation we have for udpating the per-CPU variables.
+	 */
+	VM_BUG_ON(!irqs_disabled());
+
 	if (!do_memsw_account())
 		return;
 
@@ -5828,6 +5836,12 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 	if (!memcg)
 		return;
 
+	/*
+	 * In case the memcg owning these pages has been offlined and doesn't
+	 * have an ID allocated to it anymore, charge the closest online
+	 * ancestor for the swap instead. Hierarchical charges will be preserved
+	 * and the offlined one will not cry with some discrepances in statistics
+	 */
 	swap_memcg = mem_cgroup_id_get_active(memcg);
 	oldid = swap_cgroup_record(entry, mem_cgroup_id(swap_memcg));
 	VM_BUG_ON_PAGE(oldid, page);
@@ -5837,21 +5851,11 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
 
 	if (!mem_cgroup_is_root(memcg))
 		page_counter_uncharge(&memcg->memory, 1);
-	if (memcg != swap_memcg) {
-		if (!mem_cgroup_is_root(swap_memcg))
-			page_counter_charge(&swap_memcg->memsw, 1);
-		page_counter_uncharge(&memcg->memsw, 1);
-	}
 
-	/*
-	 * Interrupts should be disabled here because the caller holds the
-	 * mapping->tree_lock lock which is taken with interrupts-off. It is
-	 * important here to have the interrupts disabled because it is the
-	 * only synchronisation we have for udpating the per-CPU variables.
-	 */
-	VM_BUG_ON(!irqs_disabled());
-	mem_cgroup_charge_statistics(memcg, page, false, -1);
-	memcg_check_events(memcg, page);
+	if (memcg == swap_memcg) {
+		mem_cgroup_charge_statistics(memcg, page, false, -1);
+		memcg_check_events(memcg, page);
+	}
 
 	if (!mem_cgroup_is_root(memcg))
 		css_put(&memcg->css);
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1455751

On Wed 03-08-16 12:50:49, Vladimir Davydov wrote:
> On Tue, Aug 02, 2016 at 06:00:26PM +0200, Michal Hocko wrote:
> > On Tue 02-08-16 18:00:48, Vladimir Davydov wrote:
> ...
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 3be791afd372..4ae12effe347 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -4036,6 +4036,24 @@ static void mem_cgroup_id_get(struct mem_cgroup *memcg)
> > >  	atomic_inc(&memcg->id.ref);
> > >  }
> > >  
> > > +static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
> > > +{
> > > +	while (!atomic_inc_not_zero(&memcg->id.ref)) {
> > > +		/*
> > > +		 * The root cgroup cannot be destroyed, so it's refcount must
> > > +		 * always be >= 1.
> > > +		 */
> > > +		if (memcg == root_mem_cgroup) {
> > > +			VM_BUG_ON(1);
> > > +			break;
> > > +		}
> > 
> > why not simply VM_BUG_ON(memcg == root_mem_cgroup)?
> 
> Because with DEBUG_VM disabled we could wind up looping forever here if
> the refcount of the root_mem_cgroup got screwed up. On production
> kernels, it's better to break the loop and carry on closing eyes on
> diverging counters rather than getting a lockup.

Wouldn't this just paper over a real bug? Anyway I will not insist but
making the code more complex just to pretend we can handle a situation
gracefully doesn't sound right to me.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1455770

On Wed 03-08-16 14:46:40, Vladimir Davydov wrote:
> On Wed, Aug 03, 2016 at 01:09:42PM +0200, Michal Hocko wrote:
> > On Wed 03-08-16 12:50:49, Vladimir Davydov wrote:
> > > On Tue, Aug 02, 2016 at 06:00:26PM +0200, Michal Hocko wrote:
> > > > On Tue 02-08-16 18:00:48, Vladimir Davydov wrote:
> > > ...
> > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > > > index 3be791afd372..4ae12effe347 100644
> > > > > --- a/mm/memcontrol.c
> > > > > +++ b/mm/memcontrol.c
> > > > > @@ -4036,6 +4036,24 @@ static void mem_cgroup_id_get(struct mem_cgroup *memcg)
> > > > >  	atomic_inc(&memcg->id.ref);
> > > > >  }
> > > > >  
> > > > > +static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
> > > > > +{
> > > > > +	while (!atomic_inc_not_zero(&memcg->id.ref)) {
> > > > > +		/*
> > > > > +		 * The root cgroup cannot be destroyed, so it's refcount must
> > > > > +		 * always be >= 1.
> > > > > +		 */
> > > > > +		if (memcg == root_mem_cgroup) {
> > > > > +			VM_BUG_ON(1);
> > > > > +			break;
> > > > > +		}
> > > > 
> > > > why not simply VM_BUG_ON(memcg == root_mem_cgroup)?
> > > 
> > > Because with DEBUG_VM disabled we could wind up looping forever here if
> > > the refcount of the root_mem_cgroup got screwed up. On production
> > > kernels, it's better to break the loop and carry on closing eyes on
> > > diverging counters rather than getting a lockup.
> > 
> > Wouldn't this just paper over a real bug? Anyway I will not insist but
> > making the code more complex just to pretend we can handle a situation
> > gracefully doesn't sound right to me.
> 
> But we can handle this IMO. AFAICS diverging id refcount will typically
> result in leaking swap charges, which aren't even a real resource.

Fair enough.

> At
> worst, we can leak an offline mem_cgroup, which is also not critical
> enough to crash the production system.

Agreed.

> I see your concern of papering over a bug though. What about adding a
> warning there?

WARN_ON_ONCE sounds better...
 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1c0aa59fd333..8c8e68becee9 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4044,7 +4044,7 @@ static struct mem_cgroup *mem_cgroup_id_get_online(struct mem_cgroup *memcg)
>  		 * The root cgroup cannot be destroyed, so it's refcount must
>  		 * always be >= 1.
>  		 */
> -		if (memcg == root_mem_cgroup) {
> +		if (WARN_ON_ONCE(memcg == root_mem_cgroup)) {
>  			VM_BUG_ON(1);
>  			break;
>  		}
> 
> --
> 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

[toc] | [prev] | [next] | [standalone]

#1455815

On Wed, Aug 03, 2016 at 02:46:40PM +0300, Vladimir Davydov wrote:
> On Wed, Aug 03, 2016 at 01:09:42PM +0200, Michal Hocko wrote:
> > On Wed 03-08-16 12:50:49, Vladimir Davydov wrote:
> > > On Tue, Aug 02, 2016 at 06:00:26PM +0200, Michal Hocko wrote:
> > > > On Tue 02-08-16 18:00:48, Vladimir Davydov wrote:
> > > ...
> > > > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > > > index 3be791afd372..4ae12effe347 100644
> > > > > --- a/mm/memcontrol.c
> > > > > +++ b/mm/memcontrol.c
> > > > > @@ -4036,6 +4036,24 @@ static void mem_cgroup_id_get(struct mem_cgroup *memcg)
> > > > >  	atomic_inc(&memcg->id.ref);
> > > > >  }
> > > > >  
> > > > > +static struct mem_cgroup *mem_cgroup_id_get_active(struct mem_cgroup *memcg)
> > > > > +{
> > > > > +	while (!atomic_inc_not_zero(&memcg->id.ref)) {
> > > > > +		/*
> > > > > +		 * The root cgroup cannot be destroyed, so it's refcount must
> > > > > +		 * always be >= 1.
> > > > > +		 */
> > > > > +		if (memcg == root_mem_cgroup) {
> > > > > +			VM_BUG_ON(1);
> > > > > +			break;
> > > > > +		}
> > > > 
> > > > why not simply VM_BUG_ON(memcg == root_mem_cgroup)?
> > > 
> > > Because with DEBUG_VM disabled we could wind up looping forever here if
> > > the refcount of the root_mem_cgroup got screwed up. On production
> > > kernels, it's better to break the loop and carry on closing eyes on
> > > diverging counters rather than getting a lockup.
> > 
> > Wouldn't this just paper over a real bug? Anyway I will not insist but
> > making the code more complex just to pretend we can handle a situation
> > gracefully doesn't sound right to me.
> 
> But we can handle this IMO. AFAICS diverging id refcount will typically
> result in leaking swap charges, which aren't even a real resource. At
> worst, we can leak an offline mem_cgroup, which is also not critical
> enough to crash the production system.

Agreed. If we have the option to detect and warn about the bug, but
can continue to limp along without causing data corruption, then
that's what we should do.

> I see your concern of papering over a bug though. What about adding a
> warning there?
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1c0aa59fd333..8c8e68becee9 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4044,7 +4044,7 @@ static struct mem_cgroup *mem_cgroup_id_get_online(struct mem_cgroup *memcg)
>  		 * The root cgroup cannot be destroyed, so it's refcount must
>  		 * always be >= 1.
>  		 */
> -		if (memcg == root_mem_cgroup) {
> +		if (WARN_ON_ONCE(memcg == root_mem_cgroup)) {
>  			VM_BUG_ON(1);
>  			break;
>  		}

The WARN_ON_ONCE() makes sense to me. But if we warn on all configs
anyway, the VM_BUG_ON() doesn't provide any additional value. Anybody
who is testing new code and enables DEBUG_VM should notice a warning
without requiring the kernel to blow up in their face; it also allows
them to check other state that is not necessarily available in BUG().

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web