[PATCH v7 5/9] arm64: hugetlb: Handle swap entries in huge_pte_offset() for contiguous hugepages

[Julien Thierry]

On 22/08/17 17:18, Punit Agrawal wrote:
> Julien Thierry <julien.thierry at arm.com> writes:
> 
>> On 22/08/17 15:41, Punit Agrawal wrote:
>>> Julien Thierry <julien.thierry at arm.com> writes:
>>>
>>>> Hi Punit,
>>>>
>>>> On 22/08/17 11:42, Punit Agrawal wrote:
>>>>> huge_pte_offset() was updated to correctly handle swap entries for
>>>>> hugepages. With the addition of the size parameter, it is now possible
>>>>> to disambiguate whether the request is for a regular hugepage or a
>>>>> contiguous hugepage.
>>>>>
>>>>> Fix huge_pte_offset() for contiguous hugepages by using the size to find
>>>>> the correct page table entry.
>>>>>
>>>>> Signed-off-by: Punit Agrawal <punit.agrawal at arm.com>
>>>>> Cc: David Woods <dwoods at mellanox.com>
>>>>> ---
>>>>>     arch/arm64/mm/hugetlbpage.c | 21 ++++++++++++++++-----
>>>>>     1 file changed, 16 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>>>>> index 594232598cac..b95e24dc3477 100644
>>>>> --- a/arch/arm64/mm/hugetlbpage.c
>>>>> +++ b/arch/arm64/mm/hugetlbpage.c
>>>>> @@ -214,6 +214,7 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>>>>     	pgd_t *pgd;
>>>>>     	pud_t *pud;
>>>>>     	pmd_t *pmd;
>>>>> +	pte_t *pte;
>>>>>       	pgd = pgd_offset(mm, addr);
>>>>>     	pr_debug("%s: addr:0x%lx pgd:%p\n", __func__, addr, pgd);
>>>>> @@ -221,19 +222,29 @@ pte_t *huge_pte_offset(struct mm_struct *mm,
>>>>>     		return NULL;
>>>>>       	pud = pud_offset(pgd, addr);
>>>>> -	if (pud_none(*pud))
>>>>> +	if (sz != PUD_SIZE && pud_none(*pud))
>>>>>     		return NULL;
>>>>> -	/* swap or huge page */
>>>>> -	if (!pud_present(*pud) || pud_huge(*pud))
>>>>> +	/* hugepage or swap? */
>>>>> +	if (pud_huge(*pud) || !pud_present(*pud))
>>>>>     		return (pte_t *)pud;
>>>>>     	/* table; check the next level */
>>>>>     +	if (sz == CONT_PMD_SIZE)
>>>>> +		addr &= CONT_PMD_MASK;
>>>>> +
>>>>>     	pmd = pmd_offset(pud, addr);
>>>>> -	if (pmd_none(*pmd))
>>>>> +	if (!(sz == PMD_SIZE || sz == CONT_PMD_SIZE) &&
>>>>> +	    pmd_none(*pmd))
>>>>>     		return NULL;
>>>>> -	if (!pmd_present(*pmd) || pmd_huge(*pmd))
>>>>> +	if (pmd_huge(*pmd) || !pmd_present(*pmd))
>>>>>     		return (pte_t *)pmd;
>>>>>     +	if (sz == CONT_PTE_SIZE) {
>>>>> +		pte = pte_offset_kernel(
>>>>> +			pmd, (addr & CONT_PTE_MASK));
>>>>> +		return pte;
>>>>
>>>> Nit: Looks like this is the only place the new variable pte is
>>>> used. Since we don't need to test its value, why not just write:
>>>> 	return pte_offset_kernel(pmd, (addr & CONT_PTE_MASK));
>>>>
>>>> and get rid of the pte variable?
>>>
>>> There is no benefit to getting rid of "pte" other than conciseness of
>>> the patch. Having an explicit identifier helps highlight the level of
>>> the page tables we are accessing.
>>>
>>> And we always want to prioritise readability vs conciseness of the
>>> patch, no?
>>>
>>
>> I agree, but I feel here it is more redundancy than increase of
>> readability, because we know pte_offset_kernel returns the address of
>> a pte, no? (otherwise I feel a comment would fit better than a
>> variable).
>>
>> Also, we end up with a variable declared in one scope where it is not
>> used, and it is referenced in a single inner scope, which seems a bit
>> odd to me. Might make the reader pointlessly wonder where else it is
>> used.
> 
> I would've thought looking at the function makes the variable usage
> quite clear. But I think at this stage we are disagreeing over personal
> preferences rather than any real issues (imho) with the code.
> 
> If you feel strongly about this, I can update the code if there is a
> need for another version. But I am reluctant to send a new version just
> for this change.
> 

Fair enough, it was just nitpicking.

Rest of the patchset looks good from my understanding.

Cheers,

-- 
Julien Thierry