linux-kernel - Re: [PATCH] mm: warn about allocations which stall for too long

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 23 Sep 2016 17:02:34 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
Cc:     linux-mm@...ck.org, akpm@...ux-foundation.org, hannes@...xchg.org,
        mgorman@...e.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm: warn about allocations which stall for too long

On Fri 23-09-16 23:36:22, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > @@ -3659,6 +3661,15 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >  	else
> >  		no_progress_loops++;
> >  
> > +	/* Make sure we know about allocations which stall for too long */
> > +	if (!(gfp_mask & __GFP_NOWARN) && time_after(jiffies, alloc_start + stall_timeout)) {
> 
> Should we check !__GFP_NOWARN ? I think __GFP_NOWARN is likely used with
> __GFP_NORETRY, and __GFP_NORETRY is already checked by now.
> 
> I think printing warning regardless of __GFP_NOWARN is better because
> this check is similar to hungtask warning.

Well, if the user said to not warn we should really obey that. Why would
that matter?
 
> > +		pr_warn("%s: page alloction stalls for %ums: order:%u mode:%#x(%pGg)\n",
> > +				current->comm, jiffies_to_msecs(jiffies-alloc_start),
> > +				order, gfp_mask, &gfp_mask);
> > +		stall_timeout += 10 * HZ;
> > +		dump_stack();
> 
> Can we move this pr_warn() + dump_stack() to a separate function like
> 
> static void __warn_memalloc_stall(unsigned int order, gfp_t gfp_mask, unsigned long alloc_start)
> {
> 	pr_warn("%s: page alloction stalls for %ums: order:%u mode:%#x(%pGg)\n",
> 		current->comm, jiffies_to_msecs(jiffies-alloc_start),
> 		order, gfp_mask, &gfp_mask);
> 	dump_stack();
> }
> 
> in order to allow SystemTap scripts to perform additional actions by name (e.g.
> 
> # stap -g -e 'probe kernel.function("__warn_memalloc_stall").return { panic(); }

I find this reasoning and the use case really _absurd_, seriously! Pulling
the warning into a separate function might be reasonable regardless,
though. It matches warn_alloc_failed. Also if we find out we need some
rate limitting or more checks it might just turn out being easier to
follow rather than in the middle of an already complicated allocation
slow path. I just do not like that the stall_timeout would have to stay
in the original place or have it an in/out parameter.

> ) rather than by line number, and surround __warn_memalloc_stall() call with
> mutex in order to serialize warning messages because it is possible that
> multiple allocation requests are stalling?

we do not use any lock in warn_alloc_failed so why this should be any
different?

-- 
Michal Hocko
SUSE Labs