[PATCH] drm/amdgpu: Fix minmax error

[James Zhu]

On 2022-11-25 14:42, Luben Tuikov wrote:
> On 2022-11-25 04:57, Christian König wrote:
>>
>> Am 25.11.22 um 09:33 schrieb Luben Tuikov:
>>> On 2022-11-25 02:59, Christian König wrote:
>>>> Am 25.11.22 um 08:56 schrieb Luben Tuikov:
>>>>> On 2022-11-25 02:45, Christian König wrote:
>>>>>> Am 24.11.22 um 22:19 schrieb Luben Tuikov:
>>>>>>> Fix minmax compilation error by using min_t()/max_t(), of the assignment type.
>>>>>>>
>>>>>>> Cc: James Zhu <James.Zhu at amd.com>
>>>>>>> Cc: Felix Kuehling <Felix.Kuehling at amd.com>
>>>>>>> Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory")
>>>>>>> Signed-off-by: Luben Tuikov <luben.tuikov at amd.com>
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c | 10 +++++++---
>>>>>>>      1 file changed, 7 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>>>>>> index 8a2e5716d8dba2..d22d14b0ef0c84 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hmm.c
>>>>>>> @@ -191,14 +191,18 @@ int amdgpu_hmm_range_get_pages(struct mmu_interval_notifier *notifier,
>>>>>>>      	hmm_range->dev_private_owner = owner;
>>>>>>>      
>>>>>>>      	do {
>>>>>>> -		hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end);
>>>>>>> +		hmm_range->end = min_t(typeof(hmm_range->end),
>>>>>>> +				       hmm_range->start + MAX_WALK_BYTE,
>>>>>>> +				       end);
>>>>>> Since end is a local variable I would strongly prefer to just have it
>>>>>> use the correct type for it.
>>>>>>
>>>>>> Otherwise we might end up using something which doesn't work on all
>>>>>> architectures.
>>>>> They all appear to be "unsigned long". I thought, since we assign to
>>>>> hmm_range->end, we use that type.
>>>> Mhm, then why does the compiler complain here?
>>> Right... so MAX_WALK_BYTE is 2^36 ULL (diabolically defined as 64ULL<<30 :-) ),
>>> and this is why the minmax check complains.
>>>
>>> So, since the left-hand expression is unsigned long,
>>> i.e.,
>>> 	hmm_range->end = min(hmm_range->start + MAX_WALK_BYTE, end);
>>> is,
>>> 	unsigned long = min(unsigned long long, unsigned long);
>>> The compiler complains.
>>>
>>> I'd really prefer MAX_WALK_BYTE be less than or equal to ULONG_MAX,
>> That's not only a preference, but a must have. Otherwise the code maybe
>> won't work as expected on 32bit architectures.
> Well, I don't know what to change MAX_WALK_BYTE to, and given the suggestion
> below to change to min_t(u64, ...), I wonder if messing with MAX_WALK_BYTE
> even matters--it wouldn't matter so long as the type in min_t() is u64.
> It's a macro at the moment.
>
> However, the LHS--struct hmm_range's members are all
> unsigned long and then we're essentially doing (unsigned long) = (u64),
> which on 32-bit is (u32) = (u64).
[JZ]MAX_WALK_BYTE can be reduce to #define MAX_WALK_BYTE (2UL<<30)
>
> Regards,
> Luben
>
>>> and be defined as <literal>UL. I mean, why is everything in struct hmm_range
>>> "unsigned long", but we set a high limit of 10_0000_0000h for an end, and
>>> compare it to "end" to find the smaller? If our "end" could potentially
>>> be 10_0000_0000h then shouldn't the members in struct hmm_range be
>>> unsigned long long as well?
>> No, that the hmm range depends on the address space bits of the CPU is
>> perfectly correct. Essentially this is just an userspace address range.
>>
>> Our problem here is that this code needs to work on both 32bit and 64bit
>> systems. And on a 32bit system limiting the types wouldn't work
>> correctly as far as I can see.
>>
>> So the compiler is complaining for rather good reasons and by using
>> "min_t(UL" we just hide that instead of fixing the problem.
>>
>> I suggest to use "min_t(u64" instead. An intelligent compiler should
>> even be capable of optimizing this away by looking at the input types on
>> 32bit archs.
>>
>>> And for the timeout, we have the (now) obvious,
>>>
>>> 	timeout = max((hmm_range->end - hmm_range->start) >> 29, 1ULL);
>>>
>>> and I don't know why we necessarily need a "1ULL", when 1UL would do just fine,
>>> and then compilation passes for that statement. I can set this to 1UL, instead
>>> of using max_t().
>> I think just changing this to 1UL should be sufficient.
>>
>> Regards,
>> Christian.
>>
>>> Regards,
>>> Luben
>>>
>>>
>>>> As far as I can see "unsigned long" is correct here, but if we somehow
>>>> have a typecast then something is not working as expected.
>>>>
>>>> Is MAX_WALK_BYTE maybe of signed type?
>>>>
>>>>> Would you prefer at the top of the function to define "timeout" and "end" as,
>>>>> 	typeof(hmm_range->end) end, timeout;
>>>> Well for end that might make sense, but timeout is independent of the
>>>> hmm range.
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Regards,
>>>>> Luben
>>>>>