[PATCH] staging: android: ion: Add per-heap counters

[Alexey Skidanov]

On 09/10/2018 11:27 AM, Greg KH wrote:
> On Sun, Sep 09, 2018 at 01:44:31AM +0300, Alexey Skidanov wrote:
>> The heap statistics have been removed and currently even basics statistics
>> are missing.
>>
>> This patch creates per heap debugfs directory /sys/kernel/debug/<heap_name>
>> and adds two counters - the number of allocated buffers and number of
>> allocated bytes.
> 
> Why?  What can we do with this information?
Are you in doubt that we need these two specific counters or debugfs at all?
> 
>>
>> Signed-off-by: Alexey Skidanov <alexey.skidanov at intel.com>
>> ---
>>  drivers/staging/android/ion/ion.c | 40 +++++++++++++++++++++++++++++++--------
>>  drivers/staging/android/ion/ion.h |  3 ++-
>>  2 files changed, 34 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
>> index 9907332..62335b9 100644
>> --- a/drivers/staging/android/ion/ion.c
>> +++ b/drivers/staging/android/ion/ion.c
>> @@ -95,6 +95,9 @@ static struct ion_buffer *ion_buffer_create(struct ion_heap *heap,
>>  		goto err1;
>>  	}
>>  
>> +	heap->num_of_buffers++;
>> +	heap->num_of_alloc_bytes += len;
>> +
>>  	INIT_LIST_HEAD(&buffer->attachments);
>>  	mutex_init(&buffer->lock);
>>  	mutex_lock(&dev->buffer_lock);
>> @@ -528,6 +531,8 @@ void ion_device_add_heap(struct ion_heap *heap)
>>  {
>>  	struct ion_device *dev = internal_dev;
>>  	int ret;
>> +	struct dentry *heap_root;
>> +	char debug_name[64];
>>  
>>  	if (!heap->ops->allocate || !heap->ops->free)
>>  		pr_err("%s: can not add heap with invalid ops struct.\n",
>> @@ -546,6 +551,33 @@ void ion_device_add_heap(struct ion_heap *heap)
>>  	}
>>  
>>  	heap->dev = dev;
>> +	heap->num_of_buffers = 0;
>> +	heap->num_of_alloc_bytes = 0;
>> +
>> +	/* Create heap root directory. If creation is failed, we may continue */
>> +	heap_root = debugfs_create_dir(heap->name, dev->debug_root);
>> +	if (!IS_ERR_OR_NULL(heap_root)) {
> 
> Close, but no, never check the return value here.  Just keep on moving,
> and call further debugfs calls.  No need even for a comment here.
> 
>> +		debugfs_create_u64("num_of_buffers",
>> +				   0444, heap_root,
>> +				   &heap->num_of_buffers);
>> +		debugfs_create_u64("num_of_alloc_bytes",
>> +				   0444,
>> +				   heap_root,
>> +				   &heap->num_of_alloc_bytes);
>> +
>> +		if (heap->shrinker.count_objects &&
>> +		    heap->shrinker.scan_objects) {
>> +			snprintf(debug_name, 64, "%s_shrink", heap->name);
>> +			debugfs_create_file(debug_name,
>> +					    0644,
>> +					    heap_root,
>> +					    heap,
>> +					    &debug_shrink_fops);
>> +		}
>> +	} else {
>> +		pr_err("%s: Failed to create heap dir in debugfs\n", __func__);
> 
> No need to print anything.
> 
>> +	}
>> +
>>  	down_write(&dev->lock);
>>  	heap->id = heap_id++;
>>  	/*
>> @@ -555,14 +587,6 @@ void ion_device_add_heap(struct ion_heap *heap)
>>  	plist_node_init(&heap->node, -heap->id);
>>  	plist_add(&heap->node, &dev->heaps);
>>  
>> -	if (heap->shrinker.count_objects && heap->shrinker.scan_objects) {
>> -		char debug_name[64];
>> -
>> -		snprintf(debug_name, 64, "%s_shrink", heap->name);
>> -		debugfs_create_file(debug_name, 0644, dev->debug_root,
>> -				    heap, &debug_shrink_fops);
>> -	}
> 
> You just moved the location of these debugfs files, right?  What
> userspace tool just broke when you did that?  :)
> 
> thanks,
> 
> greg k-h
>