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