[v6,17/17] mm: Distinguish VMalloc pages

Commit Message

Andrew Morton May 22, 2018, 8:48 p.m. UTC

On Tue, 22 May 2018 13:19:58 -0700 Matthew Wilcox <[e-mail]> wrote:

> On Tue, May 22, 2018 at 10:57:34PM +0300, Andrey Ryabinin wrote:
> > On 05/22/2018 08:58 PM, Matthew Wilcox wrote:
> > > On Tue, May 22, 2018 at 07:10:52PM +0300, Andrey Ryabinin wrote:
> > >> On 05/18/2018 10:45 PM, Matthew Wilcox wrote:
> > >>> From: Matthew Wilcox <[e-mail]>
> > >>>
> > >>> For diagnosing various performance and memory-leak problems, it is helpful
> > >>> to be able to distinguish pages which are in use as VMalloc pages.
> > >>> Unfortunately, we cannot use the page_type field in struct page, as
> > >>> this is in use for mapcount by some drivers which map vmalloced pages
> > >>> to userspace.
> > >>>
> > >>> Use a special page->mapping value to distinguish VMalloc pages from
> > >>> other kinds of pages.  Also record a pointer to the vm_struct and the
> > >>> offset within the area in struct page to help reconstruct exactly what
> > >>> this page is being used for.
> > >>
> > >> This seems useless. page->vm_area and page->vm_offset are never used.
> > >> There are no follow up patches which use this new information 'For diagnosing various performance and memory-leak problems',
> > >> and no explanation how is it can be used in current form.
> > > 
> > > Right now, it's by-hand.  tools/vm/page-types.c will tell you which pages
> > > are allocated to VMalloc.  Many people use kernel debuggers, crashdumps
> > > and similar to examine the kernel's memory.  Leaving these breadcrumbs
> > > is helpful, and those fields simply weren't in use before.
> > > 
> > >> Also, this patch breaks code like this:
> > >> 	if (mapping = page_mapping(page))
> > >> 		// access mapping
> > > 
> > > Example of broken code, please?  Pages allocated from the page allocator
> > > with alloc_page() come with page->mapping == NULL.  This code snippet
> > > would not have granted access to vmalloc pages before.
> > > 
> > 
> > Some implementation of the flush_dcache_page(), also set_page_dirty() can be called
> > on userspace-mapped vmalloc pages during unmap - zap_pte_range() -> set_page_dirty()
> 
> Ah, good catch!  I'm anticipating we'll have other special values for
> page->mapping in the future. so how about this?
> 
> (no changelog because I assume Andrew will add this as a -fix patch)

I give the -fix patches a single-line summary in the final rollup.

> diff --git a/mm/util.c b/mm/util.c
> index 10ca6f1d5c75..be81c9052ef7 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -561,6 +561,8 @@ struct address_space *page_mapping(struct page *page)
>  	mapping = page->mapping;
>  	if ((unsigned long)mapping & PAGE_MAPPING_ANON)
>  		return NULL;
> +	if ((unsigned long)mapping < PAGE_SIZE)
> +		return NULL;
>  
>  	return (void *)((unsigned long)mapping & ~PAGE_MAPPING_FLAGS);
>  }

-ENOCOMMENT ;)

It's a bit sad to put even more stuff into page_mapping() just for
page_types diddling.  Is this really justified?  How many people will
use it, and get significant benefit from it?

Comments

Matthew Wilcox May 22, 2018, 9:45 p.m. UTC | #1

On Tue, May 22, 2018 at 01:48:38PM -0700, Andrew Morton wrote:
> -ENOCOMMENT ;)
> 
> --- a/mm/util.c~mm-distinguish-vmalloc-pages-fix-fix
> +++ a/mm/util.c
> @@ -512,6 +512,8 @@ struct address_space *page_mapping(struc
>  	mapping = page->mapping;
>  	if ((unsigned long)mapping & PAGE_MAPPING_ANON)
>  		return NULL;
> +
> +	/* Don't trip over a vmalloc page's MAPPING_VMalloc cookie */
>  	if ((unsigned long)mapping < PAGE_SIZE)
>  		return NULL;
>  
> It's a bit sad to put even more stuff into page_mapping() just for
> page_types diddling.  Is this really justified?  How many people will
> use it, and get significant benefit from it?

We could leave page->mapping NULL for vmalloc pages.  We just need to
find a spot where we can put a unique identifier.  The first word of
the union looks like a string candidate; bit 0 is already reserved for
PageTail.  The other users are list_head.prev, a struct page *, and
struct dev_pagemap *, so that should work out OK.

If you want to just drop this patch, I'd be OK with that.  I can always
submit it to you again next merge window.

Andrew Morton May 22, 2018, 11:02 p.m. UTC | #2

On Tue, 22 May 2018 14:45:17 -0700 Matthew Wilcox <[e-mail]> wrote:

> On Tue, May 22, 2018 at 01:48:38PM -0700, Andrew Morton wrote:
> > -ENOCOMMENT ;)
> > 
> > --- a/mm/util.c~mm-distinguish-vmalloc-pages-fix-fix
> > +++ a/mm/util.c
> > @@ -512,6 +512,8 @@ struct address_space *page_mapping(struc
> >  	mapping = page->mapping;
> >  	if ((unsigned long)mapping & PAGE_MAPPING_ANON)
> >  		return NULL;
> > +
> > +	/* Don't trip over a vmalloc page's MAPPING_VMalloc cookie */
> >  	if ((unsigned long)mapping < PAGE_SIZE)
> >  		return NULL;
> >  
> > It's a bit sad to put even more stuff into page_mapping() just for
> > page_types diddling.  Is this really justified?  How many people will
> > use it, and get significant benefit from it?
> 
> We could leave page->mapping NULL for vmalloc pages.  We just need to
> find a spot where we can put a unique identifier.  The first word of
> the union looks like a string candidate; bit 0 is already reserved for
> PageTail.  The other users are list_head.prev, a struct page *, and
> struct dev_pagemap *, so that should work out OK.
> 
> If you want to just drop this patch, I'd be OK with that.  I can always
> submit it to you again next merge window.

OK, let's park it for now.

Patch

--- a/mm/util.c~mm-distinguish-vmalloc-pages-fix-fix
+++ a/mm/util.c
@@ -512,6 +512,8 @@  struct address_space *page_mapping(struc
 	mapping = page->mapping;
 	if ((unsigned long)mapping & PAGE_MAPPING_ANON)
 		return NULL;
+
+	/* Don't trip over a vmalloc page's MAPPING_VMalloc cookie */
 	if ((unsigned long)mapping < PAGE_SIZE)
 		return NULL;