Julien Thierry <julien.thierry@xxxxxxx> writes:
> On 22/08/17 15:41, Punit Agrawal wrote:
>> Julien Thierry <julien.thierry@xxxxxxx> 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@xxxxxxx>
>>>> Cc: David Woods <dwoods@xxxxxxxxxxxx>
>>>> ---
>>>> 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.
Thanks,
Punit
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
