Re: [PATCH 3/4] gcov: add gcov profiling infrastructure

[Peter Oberparleiter <oberpar@linux.vnet.ibm.com>]

Michael Ellerman wrote:
> On Fri, 2009-06-05 at 12:12 +0200, Peter Oberparleiter wrote:
>> Andrew Morton wrote:
>>> On Fri, 05 Jun 2009 11:23:04 +0200 Peter Oberparleiter <oberpar@linux.vnet.ibm.com> wrote:
>>>
>>>> Amerigo Wang wrote:
>>>>> On Wed, Jun 03, 2009 at 05:26:22PM +0200, Peter Oberparleiter wrote:
>>>>>> Peter Oberparleiter wrote:
>>>>>>> Andrew Morton wrote:
>>>>>>>> On Tue, 02 Jun 2009 13:44:02 +0200
>>>>>>>> Peter Oberparleiter <oberpar@linux.vnet.ibm.com> wrote:
>>>>>>>>> +	/* Duplicate gcov_info. */
>>>>>>>>> +	active = num_counter_active(info);
>>>>>>>>> +	dup = kzalloc(sizeof(struct gcov_info) +
>>>>>>>>> +		      sizeof(struct gcov_ctr_info) * active, GFP_KERNEL);
>>>>>>>> How large can this allocation be?
>>>>>>> Hm, good question. Having a look at my test system, I see coverage data 
>>>>>>> files of up to 60kb size. With counters making up the largest part of 
>>>>>>> those, I'd guess the allocation size can be around ~55kb. I assume that 
>>>>>>> makes it a candidate for vmalloc?
>>>>>> A further run with debug output showed that the maximum size is
>>>>>> actually around 4k, so in my opinion, there is no need to switch
>>>>>> to vmalloc.
>>>>> Unless you want virtually continious memory, you don't need to
>>>>> bother vmalloc().
>>>>>
>>>>> kmalloc() and get_free_pages() are all fine for this.
>>>> kmalloc() requires contiguous pages to serve an allocation request 
>>>> larger than a single page. The longer a kernel runs, the more fragmented 
>>>> the pool of free pages gets and the probability to find enough 
>>>> contiguous free pages is significantly reduced.
>>>>
>>>> In this case (having had a 3rd look), I found allocations of up to 
>>>> ~50kb, so to be sure, I'll switch that particular allocation to vmalloc().
>>> Well, vmalloc() isn't magic.  It can suffer internal fragmentation of
>>> the fixed-sized virtual address arena.
>>>
>>> Is it possible to redo the data structures so that the large array
>>> isn't needed?  Use a list, or move the data elsewhere or such?
>> Unfortunately not - the format of the data is dictated by gcc. Any 
>> attempt to break it down into page-sized chunks would only imitate what 
>> vmalloc() already does.
>>
>> Note though that this function is not called very often - it's only used 
>> to preserve coverage data for modules which are unloaded. And I only saw 
>> the 50kb counter data size for one file: kernel/sched.c (using a 
>> debugging patch).
> 
> Isn't it also called from gcov_seq_open() ?

Duh, of course - I totally forgot about that one. It used to be on 
module unload only but is, as you rightly pointed out, now basically 
called every time a coverage file is opened.

> 
>> So hm, I'm not sure about this anymore. I can also leave it at kmalloc() 
>> - chances are slim that anyone will actually experience a problem and if 
>> they do, they get an "order-n allocation failed" message so theres a 
>> hint at the cause for the problem.
> 
> Why are we duping it anyway? Rather than allocating it in the beginning,
> is it because gcc-generated code is writing directly to the original
> copy?

Yes - gcc's profiling code is writing to the original data structure. 
We're duping it on open() so that
a) data is consistent across reads() from the same file description
b) a non-vetoable module unload does not leave us with a defunct file 
descriptor

> 
> If there's any chance of memory allocation failure it'd be preferable
> for it to happen before the test run that generates the coverage data,
> that way you know before hand that you are out of memory. Rather than
> running some (possibly long & involved) test case, and then losing all
> your data.
> 

I agree - though I would not want to artificially limit the number of 
times a coverage file can be opened concurrently (which would need to be 
done in case of pre-allocation). If, after a long and difficult test 
you're running out of memory while collecting coverage data, you could 
try to free up some memory manually. Alternatively, assuming that the 
gcov-kdump tool will be ported to the upstream version as well, you 
could then create a kernel dump and extract coverage information from that.

So even with vmalloc() not being a perfect solution, it should reduce 
the number of allocation failures that might be seen after long running 
test cases and is therefore the method of choice.

===================
Subject: gcov: use vmalloc for duplicating counter data

From: Peter Oberparleiter <oberpar@linux.vnet.ibm.com>

Coverage data is duplicated during each open() of a data file and when
modules are unloaded. The contained data areas can get large (>50 kb)
so that kmalloc()-based allocations may fail due to memory
fragmentation, especially when a system has run for a long time. As
we only need virtually contiguous memory for the duplicate, the use
of vmalloc() can help prevent this problem.

Signed-off-by: Peter Oberparleiter <oberpar@linux.vnet.ibm.com>
---
 kernel/gcov/gcc_3_4.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-2.6.30-rc8-mm1/kernel/gcov/gcc_3_4.c
===================================================================
--- linux-2.6.30-rc8-mm1.orig/kernel/gcov/gcc_3_4.c
+++ linux-2.6.30-rc8-mm1/kernel/gcov/gcc_3_4.c
@@ -18,6 +18,7 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/seq_file.h>
+#include <linux/vmalloc.h>
 #include "gcov.h"
 
 /* Symbolic links to be created for each profiling data file. */
@@ -152,9 +153,10 @@ struct gcov_info *gcov_info_dup(struct g
 
 		dup->counts[i].num = ctr->num;
 		dup->counts[i].merge = ctr->merge;
-		dup->counts[i].values = kmemdup(ctr->values, size, GFP_KERNEL);
+		dup->counts[i].values = vmalloc(size);
 		if (!dup->counts[i].values)
 			goto err_free;
+		memcpy(dup->counts[i].values, ctr->values, size);
 	}
 	return dup;
 
@@ -173,7 +175,7 @@ void gcov_info_free(struct gcov_info *in
 	unsigned int i;
 
 	for (i = 0; i < active ; i++)
-		kfree(info->counts[i].values);
+		vfree(info->counts[i].values);
 	kfree(info->functions);
 	kfree(info->filename);
 	kfree(info);