Re: [PATCHv4 09/39] thp, mm: introduce mapping_can_have_hugepages()predicate

[Kirill A. Shutemov]

Dave Hansen wrote:
> On 05/11/2013 06:23 PM, Kirill A. Shutemov wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
> > 
> > Returns true if mapping can have huge pages. Just check for __GFP_COMP
> > in gfp mask of the mapping for now.
> > 
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > ---
> >  include/linux/pagemap.h |   12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> > index e3dea75..28597ec 100644
> > --- a/include/linux/pagemap.h
> > +++ b/include/linux/pagemap.h
> > @@ -84,6 +84,18 @@ static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask)
> >  				(__force unsigned long)mask;
> >  }
> >  
> > +static inline bool mapping_can_have_hugepages(struct address_space *m)
> > +{
> > +	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE_PAGECACHE)) {
> > +		gfp_t gfp_mask = mapping_gfp_mask(m);
> > +		/* __GFP_COMP is key part of GFP_TRANSHUGE */
> > +		return !!(gfp_mask & __GFP_COMP) &&
> > +			transparent_hugepage_pagecache();
> > +	}
> > +
> > +	return false;
> > +}
> 
> transparent_hugepage_pagecache() already has the same IS_ENABLED()
> check,  Is it really necessary to do it again here?
> 
> IOW, can you do this?
> 
> > +static inline bool mapping_can_have_hugepages(struct address_space
> > +{
> > +		gfp_t gfp_mask = mapping_gfp_mask(m);
> 		if (!transparent_hugepage_pagecache())
> 			return false;
> > +		/* __GFP_COMP is key part of GFP_TRANSHUGE */
> > +		return !!(gfp_mask & __GFP_COMP);
> > +}

Yeah, it's better.

> I know we talked about this in the past, but I've forgotten already.
> Why is this checking for __GFP_COMP instead of GFP_TRANSHUGE?

It's up to filesystem what gfp mask to use. For example ramfs's pages are
not movable currently. So, we check only part which matters.

> Please flesh out the comment.

I'll make the comment in code a bit more descriptive.

> Also, what happens if "transparent_hugepage_flags &
> (1<<TRANSPARENT_HUGEPAGE_PAGECACHE)" becomes false at runtime and you
> have some already-instantiated huge page cache mappings around?  Will
> things like mapping_align_mask() break?

We will not touch existing huge pages in existing VMAs. The userspace can
use them until they will be unmapped or split. It's consistent with anon
THP pages.

If anybody mmap() the file after disabling the feature, we will not
setup huge pages anymore: transparent_hugepage_enabled() check in
handle_mm_fault will fail and the page fill be split.

mapping_align_mask() is part of mmap() call path, so there's only chance
that we will get VMA aligned more strictly then needed. Nothing to worry
about.

-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/