Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 9 Mar 2021 17:32:08 +0100
From:   Michal Hocko <mhocko@suse.com>
To:     Minchan Kim <minchan@kernel.org>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        linux-mm <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>,
        John Dias <joaodias@google.com>,
        David Hildenbrand <david@redhat.com>,
        Jason Baron <jbaron@akamai.com>
Subject: Re: [PATCH v2] mm: page_alloc: dump migrate-failed pages
Message-ID: <YEejCP5tzUtrAjcw@dhcp22.suse.cz>
References: <20210308202047.1903802-1-minchan@kernel.org>
 <YEdAw6gnp9XxoWUQ@dhcp22.suse.cz>
 <YEefLYiX6rF3Uk4E@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YEefLYiX6rF3Uk4E@google.com>
Precedence: bulk

On Tue 09-03-21 08:15:41, Minchan Kim wrote:
> On Tue, Mar 09, 2021 at 10:32:51AM +0100, Michal Hocko wrote:
> > On Mon 08-03-21 12:20:47, 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.
> > 
> > I disagree with this statement. alloc_contig_range is not a reliable
> > allocator. Any user, be it CMA or direct users of alloc_contig_range
> > have to deal with allocation failures. Debugging information can be
> > still useful but considering migration failures critical is
> > overstatement to say the least.
> 
> Fair enough. Let's change it.
> 
> "Currently, debugging CMA allocation failure is too hard
> due to lacking of page information. alloc_contig_range is
> proper place to dump them since it has migrate-failed page
> list."

"Currently, debugging CMA allocation failures is quite limited. The most
commong source of these failures seems to be page migration which
doesn't provide any useful information on the reason of the failure by
itself. alloc_contig_range can report those failures as it holds a list
of migrate-failed pages."
 
> > > page refcount, mapcount with page flags on dump_page are
> > > helpful information to deduce the culprit. Furthermore,
> > > dump_page_owner was super helpful to find long term pinner
> > > who initiated the page allocation.
> > > 
> > > Admin could enable the dump like this(by default, disabled)
> > > 
> > > 	echo "func dump_migrate_failure_pages +p" > control
> > > 
> > > Admin could disable it.
> > > 
> > > 	echo "func dump_migrate_failure_pages =_" > control
> > 
> > My original idea was to add few pr_debug and -DDYNAMIC_DEBUG_MODULE for
> > page_alloc.c. It makes sense to enable a whole bunch at once though.
> > The naming should better reflect this is alloc_contig_rage related
> > because the above sounds like a generic migration failure thing.
> 
> alloc_contig_dump_pages?

Yes this is more clear.

> > Somebody more familiar with the dynamic debugging infrastructure needs
> > to have a look but from from a quick look it seems ok.
> > 
> > Do we really need all the ugly ifdefery, though? Don't we want to have
> > this compiled in all the time and just rely on the static branch managed
> > by the dynamic debugging framework?
> 
> I have no further idea to make it simple while we keep the flexibility
> for arguments and print format.
> 
> #if defined(CONFIG_DYNAMIC_DEBUG) || \
>         (defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
> static void alloc_contig_dump_pages(struct list_head *page_list)
> {
>         static DEFINE_RATELIMIT_STATE(_rs,
>                                         DEFAULT_RATELIMIT_INTERVAL,
>                                         DEFAULT_RATELIMIT_BURST);
> 
>         DEFINE_DYNAMIC_DEBUG_METADATA(descriptor,
>                         "migrate failure");
>         if (DYNAMIC_DEBUG_BRANCH(descriptor) && __ratelimit(&_rs)) {
>                 struct page *page;
> 
>                 WARN(1, "failed callstack");
>                 list_for_each_entry(page, page_list, lru)
>                         dump_page(page, "migration failure");
>         }
> }
> #else
> static inline void alloc_contig_dump_pages(struct list_head *page_list)
> {
> }
> #endif

First, you would be much better off by droping the rate limitting. I am
nt really convinced this is really necessary as this is a debugging aid
enabled on request. A single list can be large enough to swamp logs so
why bother?

Also are all those CONFIG_DYNAMIC_DEBUG* ifdefs necessary?  Can we
simply enable DYNAMIC_DEBUG for page_alloc as I've suggested above?
-- 
Michal Hocko
SUSE Labs