Re: [PATCH] drm/amdgpu: Fix minmax error

[Luben Tuikov <luben.tuikov@amd.com>]

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@amd.com>
>>>> Cc: Felix Kuehling <Felix.Kuehling@amd.com>
>>>> Fixes: 58170a7a002ad6 ("drm/amdgpu: fix stall on CPU when allocate large system memory")
>>>> Signed-off-by: Luben Tuikov <luben.tuikov@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,
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?

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().

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
>>
>