THP refcounting in disallowed_hugepage_adjust()?

Hi Paolo et al,

while looking again at the recently added IFU patches I noticed a dicrepancy between the two _hugepage_adjust() functions which doesn't make sense to me yet:

* transparent_hugepage_adjust(), when changing the value of pfn, does a kvm_release_pfn_clean() on the old value and a kvm_get_pfn() on the new value to make sure the code holds the reference to the correct pfn.

* disallowed_hugepage_adjust() also changes the value of the pfn to map, kinda reverses what transparent_hugepage_adjust() did before. But that function does not care about the pfn refcounting.

I was wondering what the reason for that might be, is it just not necessary in disallowed_hugepage_adjust() or is that an oversight?

Regards,

Joerg

The page fault flows don't actually rely on holding a reference to the page once they reach thp_adjust(). At that point, they hold mmu_lock and have verified no invalidation from mmu_notifier have occured since the page reference was acquired.

The release/get pfn dance in transparent_hugepage_adjust() is a quirk of sorts that is necessitated because thp_adjust() modifies the local @pfn variable in FNAME(page_fault)(), nonpaging_map() and tdp_page_fault(). Those functions all call kvm_release_pfn_clean() on @pfn, so thp_adjust() needs to transfer the page reference purely for correctness when the pfn is released.

disallowed_hugepage_adjust() is called from __direct_map() and its modification of the pfn is also contained to __direct_map(), i.e. the updated @pfn doesn't get propagated back up to the fault handlers. Thus, kvm_release_pfn_clean() is called on the original pfn and so there's no need to transfer the page reference.

The above discrepancy can resolved by moving thp_adjust() into FNAME(fetch) and __direct_map() so that the "top-level" @pfn isn't modified. Getting rid of the kvm_get_pfn() call would be a nice side effect. The downside is that @force_pt_level would need to be passed down the call stack.

Hi Jean,

Thanks for your explanation, makes a lot of sense to me now.

Regards,

Joerg