Re: [PATCH v3 3/4] mm: cma: add list of currently allocated CMA buffers to debugfs

[Stefan Strogin <s.strogin@partner.samsung.com>]

Hi Michał,

Thank you for the answer.

On 25/02/15 00:32, Michal Nazarewicz wrote:
> On Tue, Feb 24 2015, Stefan Strogin <s.strogin@partner.samsung.com> wrote:
>> --- a/mm/cma.h
>> +++ b/mm/cma.h
>> @@ -11,8 +13,32 @@ struct cma {
>>  	struct hlist_head mem_head;
>>  	spinlock_t mem_head_lock;
>>  #endif
>> +#ifdef CONFIG_CMA_BUFFER_LIST
>> +	struct list_head buffer_list;
>> +	struct mutex	list_lock;
>> +#endif
>>  };
>>  
>> +#ifdef CONFIG_CMA_BUFFER_LIST
>> +struct cma_buffer {
>> +	unsigned long pfn;
>> +	unsigned long count;
>> +	pid_t pid;
>> +	char comm[TASK_COMM_LEN];
>> +#ifdef CONFIG_CMA_ALLOC_STACKTRACE
>> +	unsigned long trace_entries[16];
>> +	unsigned int nr_entries;
>> +#endif
>> +	struct list_head list;
>> +};
> 
> This structure is only ever used in cma_debug.c so is there a reason
> to define it in the header file?
> 

No, there isn't. Thanks. I'll move it to cma_debug.c

>> +
>> +extern int cma_buffer_list_add(struct cma *cma, unsigned long pfn, int count);
>> +extern void cma_buffer_list_del(struct cma *cma, unsigned long pfn, int count);
>> +#else
>> +#define cma_buffer_list_add(cma, pfn, count) { }
>> +#define cma_buffer_list_del(cma, pfn, count) { }
>> +#endif /* CONFIG_CMA_BUFFER_LIST */
>> +
>>  extern struct cma cma_areas[MAX_CMA_AREAS];
>>  extern unsigned cma_area_count;
> 
> 
>> +#ifdef CONFIG_CMA_BUFFER_LIST
>> +static ssize_t cma_buffer_list_read(struct file *file, char __user *userbuf,
>> +				    size_t count, loff_t *ppos)
>> +{
>> +	struct cma *cma = file->private_data;
>> +	struct cma_buffer *cmabuf;
>> +	char *buf;
>> +	int ret, n = 0;
>> +#ifdef CONFIG_CMA_ALLOC_STACKTRACE
>> +	struct stack_trace trace;
>> +#endif
>> +
>> +	if (*ppos < 0 || !count)
>> +		return -EINVAL;
>> +
>> +	buf = vmalloc(count);
>> +	if (!buf)
>> +		return -ENOMEM;
>> +
>> +	mutex_lock(&cma->list_lock);
>> +	list_for_each_entry(cmabuf, &cma->buffer_list, list) {
>> +		n += snprintf(buf + n, count - n,
>> +			      "0x%llx - 0x%llx (%lu kB), allocated by pid %u (%s)\n",
>> +			      (unsigned long long)PFN_PHYS(cmabuf->pfn),
>> +			      (unsigned long long)PFN_PHYS(cmabuf->pfn +
>> +				      cmabuf->count),
>> +			      (cmabuf->count * PAGE_SIZE) >> 10, cmabuf->pid,
>> +			      cmabuf->comm);
>> +
>> +#ifdef CONFIG_CMA_ALLOC_STACKTRACE
>> +		trace.nr_entries = cmabuf->nr_entries;
>> +		trace.entries = &cmabuf->trace_entries[0];
>> +		n += snprint_stack_trace(buf + n, count - n, &trace, 0);
>> +		n += snprintf(buf + n, count - n, "\n");
>> +#endif
>> +	}
>> +	mutex_unlock(&cma->list_lock);
>> +
>> +	ret = simple_read_from_buffer(userbuf, count, ppos, buf, n);
>> +	vfree(buf);
>> +
>> +	return ret;
>> +}
> 
> So in practice user space must allocate buffer big enough to read the
> whole file into memory.  Calling read(2) with some count will never read
> anything past the first count bytes of the file.
> 

My fault. You are right.
I'm not sure how to do the output nice... I could use *ppos to point the
number of next list entry to read (like that is used in
read_page_owner()). But in this case the list could be changed before we
finish reading, it's bad.
Or we could use seq_files like in v1, iterating over buffer_list
entries. But seq_print_stack_trace() has to be added.