Re: [PATCH v6 4/5] drm: add drm_memory_stats_is_zero

["Christian König" <christian.koenig@amd.com>]

Am 07.11.24 um 15:43 schrieb Tvrtko Ursulin:
> On 07/11/2024 14:17, Li, Yunxiang (Teddy) wrote:
>> [AMD Official Use Only - AMD Internal Distribution Only]
>>
>>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Sent: Thursday, November 7, 2024 5:41
>>> On 25/10/2024 18:41, Yunxiang Li wrote:
>>>> Add a helper to check if the memory stats is zero, this will be used
>>>> to check for memory accounting errors.
>>>>
>>>> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_file.c | 9 +++++++++
>>>>    include/drm/drm_file.h     | 1 +
>>>>    2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>> index 714e42b051080..75ed701d80f74 100644
>>>> --- a/drivers/gpu/drm/drm_file.c
>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>> @@ -859,6 +859,15 @@ static void print_size(struct drm_printer *p, 
>>>> const char
>>> *stat,
>>>>      drm_printf(p, "drm-%s-%s:\t%llu%s\n", stat, region, sz, 
>>>> units[u]);
>>>>    }
>>>>
>>>> +int drm_memory_stats_is_zero(const struct drm_memory_stats *stats) {
>>>> +   return (stats->shared == 0 &&
>>>> +           stats->private == 0 &&
>>>> +           stats->resident == 0 &&
>>>> +           stats->purgeable == 0 &&
>>>> +           stats->active == 0);
>>>> +}
>>>
>>> Could use mem_is_zero() for some value of source/binary compactness.
>>
>> Yeah, the patch set started out with that when it's just a function 
>> in amdgpu, but Christ didn't like it.
>
> Okay, I don't feel so strongly about the implementation details.

mem_is_zero() just has the tendency to randomly fail when the compiler 
adds padding in between fields.

>>>> +EXPORT_SYMBOL(drm_memory_stats_is_zero);
>>>> +
>>>
>>> I am not a huge fan of adding this as an interface as the only 
>>> caller appears to be a
>>> sanity check in amdgpu_vm_fini():
>>>
>>>        if (!amdgpu_vm_stats_is_zero(vm))
>>>                dev_err(adev->dev, "VM memory stats is non-zero when 
>>> fini\n");
>>>
>>> But I guess there is some value in sanity checking since amdgpu does 
>>> not have a
>>> notion of debug only code (compiled at production and exercised via 
>>> a test suite).
>>>
>>> I do suggest to demote the dev_err to notice log level would suffice 
>>> and be more
>>> accurate.
>>
>> I think it's very important to have a check like this when we have a 
>> known invariant, especially in this case where there's stat tracking 
>> code spread out everywhere and we have very little chance of catching 
>> a bug right when it happened. And since whenever this check fails we 
>> know for sure there is a bug, I don't see the harm of keeping it as 
>> an error.
> It would indeed be a programming error if it can happen, but from the 
> point of view of a driver and system log I think a warning is actually 
> right.

Yeah agree, an error usually means you have either done something wrong 
or your data is corrupted because something bad happened (failed disk 
etc...).

The the stats are nonsense that is annoying but not fatal, so not really 
an error.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>>
>> Now that I think about it, I probably want to have the process & task 
>> name in here to aid in reproduction.
>>
>> Teddy