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

On Aug 5, 2019, at 7:28 AM, Kirill A. Shutemov [off-list ref] wrote:

quoted

Is there different terminology you'd prefer to see me use here to clarify
this?

My point is that maybe we should just use ~HPAGE_P?D_MASK in code. The new
HPAGE_P?D_OFFSET doesn't add much for readability in my opinion.

Fair enough, I'll make that change.

quoted

OK, I can do that; I didn't want to unnecessarily eliminate the
VM_BUG_ON_PAGE(PageTransHuge(page)) call for everyone given this
is CONFIG experimental code.

If you bring the feature, you bring the feature. Just drop these VM_BUGs.

OK.

I'm not sure. It will be costly comparing to PageTransCompound/Huge as we
need to check more than flags.

To exclude hugetlb pages here, use VM_BUG_ON_PAGE(PageHuge(page), page).
It will allow to catch wrong usage of the function.

That will also do it and that way we will better know if it ever happens,
which of course it shouldn't.

quoted

This routine's main function other than validation is to make sure the page
cache has not been polluted between when we go out to read the large page
and when the page is added to the cache (more on that coming up.) For
example, the way I was able to tell readahead() was polluting future
possible THP mappings is because after a buffered read I would typically see
52 (the readahead size) PAGESIZE pages for the next 2M range in the page
cache.

My point is that you should only see compound pages here if they are
HPAGE_PMD_ORDER, shouldn't you? Any other order of compound page would be
unexpected to say the least.

Yes, compound pages should only be HPAGE_PMD_ORDER.

The routine and the check will need to be updated if we ever can
allocate/cache larger pages.

quoted

It's my understanding that pages in the page cache should be locked, so I
wanted to check for that.

No. They are get locked temporary for some operation, but not all the
time.

OK, thanks for that.

quoted

I don't really care if the start of the VMA is suitable, just whether I can map
the current faulting page with a THP. As far as I know, there's nothing wrong
with mapping all the pages before the VMA hits a properly aligned bound with
PAGESIZE pages and then aligned chunks in the middle with THP.

You cannot map any paged as huge into wrongly aligned VMA.

THP's ->index must be aligned to HPAGE_PMD_NR, so if the combination VMA's
->vm_start and ->vm_pgoff doesn't allow for this, you must fallback to
mapping the page with PTEs. I don't see it handled properly here.

It was my assumption that if say a VMA started at an address say one page
before a large page alignment, you could map that page with a PAGESIZE
page but if VMA size allowed, there was a fault on the next page, and
VMA size allowed, you could map that next range with a large page, taking
taking the approach of mapping chunks of the VMA with the largest page
possible.

Is it that the start of the VMA must always align or that the entire VMA
must be properly aligned and a multiple of the PMD size (so you either map
with all large pages or none)?

quoted

This is the page that content was just read to; readpage() will unlock the page
when it is done with I/O, but the page needs to be locked before it's inserted
into the page cache.

Then you must to lock the page properly with lock_page().

__SetPageLocked() is fine for just allocated pages that was not exposed
anywhere. After ->readpage() it's not the case and it's not safe to use
__SetPageLocked() for them.

In the current code, it's assumed it is not exposed, because a single read
of a large page that does no readahead before the page is inserted into the
cache means there are no external users of the page.

Regardless, I can make this change as part of the changes I will need to to
reorder when the page is inserted into the cache.

quoted

I can make that change; originally alloc_set_pte() didn't use the second
parameter at all when mapping a read-only page.

Even now, if the page isn't writable, it would only be dereferenced by a
VM_BUG_ON_PAGE() call if it's COW.

Please do change this. It has to be future-proof.

OK, thanks.

quoted

My thinking had been if any part of reading a large page and mapping it had
failed, the code could just put_page() the newly allocated page and fallback
to mapping the page with PAGESIZE pages rather than add a THP to the cache.

I think it's must change. We should not allow inconsistent view on page
cache.

Yes, I can see where it would be confusing to anyone looking at it that assumed
the page must already be in the cache before readpage() is called.

quoted

If mprotect() is called, wouldn't the pages be COWed to PAGESIZE pages the
first time the area was written to? I may be way off on this assumption.

Okay, fair enough. COW will happen for private mappings.

But for private mappings you don't need to enforce even RO. All permission
mask should be fine.

Thanks.

quoted

Once again, the question is whether we want to make this just RO or RO + EXEC
to maintain my goal of just mapping program TEXT via THP. I'm willing to
hear arguments either way.

It think the goal is to make feature useful and therefore we need to make
it available for widest possible set of people.

I think is should be allowed for RO (based on how file was opened, not on
PROT_*) + SHARED and for any PRIVATE mappings.

That makes sense.

quoted

I did that because the existing code just blindly sets VM_MAYWRITE and I
obviously didn't want to, so making it a variable allowed me to shut it off
if it was a THP mapping.

I think touching VM_MAYWRITE here is wrong. It should reflect what file
under the mapping allows.

Fair enough.

Thanks again!
    -- Bill