Re: [patch 03/12] mm: huge_memory: use GFP_TRANSHUGE when charging huge pages

[Johannes Weiner <hannes@cmpxchg.org>]

On Tue, Jun 17, 2014 at 04:23:17PM +0200, Michal Hocko wrote:
> On Mon 16-06-14 15:54:23, Johannes Weiner wrote:
> > Transparent huge page charges prefer falling back to regular pages
> > rather than spending a lot of time in direct reclaim.
> > 
> > Desired reclaim behavior is usually declared in the gfp mask, but THP
> > charges use GFP_KERNEL and then rely on the fact that OOM is disabled
> > for THP charges, and that OOM-disabled charges currently skip reclaim.
> > Needless to say, this is anything but obvious and quite error prone.
> > 
> > Convert THP charges to use GFP_TRANSHUGE instead, which implies
> > __GFP_NORETRY, to indicate the low-latency requirement.
> 
> Maybe we can get one step further and even get rid of oom parameter.
> It is only THP (handled by this patch) and mem_cgroup_do_precharge that
> want OOM disabled explicitly.

Great idea!

> GFP_KERNEL & (~__GFP_NORETRY) is ugly and something like GFP_NO_OOM
> would be better but this is just a quick scratch.

I think it's fine, actually.

> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 52550bbff1ef..5d247822b03a 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2555,15 +2555,13 @@ static int memcg_cpu_hotplug_callback(struct notifier_block *nb,
>   * 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
>   *
>   * 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 mem_cgroup *memcg,
>  				 gfp_t gfp_mask,
> -				 unsigned int nr_pages,
> -				 bool oom)
> +				 unsigned int nr_pages)
>  {
>  	unsigned int batch = max(CHARGE_BATCH, nr_pages);
>  	int nr_retries = MEM_CGROUP_RECLAIM_RETRIES;
> @@ -2647,7 +2645,7 @@ retry:
>  	if (fatal_signal_pending(current))
>  		goto bypass;
>  
> -	if (!oom)
> +	if (!oom_gfp_allowed(gfp_mask))
>  		goto nomem;

We don't actually need that check: if __GFP_NORETRY is set, we goto
nomem directly after reclaim fails and don't even reach here.

So here is the patch I have now - can I get your sign-off on this?

---