Re: [PATCH v2 2/2] mm,thp: Add experimental config option RO_EXEC_FILEMAP_HUGE_FAULT_THP

[William Kucharski <william.kucharski@oracle.com>]

On 7/29/19 4:51 PM, Song Liu wrote:

>
>> +#define	HPAGE_PMD_OFFSET	(HPAGE_PMD_SIZE - 1)
>            ^ space vs. tab difference here.

Thanks, good catch!

> 
>> +#define HPAGE_PMD_MASK		(~(HPAGE_PMD_OFFSET))
>> +
>> +#define HPAGE_PUD_SHIFT		PUD_SHIFT
>> +#define HPAGE_PUD_SIZE		((1UL) << HPAGE_PUD_SHIFT)
>> +#define	HPAGE_PUD_OFFSET	(HPAGE_PUD_SIZE - 1)

Saw this one, too.

> Should HPAGE_PMD_OFFSET and HPAGE_PUD_OFFSET include bits for
> PAGE_OFFSET? I guess we can just keep huge_mm.h as-is and use
> ~HPAGE_PMD_MASK.

That's what I had intended; would you rather see those macros
omit the unneeded for the larger page size bits?

>> - * - FGP_PMD: If FGP_CREAT is specified, attempt to allocate a PMD-sized page.
>> + * - FGP_PMD: If FGP_CREAT is specified, attempt to allocate a PMD-sized page

No - this came in as part of patch 1/2 and I missed dropping the period 
at the end of the line that caused this to be a diff, so I will put it
back. :-)

> We have been using name "xas" for "struct xa_state *". Let's keep using it?

Thanks, done.

>> +	if (unlikely(!(PageCompound(new_page)))) {
> 
>     What condition triggers this case
I wanted a check to make sure that __page_cacke_alloc() returned a large 
page. I don't recall if the mechanism guarantees that when you ask for
a large page, you get one, so I wanted to handle that case.

If you prefer, I could make this a VM_BUG_ON_PAGE() instead, but I
wanted it to fallback gracefully if it can't get a properly sized
page.

>> +#ifndef	COMPOUND_PAGES_HEAD_ONLY
> 
> Where do we define COMPOUND_PAGES_HEAD_ONLY?

At present, we do not.

I used this so I could include the code that would be needed once
Matthew's "store only head pages in page cache" changes go back in,
which looks like it may not be until 5.4-rc1. Matthew recommended I
include this so we didn't lose track of the code change that would be
needed then. I'll be talking to him today about this and the issues
you raised regarding patch 1/2.

Thanks for going through this!!

     -- Bill