lkml.org 
[lkml]   [2018]   [Jun]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] mm: drop VM_BUG_ON from __get_free_pages
On Tue, 26 Jun 2018 15:57:39 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:

> On 06/22/2018 06:28 PM, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> >
> > There is no real reason to blow up just because the caller doesn't know
> > that __get_free_pages cannot return highmem pages. Simply fix that up
> > silently. Even if we have some confused users such a fixup will not be
> > harmful.
> >
>
> ...
>
> > /*
> > - * Common helper functions.
> > + * Common helper functions. Never use with __GFP_HIGHMEM because the returned
> > + * address cannot represent highmem pages. Use alloc_pages and then kmap if
> > + * you need to access high mem.
> > */
> > unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
> > {
> > struct page *page;
> >
> > - /*
> > - * __get_free_pages() returns a virtual address, which cannot represent
> > - * a highmem page
> > - */
> > - VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
> > -
> > page = alloc_pages(gfp_mask, order);
>
> The previous version had also replaced the line above with:
>
> + page = alloc_pages(gfp_mask & ~__GFP_HIGHMEM, order);
>
> This one doesn't, yet you say "fix that up silently". Bug?
>

This reminds me what is irritating about the patch. We're adding
additional code to a somewhat fast path to handle something which we
know never happens, thanks to the now-removed check.

This newly-added code might become functional in the future, if people
add incorrect callers. Callers whose incorrectness would have been
revealed by the now-removed check!

So.. argh.

Really, the changelog isn't right. There *is* a real reason to blow
up. Effectively the caller is attempting to obtain the virtual address
of a highmem page without having kmapped it first. That's an outright
bug.


An alternative might be to just accept the bogus __GFP_HIGHMEM, let
page_to_virt() return a crap address and wait for the user bug reports
to come in when someone tries to run the offending code on a highmem
machine. That shouldn't take too long - the page allocator will prefer
to return a highmem page in this case.

And adding a rule to the various static checkers should catch most
offenders.

Or just leave the ode as it is now.

\
 
 \ /
  Last update: 2018-06-26 19:05    [W:0.059 / U:0.240 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site