Re: [PATCH v5 1/2] drm/panthor: Expose size of driver internal BO's over fdinfo

[Steven Price <steven.price@arm.com>]

On 19/12/2024 16:30, Mihail Atanassov wrote:
> 
> 
> On 18/12/2024 18:18, Adrián Martínez Larumbe wrote:
>> From: Adrián Larumbe <adrian.larumbe@collabora.com>
>>
>> This will display the sizes of kenrel BO's bound to an open file,
>> which are
>> otherwise not exposed to UM through a handle.
>>
>> The sizes recorded are as follows:
>>   - Per group: suspend buffer, protm-suspend buffer, syncobjcs
>>   - Per queue: ringbuffer, profiling slots, firmware interface
>>   - For all heaps in all heap pools across all VM's bound to an open
>> file,
>>   record size of all heap chuks, and for each pool the gpu_context BO
>> too.
>>
>> This does not record the size of FW regions, as these aren't bound to a
>> specific open file and remain active through the whole life of the
>> driver.
>>
>> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
>> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
>> ---

[...]

>> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/
>> panthor/panthor_mmu.c
>> index c39e3eb1c15d..51f6e66df3f5 100644
>> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
>> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
>> @@ -1941,6 +1941,41 @@ struct panthor_heap_pool
>> *panthor_vm_get_heap_pool(struct panthor_vm *vm, bool c
>>       return pool;
>>   }
>>   +/**
>> + * panthor_vm_heaps_size() - Calculate size of all heap chunks across
>> all
>> + * heaps over all the heap pools in a VM
>> + * @pfile: File.
>> + * @status: Memory status to be updated.
>> + *
>> + * Calculate all heap chunk sizes in all heap pools bound to a VM. If
>> the VM
>> + * is active, record the size as active as well.
>> + */
>> +void panthor_vm_heaps_sizes(struct panthor_file *pfile, struct
>> drm_memory_stats *status)
>> +{
>> +    struct panthor_vm *vm;
>> +    unsigned long i;
>> +
>> +    if (!pfile->vms)
>> +        return;
>> +
>> +    xa_for_each(&pfile->vms->xa, i, vm) {
>> +        size_t size;
>> +
>> +        mutex_lock(&vm->heaps.lock);
> 
> Use `scoped_guard` instead?
> 
> #include <linux/cleanup.h>
> 
> /* ... */
> 
>     xa_for_each(...) {
>         size_t size;
> 
>         scoped_guard(mutex, &vm->heaps.lock) {
>             if (!vm->heaps.pool)
>                 continue;
> 
>             size = panthor_heap_pool_size(vm->heaps.pool);
>         }
>         /* ... */

I don't believe this actually works. The implementation of scoped_guard
uses a for() loop. So the "continue" will be applied to this (hidden)
internal loop rather than the xa_for_each() loop intended.

An alternative would be:

	xa_for_each(&pfile->vms->xa, i, vm) {
		size_t size = 0;

		mutex_lock(&vm->heaps.lock);
		if (vm->heaps.pool)
			size = panthor_heap_pool_size(vm->heaps.pool);
		mutex_unlock(&vm->heaps.lock);

		status->resident += size;
		status->private += size;
		if (vm->as.id >= 0)
			status->active += size;
	}

(relying on size=0 being a no-op for the additions). Although I was
personally also happy with the original - but perhaps that's just
because I'm old and still feel anxious when I see scoped_guard() ;)

Steve