Re: [RFC 1/5] memcg: cleanup charge routines

[Michal Hocko]

On Mon 03-02-14 10:51:27, Johannes Weiner wrote:
> On Mon, Feb 03, 2014 at 02:20:01PM +0100, Michal Hocko wrote:
> > On Thu 30-01-14 12:18:37, Johannes Weiner wrote:
> > > On Tue, Dec 17, 2013 at 04:45:26PM +0100, Michal Hocko wrote:
> > [...]
> > > > -static int __mem_cgroup_try_charge(struct mm_struct *mm,
> > > > -				   gfp_t gfp_mask,
> > > > +static int __mem_cgroup_try_charge_memcg(gfp_t gfp_mask,
> > > >  				   unsigned int nr_pages,
> > > > -				   struct mem_cgroup **ptr,
> > > > +				   struct mem_cgroup *memcg,
> > > >  				   bool oom)
> > > 
> > > Why not keep the __mem_cgroup_try_charge() name?  It's shorter and
> > > just as descriptive.
> > 
> > I wanted to have 2 different names with clear reference to _what_ is
> > going to be charged. But I am always open to naming suggestions.
> >
> > > > +/*
> > > > + * Charges and returns memcg associated with the given mm (or root_mem_cgroup
> > > > + * if mm is NULL). Returns NULL if memcg is under OOM.
> > > > + */
> > > > +static struct mem_cgroup *mem_cgroup_try_charge_mm(struct mm_struct *mm,
> > > > +				   gfp_t gfp_mask,
> > > > +				   unsigned int nr_pages,
> > > > +				   bool oom)
> > > 
> > > We already have a try_get_mem_cgroup_from_mm().
> > >
> > > After this series, this function basically duplicates that and it
> > > would be much cleaner if we only had one try_charge() function and let
> > > all the callers use the appropriate try_get_mem_cgroup_from_wherever()
> > > themselves.
> > 
> > try_get_mem_cgroup_from_mm doesn't charge memory itself. It just tries
> > to get memcg from the given mm. It is called also from a context which
> > doesn't charge any memory (task_in_mem_cgroup). Or have I misunderstood
> > you?
> 
> Your mem_cgroup_try_charge_mm() looks up a memcg from mm and calls
> try_charge().  But we have try_get_mem_cgroup_from_mm() to do the
> first half, so why not have the current callers of
> mem_cgroup_try_charge_mm() just use try_get_mem_cgroup_from_mm() and
> try_charge()?  Why is charging through an mm - as opposed to through a
> page or through the task - special?

OK, I thought it would be easier to follow those two different ways of
charging (known memcg vs. current mm - I needed that when tracking some
issue and ended up quite despair from all the different combinations)
but that is probably not that interesting and what you are proposing
reduces even more code. OK I will play with this some more.

> Most callsites already do the lookups themselves:
> 
> try_charge_swapin:	uses try_get_mem_cgroup_from_page()
> kmem_newpage_charge:	uses try_get_mem_cgroup_from_mm()
> precharge:		uses mem_cgroup_from_task()
> 
> So just let these two do the same:
> 
> newpage_charge:		could use try_get_mem_cgroup_from_mm()
> cache_charge:		could use try_get_mem_cgroup_from_mm()
> 
> And then provide one try_charge() that always takes a non-null memcg.

Thanks!

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/