Leak check: process PID and name (patch included)

[GoogleCodeExporter]

Another "nice-to-have" feature. Not that it's crucial in any way, but it'll 
make life easier in some circumstances.

Original issue reported on code.google.com by [email protected] on 9 Apr 2013 at 9:35

Attachments:

• klc_pid_name.diff

[GoogleCodeExporter]

Original comment by [email protected] on 10 Apr 2013 at 6:58

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Thanks for the patch. My notes follow.

1. Currently, LeakCheck actually outputs the most recent event in the group 
with the same call stack. 

Would it be useful if the process name and PID were output for that event only? 
This way, we can preserve grouping of the similar events. Your patch with 
slight modifications would do here.

Are there any important use cases when it is needed to know process name and 
PID for each leak instead of the most recent one?

2. Concerning the changes to 'resource_info_create()'. In what conditions can 
'current' be NULL? On x86 (AFAIK), it is either the pointer to the task struct 
for the current process or, in case of interrupts, to the task struct of the 
interrupted process.

To distinguish between interrupt context (hardirq, softirq) and the process 
context, we can use (in_irq() || (softirq_count() & SOFTIRQ_OFFSET)). 
'in_interrupt()' is unreliable, unfortunately. I use that workaround in the 
data race detector for the kernel modules I am currently working on 
(http://code.google.com/p/kernel-strider/).

I suppose, it is OK to leave pid as -1 for interrupts and handle that case in 
'klc_print_process_info()' too, that is, output something like "Interrupt" 
instead of "Process: <...>"

3. One more note concerning 'klc_print_process_info()'. If memory allocation 
fails, it is better to return from the function after issuing the warning:

    buf = kmalloc(len + 1, GFP_KERNEL);
    if (buf == NULL) {
        pr_warning(KEDR_LC_MSG_PREFIX "klc_print_process_info(): "
        "not enough memory to prepare a message of size %d\n",
            len);
+       return;
    }

So, what do you think?

Original comment by [email protected] on 15 Apr 2013 at 12:25

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

1. In most cases - yes, I think. It doesn't seem hard to make this behaviour 
configurable with "module_param(...)" thing, though.
2. I don't have deep knowledge about Linux kernel yet, so yes, I added the NULL 
check just to be safe, without considering when exactly that happens (if it 
does). 
Of course it would be nice to distinguish interrupts from system calls 
initiated by a process.
3. Good point. However, the existing code sometime lacks such return 
statements, too: 
http://code.google.com/p/kedr/source/browse/sources/leak_check/core/klc_output.c
?r=759e03103b2cc70f62ea1cbc16a7c24faf6c6495#587

Original comment by [email protected] on 15 Apr 2013 at 9:15

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

1. Yes, it is easy to make that configurable with the module parameters. What 
worries me is the readability of the reports and memory issues.

In some cases LeakCheck reports many false errors. This may happen if it does 
not process some kernel function for memory allocation or deallocation (e.g., 
due to a  change in the relevant kernel ABI, which happens from time to time). 
Storing these results in the output system may take a lot of memory. Before I 
implemented grouping of similar reports, I stubmled upon a situation a couple 
of times when the OS became unusable as a result. 

However, if there is a particularly important use case when separate reports 
for each {process name, pid, event}, this could be done as you suggest.

3. My bugs. Nice catch, thanks! Fixed what I could find.

Original comment by [email protected] on 16 Apr 2013 at 7:58

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Hit "save" too early.
'when separate reports for each {process name, pid, event}' =>
'when separate reports for each {process name, pid, event} are needed'

Original comment by [email protected] on 16 Apr 2013 at 8:01

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

It might be possible to display a list (process name, pid) pairs along with 
each traceback. The list might be limited to N entries.

Original comment by [email protected] on 17 Apr 2013 at 2:54

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Yes, sounds reasonable.

Original comment by [email protected] on 18 Apr 2013 at 7:37

• Added labels: ****
• Removed labels: ****

[GoogleCodeExporter]

Implemented output of the name and pid of the most recent process. 

Basically, I applied your patch with some modifications - the ones discussed 
above + workaround for get_task_comm() not being exported before kernel version 
3.0. For the present, KEDR must support kernels 2.6.32 (Debian 6, RHEL 6, ...) 
and newer.

The code is now in the repository.

In the future, we could indeed output the list of processes for each call 
stack. Have no time to implement this yet, though.

Original comment by [email protected] on 27 Apr 2013 at 11:39

• Changed state: Accepted
• Added labels: ****
• Removed labels: ****