linux-kernel - Re: [PATCH] mm: be more verbose for alloc_contig

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]

Message-ID: <YD50pcPuwV456vwm@google.com>
Date:   Tue, 2 Mar 2021 09:23:49 -0800
From:   Minchan Kim <minchan@...nel.org>
To:     Michal Hocko <mhocko@...e.com>
Cc:     David Hildenbrand <david@...hat.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-mm <linux-mm@...ck.org>,
        LKML <linux-kernel@...r.kernel.org>, joaodias@...gle.com
Subject: Re: [PATCH] mm: be more verbose for alloc_contig_range faliures

On Fri, Feb 19, 2021 at 10:28:12AM +0100, Michal Hocko wrote:
> On Thu 18-02-21 08:19:50, Minchan Kim wrote:
> > On Thu, Feb 18, 2021 at 10:43:21AM +0100, David Hildenbrand wrote:
> > > On 18.02.21 10:35, Michal Hocko wrote:
> > > > On Thu 18-02-21 10:02:43, David Hildenbrand wrote:
> > > > > On 18.02.21 09:56, Michal Hocko wrote:
> > > > > > On Wed 17-02-21 08:36:03, Minchan Kim wrote:
> > > > > > > alloc_contig_range is usually used on cma area or movable zone.
> > > > > > > It's critical if the page migration fails on those areas so
> > > > > > > dump more debugging message like memory_hotplug unless user
> > > > > > > specifiy __GFP_NOWARN.
> > > > > > 
> > > > > > I agree with David that this has a potential to generate a lot of output
> > > > > > and it is not really clear whether it is worth it. Page isolation code
> > > > > > already has REPORT_FAILURE mode which currently used only for the memory
> > > > > > hotplug because this was just too noisy from the CMA path - d381c54760dc
> > > > > > ("mm: only report isolation failures when offlining memory").
> > > > > > 
> > > > > > Maybe migration failures are less likely to fail but still.
> > > > > 
> > > > > Side note: I really dislike that uncontrolled error reporting on memory
> > > > > offlining path we have enabled as default. Yeah, it might be useful for
> > > > > ZONE_MOVABLE in some cases, but otherwise it's just noise.
> > > > > 
> > > > > Just do a "sudo stress-ng --memhotplug 1" and see the log getting flooded
> > > > 
> > > > Anyway we can discuss this in a separate thread but I think this is not
> > > > a representative workload.
> > > 
> > > Sure, but the essence is "this is noise", and we'll have more noise on
> > > alloc_contig_range() as we see these calls more frequently. There should be
> > > an explicit way to enable such *debug* messages.
> > 
> > alloc_contig_range already has gfp_mask and it respects __GFP_NOWARN.
> > Why shouldn't people use it if they don't care the failure?
> > Semantically, it makes sense to me.

Sorry for the late response.

> 
> Well, alloc_contig_range doesn't really have to implement all the gfp
> flags. This is a matter of practicality. alloc_contig_range is quite
> different from the page allocator because it is to be expected that it
> can fail the request. This is avery optimistic allocation request. That
> would suggest that complaining about allocation failures is rather
> noisy.

That was why I'd like to approach for per-call site indicator with
__GFP_NOWARN. Even though it was allocation from CMA, some of them
wouldn't be critical for the failure so those wouldn't care of
the failure. cma_alloc already has carried on "bool no_warn"
which was changed into gfp_t recently. What alloc_contig_range
should do is to take care of the request.

> 
> Now I do understand that some users would like to see why those
> allocations have failed. The question is whether that information is
> generally useful or it is more of a debugging aid. The amount of
> information is also an important aspect. It would be rather unfortunate
> to dump thousands of pages just because they cannot be migrated.

Totally, agree dumping thounds of pages as debugging aid are bad.
Couldn't we simply ratelimit them like other places?

> 
> I do not have a strong opinion here. We can make all alloc_contig_range
> users use GFP_NOWARN by default and only skip the flag from the cma
> allocator but I am slowly leaning towards (ab)using dynamic debugging

I agree the rest of the places are GFP_NOWARN by default except CMA
if they expect alloc_contig_range are optimistic allocation request.
However, I'd like to tweak it for CMA - accept gfp_t from cma_alloc
and take care of the __GFP_NOWARN since some sites of CMA could be
fault tolerant so no need to get the warning.

> infrastructure for this.

dynamic debugging is system wide flag so how to deal with if we
want to see specific alloation faliure, not whole callsites?
That's why I'd like to go with per-call site approach, still.