Re: [patch 8/8] memcg: sanitize __mem_cgroup_try_charge() call protocol

[Michal Hocko]

On Wed 12-03-14 15:01:38, Michal Hocko wrote:
> On Tue 11-03-14 21:28:34, Johannes Weiner wrote:
> > Some callsites pass a memcg directly, some callsites pass a mm that
> > first has to be translated to an mm.  This makes for a terrible
> > function interface.
> > 
> > Just push the mm-to-memcg translation into the respective callsites
> > and always pass a memcg to mem_cgroup_try_charge().
> > 
> > Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> 
>    text    data     bss     dec     hex filename
>   39435    5916    4192   49543    c187 mm/memcontrol.o.after
>   40466    5916    4192   50574    c58e mm/memcontrol.o.before

Just to prevent from confusion, .before is before the series is applied
not just this patch. 

Before _this_ patch size looks like this:
   text    data     bss     dec     hex filename
  39898    5916    4192   50006    c356 mm/memcontrol.o

> 1K down very nice. But we can shave off additional ~300B if the the
> common mm charging helper as I suggested before:
> 
>    text    data     bss     dec     hex filename
>   39100    5916    4192   49208    c038 mm/memcontrol.o.mm
> 
> commit 7aa420bc051849d85dcf5a091f3619c6b8e33cfb
> Author: Michal Hocko <mhocko@xxxxxxx>
> Date:   Wed Mar 12 14:59:06 2014 +0100
> 
>     add charge mm helper
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 2d7aa3e784d9..67e01b27a021 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2757,6 +2757,35 @@ bypass:
>  	return -EINTR;
>  }
>  
> +/**
> + * mem_cgroup_try_charge_mm - try charging a mm
> + * @mm: mm_struct to charge
> + * @nr_pages: number of pages to charge
> + * @oom: trigger OOM if reclaim fails
> + *
> + * Returns the charged mem_cgroup associated with the given mm_struct or
> + * NULL the charge failed.
> + */
> +static struct mem_cgroup *mem_cgroup_try_charge_mm(struct mm_struct *mm,
> +				 gfp_t gfp_mask,
> +				 unsigned int nr_pages,
> +				 bool oom)
> +
> +{
> +	struct mem_cgroup *memcg;
> +	int ret;
> +
> +	memcg = get_mem_cgroup_from_mm(mm);
> +	ret = mem_cgroup_try_charge(memcg, gfp_mask, nr_pages, oom);
> +	css_put(&memcg->css);
> +	if (ret == -EINTR)
> +		memcg = root_mem_cgroup;
> +	else if (ret)
> +		memcg = NULL;
> +
> +	return memcg;
> +}
> +
>  /*
>   * Somemtimes we have to undo a charge we got by try_charge().
>   * This function is for that and do uncharge, put css's refcnt.
> @@ -3828,7 +3857,6 @@ int mem_cgroup_newpage_charge(struct page *page,
>  	unsigned int nr_pages = 1;
>  	struct mem_cgroup *memcg;
>  	bool oom = true;
> -	int ret;
>  
>  	if (mem_cgroup_disabled())
>  		return 0;
> @@ -3847,13 +3875,9 @@ int mem_cgroup_newpage_charge(struct page *page,
>  		oom = false;
>  	}
>  
> -	memcg = get_mem_cgroup_from_mm(mm);
> -	ret = mem_cgroup_try_charge(memcg, gfp_mask, nr_pages, oom);
> -	css_put(&memcg->css);
> -	if (ret == -EINTR)
> -		memcg = root_mem_cgroup;
> -	else if (ret)
> -		return ret;
> +	memcg = mem_cgroup_try_charge_mm(mm, gfp_mask, nr_pages, oom);
> +	if (!memcg)
> +		return -ENOMEM;
>  	__mem_cgroup_commit_charge(memcg, page, nr_pages,
>  				   MEM_CGROUP_CHARGE_TYPE_ANON, false);
>  	return 0;
> @@ -3914,15 +3938,10 @@ int mem_cgroup_try_charge_swapin(struct mm_struct *mm, struct page *page,
>  	 */
>  	if (!PageSwapCache(page)) {
>  		struct mem_cgroup *memcg;
> -		int ret;
>  
> -		memcg = get_mem_cgroup_from_mm(mm);
> -		ret = mem_cgroup_try_charge(memcg, gfp_mask, 1, true);
> -		css_put(&memcg->css);
> -		if (ret == -EINTR)
> -			memcg = root_mem_cgroup;
> -		else if (ret)
> -			return ret;
> +		memcg = mem_cgroup_try_charge_mm(mm, gfp_mask, 1, true);
> +		if (!memcg)
> +			return -ENOMEM;
>  		*memcgp = memcg;
>  		return 0;
>  	}
> @@ -3996,13 +4015,9 @@ int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
>  	if (unlikely(!mm))
>  		memcg = root_mem_cgroup;
>  	else {
> -		memcg = get_mem_cgroup_from_mm(mm);
> -		ret = mem_cgroup_try_charge(memcg, gfp_mask, 1, true);
> -		css_put(&memcg->css);
> -		if (ret == -EINTR)
> -			memcg = root_mem_cgroup;
> -		else if (ret)
> -			return ret;
> +		memcg = mem_cgroup_try_charge_mm(mm, gfp_mask, 1, true);
> +		if (!memcg)
> +			return -ENOMEM;
>  	}
>  	__mem_cgroup_commit_charge(memcg, page, 1, type, false);
>  	return 0;
> 
> Anyway to your patch as is. The above can be posted as a separate patch
> or folded in as you prefer.
> 
> Acked-by: Michal Hocko <mhocko@xxxxxxx>
> 
> > ---
> >  mm/memcontrol.c | 184 +++++++++++++++++++++++++-------------------------------
> >  1 file changed, 83 insertions(+), 101 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 4f7192bfa5fa..876598b4505b 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2609,7 +2609,7 @@ static int memcg_cpu_hotplug_callback(struct notifier_block *nb,
> >  }
> >  
> >  
> > -/* See __mem_cgroup_try_charge() for details */
> > +/* See mem_cgroup_try_charge() for details */
> >  enum {
> >  	CHARGE_OK,		/* success */
> >  	CHARGE_RETRY,		/* need to retry but retry is not bad */
> > @@ -2682,45 +2682,34 @@ static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
> >  	return CHARGE_NOMEM;
> >  }
> >  
> > -/*
> > - * __mem_cgroup_try_charge() does
> > - * 1. detect memcg to be charged against from passed *mm and *ptr,
> > - * 2. update res_counter
> > - * 3. call memory reclaim if necessary.
> > - *
> > - * In some special case, if the task is fatal, fatal_signal_pending() or
> > - * has TIF_MEMDIE, this function returns -EINTR while writing root_mem_cgroup
> > - * to *ptr. There are two reasons for this. 1: fatal threads should quit as soon
> > - * as possible without any hazards. 2: all pages should have a valid
> > - * pc->mem_cgroup. If mm is NULL and the caller doesn't pass a valid memcg
> > - * pointer, that is treated as a charge to root_mem_cgroup.
> > - *
> > - * So __mem_cgroup_try_charge() will return
> > - *  0       ...  on success, filling *ptr with a valid memcg pointer.
> > - *  -ENOMEM ...  charge failure because of resource limits.
> > - *  -EINTR  ...  if thread is fatal. *ptr is filled with root_mem_cgroup.
> > +/**
> > + * mem_cgroup_try_charge - try charging a memcg
> > + * @memcg: memcg to charge
> > + * @nr_pages: number of pages to charge
> > + * @oom: trigger OOM if reclaim fails
> >   *
> > - * Unlike the exported interface, an "oom" parameter is added. if oom==true,
> > - * the oom-killer can be invoked.
> > + * Returns 0 if @memcg was charged successfully, -EINTR if the charge
> > + * was bypassed to root_mem_cgroup, and -ENOMEM if the charge failed.
> >   */
> > -static int __mem_cgroup_try_charge(struct mm_struct *mm,
> > -				   gfp_t gfp_mask,
> > -				   unsigned int nr_pages,
> > -				   struct mem_cgroup **ptr,
> > -				   bool oom)
> > +static int mem_cgroup_try_charge(struct mem_cgroup *memcg,
> > +				 gfp_t gfp_mask,
> > +				 unsigned int nr_pages,
> > +				 bool oom)
> >  {
> >  	unsigned int batch = max(CHARGE_BATCH, nr_pages);
> >  	int nr_oom_retries = MEM_CGROUP_RECLAIM_RETRIES;
> > -	struct mem_cgroup *memcg = NULL;
> >  	int ret;
> >  
> > +	if (mem_cgroup_is_root(memcg))
> > +		goto done;
> >  	/*
> > -	 * Unlike gloval-vm's OOM-kill, we're not in memory shortage
> > -	 * in system level. So, allow to go ahead dying process in addition to
> > -	 * MEMDIE process.
> > +	 * Unlike in global OOM situations, memcg is not in a physical
> > +	 * memory shortage.  Allow dying and OOM-killed tasks to
> > +	 * bypass the last charges so that they can exit quickly and
> > +	 * free their memory.
> >  	 */
> > -	if (unlikely(test_thread_flag(TIF_MEMDIE)
> > -		     || fatal_signal_pending(current)))
> > +	if (unlikely(test_thread_flag(TIF_MEMDIE) ||
> > +		     fatal_signal_pending(current)))
> >  		goto bypass;
> >  
> >  	if (unlikely(task_in_memcg_oom(current)))
> > @@ -2729,14 +2718,6 @@ static int __mem_cgroup_try_charge(struct mm_struct *mm,
> >  	if (gfp_mask & __GFP_NOFAIL)
> >  		oom = false;
> >  again:
> > -	if (*ptr) { /* css should be a valid one */
> > -		memcg = *ptr;
> > -		css_get(&memcg->css);
> > -	} else {
> > -		memcg = get_mem_cgroup_from_mm(mm);
> > -	}
> > -	if (mem_cgroup_is_root(memcg))
> > -		goto done;
> >  	if (consume_stock(memcg, nr_pages))
> >  		goto done;
> >  
> > @@ -2744,10 +2725,8 @@ again:
> >  		bool invoke_oom = oom && !nr_oom_retries;
> >  
> >  		/* If killed, bypass charge */
> > -		if (fatal_signal_pending(current)) {
> > -			css_put(&memcg->css);
> > +		if (fatal_signal_pending(current))
> >  			goto bypass;
> > -		}
> >  
> >  		ret = mem_cgroup_do_charge(memcg, gfp_mask, batch,
> >  					   nr_pages, invoke_oom);
> > @@ -2756,17 +2735,12 @@ again:
> >  			break;
> >  		case CHARGE_RETRY: /* not in OOM situation but retry */
> >  			batch = nr_pages;
> > -			css_put(&memcg->css);
> > -			memcg = NULL;
> >  			goto again;
> >  		case CHARGE_WOULDBLOCK: /* !__GFP_WAIT */
> > -			css_put(&memcg->css);
> >  			goto nomem;
> >  		case CHARGE_NOMEM: /* OOM routine works */
> > -			if (!oom || invoke_oom) {
> > -				css_put(&memcg->css);
> > +			if (!oom || invoke_oom)
> >  				goto nomem;
> > -			}
> >  			nr_oom_retries--;
> >  			break;
> >  		}
> > @@ -2775,16 +2749,11 @@ again:
> >  	if (batch > nr_pages)
> >  		refill_stock(memcg, batch - nr_pages);
> >  done:
> > -	css_put(&memcg->css);
> > -	*ptr = memcg;
> >  	return 0;
> >  nomem:
> > -	if (!(gfp_mask & __GFP_NOFAIL)) {
> > -		*ptr = NULL;
> > +	if (!(gfp_mask & __GFP_NOFAIL))
> >  		return -ENOMEM;
> > -	}
> >  bypass:
> > -	*ptr = root_mem_cgroup;
> >  	return -EINTR;
> >  }
> >  
> > @@ -2983,20 +2952,17 @@ static int mem_cgroup_slabinfo_read(struct seq_file *m, void *v)
> >  static int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, u64 size)
> >  {
> >  	struct res_counter *fail_res;
> > -	struct mem_cgroup *_memcg;
> >  	int ret = 0;
> >  
> >  	ret = res_counter_charge(&memcg->kmem, size, &fail_res);
> >  	if (ret)
> >  		return ret;
> >  
> > -	_memcg = memcg;
> > -	ret = __mem_cgroup_try_charge(NULL, gfp, size >> PAGE_SHIFT,
> > -				      &_memcg, oom_gfp_allowed(gfp));
> > -
> > +	ret = mem_cgroup_try_charge(memcg, gfp, size >> PAGE_SHIFT,
> > +				    oom_gfp_allowed(gfp));
> >  	if (ret == -EINTR)  {
> >  		/*
> > -		 * __mem_cgroup_try_charge() chosed to bypass to root due to
> > +		 * mem_cgroup_try_charge() chosed to bypass to root due to
> >  		 * OOM kill or fatal signal.  Since our only options are to
> >  		 * either fail the allocation or charge it to this cgroup, do
> >  		 * it as a temporary condition. But we can't fail. From a
> > @@ -3006,7 +2972,7 @@ static int memcg_charge_kmem(struct mem_cgroup *memcg, gfp_t gfp, u64 size)
> >  		 *
> >  		 * This condition will only trigger if the task entered
> >  		 * memcg_charge_kmem in a sane state, but was OOM-killed during
> > -		 * __mem_cgroup_try_charge() above. Tasks that were already
> > +		 * mem_cgroup_try_charge() above. Tasks that were already
> >  		 * dying when the allocation triggers should have been already
> >  		 * directed to the root cgroup in memcontrol.h
> >  		 */
> > @@ -3858,8 +3824,8 @@ out:
> >  int mem_cgroup_newpage_charge(struct page *page,
> >  			      struct mm_struct *mm, gfp_t gfp_mask)
> >  {
> > -	struct mem_cgroup *memcg = NULL;
> >  	unsigned int nr_pages = 1;
> > +	struct mem_cgroup *memcg;
> >  	bool oom = true;
> >  	int ret;
> >  
> > @@ -3880,8 +3846,12 @@ int mem_cgroup_newpage_charge(struct page *page,
> >  		oom = false;
> >  	}
> >  
> > -	ret = __mem_cgroup_try_charge(mm, gfp_mask, nr_pages, &memcg, oom);
> > -	if (ret == -ENOMEM)
> > +	memcg = get_mem_cgroup_from_mm(mm);
> > +	ret = mem_cgroup_try_charge(memcg, gfp_mask, nr_pages, oom);
> > +	css_put(&memcg->css);
> > +	if (ret == -EINTR)
> > +		memcg = root_mem_cgroup;
> > +	else if (ret)
> >  		return ret;
> >  	__mem_cgroup_commit_charge(memcg, page, nr_pages,
> >  				   MEM_CGROUP_CHARGE_TYPE_ANON, false);
> > @@ -3899,7 +3869,7 @@ static int __mem_cgroup_try_charge_swapin(struct mm_struct *mm,
> >  					  gfp_t mask,
> >  					  struct mem_cgroup **memcgp)
> >  {
> > -	struct mem_cgroup *memcg;
> > +	struct mem_cgroup *memcg = NULL;
> >  	struct page_cgroup *pc;
> >  	int ret;
> >  
> > @@ -3912,31 +3882,29 @@ static int __mem_cgroup_try_charge_swapin(struct mm_struct *mm,
> >  	 * in turn serializes uncharging.
> >  	 */
> >  	if (PageCgroupUsed(pc))
> > -		return 0;
> > -	if (!do_swap_account)
> > -		goto charge_cur_mm;
> > -	memcg = try_get_mem_cgroup_from_page(page);
> > +		goto out;
> > +	if (do_swap_account)
> > +		memcg = try_get_mem_cgroup_from_page(page);
> >  	if (!memcg)
> > -		goto charge_cur_mm;
> > -	*memcgp = memcg;
> > -	ret = __mem_cgroup_try_charge(NULL, mask, 1, memcgp, true);
> > +		memcg = get_mem_cgroup_from_mm(mm);
> > +	ret = mem_cgroup_try_charge(memcg, mask, 1, true);
> >  	css_put(&memcg->css);
> >  	if (ret == -EINTR)
> > -		ret = 0;
> > -	return ret;
> > -charge_cur_mm:
> > -	ret = __mem_cgroup_try_charge(mm, mask, 1, memcgp, true);
> > -	if (ret == -EINTR)
> > -		ret = 0;
> > -	return ret;
> > +		memcg = root_mem_cgroup;
> > +	else if (ret)
> > +		return ret;
> > +out:
> > +	*memcgp = memcg;
> > +	return 0;
> >  }
> >  
> >  int mem_cgroup_try_charge_swapin(struct mm_struct *mm, struct page *page,
> >  				 gfp_t gfp_mask, struct mem_cgroup **memcgp)
> >  {
> > -	*memcgp = NULL;
> > -	if (mem_cgroup_disabled())
> > +	if (mem_cgroup_disabled()) {
> > +		*memcgp = NULL;
> >  		return 0;
> > +	}
> >  	/*
> >  	 * A racing thread's fault, or swapoff, may have already
> >  	 * updated the pte, and even removed page from swap cache: in
> > @@ -3944,12 +3912,18 @@ int mem_cgroup_try_charge_swapin(struct mm_struct *mm, struct page *page,
> >  	 * there's also a KSM case which does need to charge the page.
> >  	 */
> >  	if (!PageSwapCache(page)) {
> > +		struct mem_cgroup *memcg;
> >  		int ret;
> >  
> > -		ret = __mem_cgroup_try_charge(mm, gfp_mask, 1, memcgp, true);
> > +		memcg = get_mem_cgroup_from_mm(mm);
> > +		ret = mem_cgroup_try_charge(memcg, gfp_mask, 1, true);
> > +		css_put(&memcg->css);
> >  		if (ret == -EINTR)
> > -			ret = 0;
> > -		return ret;
> > +			memcg = root_mem_cgroup;
> > +		else if (ret)
> > +			return ret;
> > +		*memcgp = memcg;
> > +		return 0;
> >  	}
> >  	return __mem_cgroup_try_charge_swapin(mm, page, gfp_mask, memcgp);
> >  }
> > @@ -3996,8 +3970,8 @@ void mem_cgroup_commit_charge_swapin(struct page *page,
> >  int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
> >  				gfp_t gfp_mask)
> >  {
> > -	struct mem_cgroup *memcg = NULL;
> >  	enum charge_type type = MEM_CGROUP_CHARGE_TYPE_CACHE;
> > +	struct mem_cgroup *memcg;
> >  	int ret;
> >  
> >  	if (mem_cgroup_disabled())
> > @@ -4005,23 +3979,32 @@ int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
> >  	if (PageCompound(page))
> >  		return 0;
> >  
> > -	if (!PageSwapCache(page)) {
> > -		/*
> > -		 * Page cache insertions can happen without an actual
> > -		 * task context, e.g. during disk probing on boot.
> > -		 */
> > -		if (!mm)
> > -			memcg = root_mem_cgroup;
> > -		ret = __mem_cgroup_try_charge(mm, gfp_mask, 1, &memcg, true);
> > -		if (ret != -ENOMEM)
> > -			__mem_cgroup_commit_charge(memcg, page, 1, type, false);
> > -	} else { /* page is swapcache/shmem */
> > +	if (PageSwapCache(page)) { /* shmem */
> >  		ret = __mem_cgroup_try_charge_swapin(mm, page,
> >  						     gfp_mask, &memcg);
> > -		if (!ret)
> > -			__mem_cgroup_commit_charge_swapin(page, memcg, type);
> > +		if (ret)
> > +			return ret;
> > +		__mem_cgroup_commit_charge_swapin(page, memcg, type);
> > +		return 0;
> >  	}
> > -	return ret;
> > +
> > +	/*
> > +	 * Page cache insertions can happen without an actual mm
> > +	 * context, e.g. during disk probing on boot.
> > +	 */
> > +	if (unlikely(!mm))
> > +		memcg = root_mem_cgroup;
> > +	else {
> > +		memcg = get_mem_cgroup_from_mm(mm);
> > +		ret = mem_cgroup_try_charge(memcg, gfp_mask, 1, true);
> > +		css_put(&memcg->css);
> > +		if (ret == -EINTR)
> > +			memcg = root_mem_cgroup;
> > +		else if (ret)
> > +			return ret;
> > +	}
> > +	__mem_cgroup_commit_charge(memcg, page, 1, type, false);
> > +	return 0;
> >  }
> >  
> >  static void mem_cgroup_do_uncharge(struct mem_cgroup *memcg,
> > @@ -6635,8 +6618,7 @@ one_by_one:
> >  			batch_count = PRECHARGE_COUNT_AT_ONCE;
> >  			cond_resched();
> >  		}
> > -		ret = __mem_cgroup_try_charge(NULL,
> > -					GFP_KERNEL, 1, &memcg, false);
> > +		ret = mem_cgroup_try_charge(memcg, GFP_KERNEL, 1, false);
> >  		if (ret)
> >  			/* mem_cgroup_clear_mc() will do uncharge later */
> >  			return ret;
> > -- 
> > 1.9.0
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe cgroups" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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