Re: [PATCH 1/1] mm/hugetlb: Make huge_pte_offset() consistent and document behaviour

[Punit Agrawal]

Mike Kravetz <mike.kravetz@xxxxxxxxxx> writes:

> On 07/26/2017 06:34 AM, Punit Agrawal wrote:
>> Michal Hocko <mhocko@xxxxxxxxxx> writes:
>> 
>>> On Wed 26-07-17 14:33:57, Michal Hocko wrote:
>>>> On Wed 26-07-17 13:11:46, Punit Agrawal wrote:
>>> [...]
>>>>> I've been running tests from mce-test suite and libhugetlbfs for similar
>>>>> changes we did on arm64. There could be assumptions that were not
>>>>> exercised but I'm not sure how to check for all the possible usages.
>>>>>
>>>>> Do you have any other suggestions that can help improve confidence in
>>>>> the patch?
>>>>
>>>> Unfortunatelly I don't. I just know there were many subtle assumptions
>>>> all over the place so I am rather careful to not touch the code unless
>>>> really necessary.
>>>>
>>>> That being said, I am not opposing your patch.
>>>
>>> Let me be more specific. I am not opposing your patch but we should
>>> definitely need more reviewers to have a look. I am not seeing any
>>> immediate problems with it but I do not see a large improvements either
>>> (slightly less nightmare doesn't make me sleep all that well ;)). So I
>>> will leave the decisions to others.
>> 
>> I hear you - I'd definitely appreciate more eyes on the code change and
>> description.
>
> I like the change in semantics for the routine.  Like you, I examined all
> callers of huge_pte_offset() and it appears that they will not be impacted
> by your change.
>
> My only concern is that arch specific versions of huge_pte_offset, may
> not (yet) follow the new semantic.  Someone could potentially introduce
> a new huge_pte_offset call and depend on the new 'documented' semantics.
> Yet, an unmodified arch specific version of huge_pte_offset might have
> different semantics.  I have not reviewed all the arch specific instances
> of the routine to know if this is even possible.  Just curious if you
> examined these, or perhaps you think this is not an issue?

>From checking through the implementations of huge_pte_offset()
architectures, the change shouldn't break anything. (I also cc'd the
posting to linux-arch for architecture maintainers to take more notice).

This is because existing users actively deal with the different returned
values (NULL, huge pte_t*, swap pte_t*) and are not checking explicitly
for pmd or pud.

Guarding against future users is more tricky - it would definitely help
to align all the implementations.