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

[Song Liu <songliubraving@fb.com>]

> On Jul 30, 2019, at 7:11 AM, William Kucharski <william.kucharski@oracle.com> wrote:
> 
> 
> 
> 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?

I think using ~HPAGE_PMD_MASK is common practice. Let's keep it 
that way. 

> 
>>> - * - 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.

I think __page_cache_alloc() guarantees compound page. If not, it
should return NULL. 

> 
>>> +#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

We don't have to wait until 5.4-rc1. We could develop based on this 
patch once it lands in mm tree. 

Thanks,
Song