Re: [PATCH for-7.5 1/5] memcg: move memory.high enforcement out of try_charge_memcg()

[Michal Hocko <mhocko@suse.com>]

On Sat 10-10-26 14:09:53, Shakeel Butt wrote:
> try_charge_memcg() does a lot. It charges the page counters, reclaims
> when a limit is hit, may call the OOM killer, and at the end it checks
> and enforces memory.high. Move the memory.high part into a new function,
> memcg_enforce_high(), so that try_charge_memcg() is shorter and the
> memory.high code is easier to read and change.
> 
> The code and its comments move as they are. The only difference is that
> the new function gets allow_spinning from gfp_mask itself.
> 
> No functional change.

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

> ---
>  mm/memcontrol.c | 125 ++++++++++++++++++++++++++----------------------
>  1 file changed, 69 insertions(+), 56 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index aad0498a7bd6..20c5dfe1a894 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2696,6 +2696,74 @@ void __mem_cgroup_handle_over_high(gfp_t gfp_mask)
>  	css_put(&memcg->css);
>  }
>  
> +/*
> + * Called after @nr_pages were charged to @memcg. If @memcg or one of its
> + * ancestors is over memory.high or memory.swap.high, start reclaim or slow
> + * down the task.
> + */
> +static void memcg_enforce_high(struct mem_cgroup *memcg, unsigned int nr_pages,
> +			       gfp_t gfp_mask)
> +{
> +	bool allow_spinning = gfpflags_allow_spinning(gfp_mask);
> +
> +	/*
> +	 * If the hierarchy is above the normal consumption range, schedule
> +	 * reclaim on returning to userland.  We can perform reclaim here
> +	 * if __GFP_RECLAIM but let's always punt for simplicity and so that
> +	 * GFP_KERNEL can consistently be used during reclaim.  @memcg is
> +	 * not recorded as it most likely matches current's and won't
> +	 * change in the meantime.  As high limit is checked again before
> +	 * reclaim, the cost of mismatch is negligible.
> +	 */
> +	do {
> +		bool mem_high, swap_high;
> +
> +		mem_high = page_counter_read(&memcg->memory) >
> +			READ_ONCE(memcg->memory.high);
> +		swap_high = page_counter_read(&memcg->swap) >
> +			READ_ONCE(memcg->swap.high);
> +
> +		/* Don't bother a random interrupted task */
> +		if (!in_task()) {
> +			if (mem_high) {
> +				if (allow_spinning)
> +					schedule_work(&memcg->high_work);
> +				else
> +					irq_work_queue(&memcg->high_irq_work);
> +				break;
> +			}
> +			continue;
> +		}
> +
> +		if (mem_high || swap_high) {
> +			/*
> +			 * The allocating tasks in this cgroup will need to do
> +			 * reclaim or be throttled to prevent further growth
> +			 * of the memory or swap footprints.
> +			 *
> +			 * Target some best-effort fairness between the tasks,
> +			 * and distribute reclaim work and delay penalties
> +			 * based on how much each task is actually allocating.
> +			 */
> +			current->memcg_nr_pages_over_high += nr_pages;
> +			set_notify_resume(current);
> +			break;
> +		}
> +	} while ((memcg = parent_mem_cgroup(memcg)));
> +
> +	/*
> +	 * Reclaim is set up above to be called from the userland
> +	 * return path. But also attempt synchronous reclaim to avoid
> +	 * excessive overrun while the task is still inside the
> +	 * kernel. If this is successful, the return path will see it
> +	 * when it rechecks the overage and simply bail out.
> +	 */
> +	if (current->memcg_nr_pages_over_high > MEMCG_CHARGE_BATCH &&
> +	    !(current->flags & PF_MEMALLOC) &&
> +	    gfpflags_allow_blocking(gfp_mask))
> +		__mem_cgroup_handle_over_high(gfp_mask);
> +}
> +
>  static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  			    unsigned int nr_pages)
>  {
> @@ -2853,62 +2921,7 @@ static int try_charge_memcg(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  	if (batch > nr_pages)
>  		refill_stock(memcg, batch - nr_pages);
>  
> -	/*
> -	 * If the hierarchy is above the normal consumption range, schedule
> -	 * reclaim on returning to userland.  We can perform reclaim here
> -	 * if __GFP_RECLAIM but let's always punt for simplicity and so that
> -	 * GFP_KERNEL can consistently be used during reclaim.  @memcg is
> -	 * not recorded as it most likely matches current's and won't
> -	 * change in the meantime.  As high limit is checked again before
> -	 * reclaim, the cost of mismatch is negligible.
> -	 */
> -	do {
> -		bool mem_high, swap_high;
> -
> -		mem_high = page_counter_read(&memcg->memory) >
> -			READ_ONCE(memcg->memory.high);
> -		swap_high = page_counter_read(&memcg->swap) >
> -			READ_ONCE(memcg->swap.high);
> -
> -		/* Don't bother a random interrupted task */
> -		if (!in_task()) {
> -			if (mem_high) {
> -				if (allow_spinning)
> -					schedule_work(&memcg->high_work);
> -				else
> -					irq_work_queue(&memcg->high_irq_work);
> -				break;
> -			}
> -			continue;
> -		}
> -
> -		if (mem_high || swap_high) {
> -			/*
> -			 * The allocating tasks in this cgroup will need to do
> -			 * reclaim or be throttled to prevent further growth
> -			 * of the memory or swap footprints.
> -			 *
> -			 * Target some best-effort fairness between the tasks,
> -			 * and distribute reclaim work and delay penalties
> -			 * based on how much each task is actually allocating.
> -			 */
> -			current->memcg_nr_pages_over_high += batch;
> -			set_notify_resume(current);
> -			break;
> -		}
> -	} while ((memcg = parent_mem_cgroup(memcg)));
> -
> -	/*
> -	 * Reclaim is set up above to be called from the userland
> -	 * return path. But also attempt synchronous reclaim to avoid
> -	 * excessive overrun while the task is still inside the
> -	 * kernel. If this is successful, the return path will see it
> -	 * when it rechecks the overage and simply bail out.
> -	 */
> -	if (current->memcg_nr_pages_over_high > MEMCG_CHARGE_BATCH &&
> -	    !(current->flags & PF_MEMALLOC) &&
> -	    gfpflags_allow_blocking(gfp_mask))
> -		__mem_cgroup_handle_over_high(gfp_mask);
> +	memcg_enforce_high(memcg, batch, gfp_mask);
>  	return ret;
>  }
>  
> -- 
> 2.53.0-Meta

-- 
Michal Hocko
SUSE Labs