Re: [PATCH v2 2/2] memcg: remove -ENOMEM at page migration.

[Michal Hocko <mhocko@suse.cz>]

On Wed 04-07-12 15:13:02, Johannes Weiner wrote:
> On Wed, Jul 04, 2012 at 02:04:45PM +0200, Michal Hocko wrote:
> > On Wed 04-07-12 10:30:19, Johannes Weiner wrote:
[...]
> > > Can we instead not charge the new page, just commit it while holding
> > > on to a css refcount, and have end_migration call a version of
> > > __mem_cgroup_uncharge_common() that updates the stats but leaves the
> > > res counters alone?
> > 
> > Yes, this is also a way to go. Both approaches have to lie a bit and
> > both have a discrepancy between stat and usage_in_bytes. I guess we can
> > live with that.
> > Kame's solution seems easier but yours prevent from a corner case when
> > the reclaim is triggered due to artificial charges so I guess it is
> > better to go with yours.
> > Few (trivial) comments on the patch bellow.
> 
> It's true that the cache/rss statistics still account for both pages.
> But they don't have behavioural impact and so I didn't bother.  

Only if somebody watches those numbers and blows up if rss+cache >
limit_in_bytes. I can imagine an LTP test like that. But the test would
need to trigger migration in the background...

> We could still fix this up later, but it's less urgent, I think.

Yes, I guess so

> 
> > > @@ -2955,7 +2956,10 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
> > >  		/* fallthrough */
> > >  	case MEM_CGROUP_CHARGE_TYPE_DROP:
> > >  		/* See mem_cgroup_prepare_migration() */
> > > -		if (page_mapped(page) || PageCgroupMigration(pc))
> > > +		if (page_mapped(page))
> > > +			goto unlock_out;
> > 
> > Don't need that test or remove the one below (seems easier to read
> > because those cases are really different things).
> > 
> > > +		if (page_mapped(page) || (!end_migration &&
> > > +					  PageCgroupMigration(pc)))
> 
> My bad, I meant to remove this second page_mapped() and forgot.  Will
> fix.  I take it
> 
> 		if (page_mapped(page))
> 			goto unlock_out;
> 		if (!end_migration && PageCgroupMigration(pc))
> 			goto unlock_out;
> 
> is what you had in mind?

Yes

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

--
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>