Re: [PATCH] mm/huge_memory: let special huge VMAs bypass the THP policy check

"David Hildenbrand (Arm)" <[email protected]>
Newsgroups gmane.linux.kernel,gmane.linux.kernel.mm,gmane.linux.kernel.stable
Message-ID <[email protected]>
>>
>> +static bool file_vma_honors_thp_toggles(struct vm_area_struct *vma,> +       
>> enum tva_type type)
>> +{
>> +    const bool forced_collapse = type == TVA_FORCED_COLLAPSE;
>> +
>> +    if (forced_collapse)
>> +        return false;
>> +    /* Huge PFN mappings allocate no folios so the policy doesn't apply. */
> 
> So, may be rename the routine to file_vma_thp_policy_applies() ?

Also a possibility. I'll let Lorenzo handle this.

> 
>> +    return !(vma_test(vma, VMA_PFNMAP_BIT) && vma->vm_ops->huge_fault);
>> +}
>> +
>> +static bool vma_thp_toggles_enabled(struct vm_area_struct *vma,
> 
> This routine could use the same 'file_vma_' prefix.

It's true even for non-file vmas.

> 
>> +        vm_flags_t vm_flags)
>> +{
>> +    /* THP=always? */
>> +    if (hugepage_global_always())
>> +        return true;
>> +    /* THP=madvise and actually advised? */
>> +    return hugepage_global_enabled() && vm_flags & VM_HUGEPAGE;
> 
> I would add extra parentheses around 'vm_flags & VM_HUGEPAGE'

It's even required and the compiler might complain :)

> 
>> +}
>> +
>> +static bool file_vma_forces_order_0(struct vm_area_struct *vma,
>> +        vm_flags_t vm_flags, enum tva_type type)
>> +{
>> +    const bool in_pf = type == TVA_PAGEFAULT;
>> +    const bool smaps = type == TVA_SMAPS;
>> +
>> +    /*
>> +     * Enforce THP collapse requirements as necessary. Anonymous vmas
>> +     * were already handled in thp_vma_allowable_orders().
>> +     */
>> +
>> +    if (file_vma_honors_thp_toggles(vma, type) &&
>> +        !vma_thp_toggles_enabled(vma, vm_flags))
>> +        return true;
>> +
>> +    /*
>> +     * Trust that ->huge_fault() handlers know what they are doing
>> +     * in fault path.
>> +     */
>> +    if (((in_pf || smaps)) && vma->vm_ops->huge_fault)
> 
> and there remove the extra parentheses.

Agreed, that was already there in existing code.

Anyhow, it was mostly for inspiration for Lorenzo. I should start spelling out
more clearly in the future that this is just a quick prototype, because recently
people assumed that some quick stuff I post inline would be complete or even
tested :)

-- 
Cheers,

David
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.