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

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

On 2022-11-25 16:03, James Zhu wrote:
> 
> 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@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,
>>> 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)

Hi James--okay, I'll prep up a patch.

Regards,
Luben