Message-ID: <4A266546.5080601@linux.vnet.ibm.com>
Date:	Wed, 03 Jun 2009 13:57:58 +0200
From:	Peter Oberparleiter <oberpar@...ux.vnet.ibm.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	linux-kernel@...r.kernel.org, andi@...stfloor.org,
	ying.huang@...el.com, W.Li@....COM, michaele@....ibm.com,
	mingo@...e.hu, heicars2@...ux.vnet.ibm.com,
	mschwid2@...ux.vnet.ibm.com
Subject: Re: [PATCH 3/4] gcov: add gcov profiling infrastructure

Andrew Morton wrote:
> On Tue, 02 Jun 2009 13:44:02 +0200
> Peter Oberparleiter <oberpar@...ux.vnet.ibm.com> wrote:
>> +/*
>> + * __gcov_init is called by gcc-generated constructor code for each object
>> + * file compiled with -fprofile-arcs.
>> + */
> 
> How does this work?  At what time does gcc call the constructors, and
> in what context are they called, etc?
> 
> IOW, please teach me about -fprofile-arcs :)

No problem :) -fprofile-arcs cause gcc to insert profiling code which 
relies on gcc's constructor mechanism for initialization purposes (so 
the two are separate things while the former makes use of the latter). 
gcc's constructor mechanism also has uses besides -fprofile-arcs, such 
as calling C++ object constructors and the like.

In userland, constructor functions are called by one of the gcc 
libraries just before the main() function. For the kernel, the kernel 
constructor patch mimics a similar behavior:
* for code which was compiled in, constructor functions are called
   in do_basic_setup(), just before processing initcalls
* for modules, init_module() calls them before mod->init().
The context is in both cases that of the calling functions.

Note that any function can be made a constructor function by tagging it 
with __attribute__((constructor)), though I'm not sure if that has any 
feasible use.

>> +void __gcov_init(struct gcov_info *info)
>> +{
>> +	static unsigned int gcov_version;
>> +
>> +	mutex_lock(&gcov_lock);
>> +	if (gcov_version == 0) {
>> +		gcov_version = info->version;
>> +		/*
>> +		 * Printing gcc's version magic may prove useful for debugging
>> +		 * incompatibility reports.
>> +		 */
>> +		pr_info("version magic: 0x%x\n", gcov_version);
> 
> Will this be spat out as simply
> 
> version magic: 0x1234
> 
> ?  If so, that'll be rather obscure because people won't know what
> subsystem printed it.  Prefixing this (and all other printks) with
> "gcov: " would fix that.

As Michael already pointed out that's already being taken care of by the 
  #define pr_fmt.

> 
>> +	}
>> +	/*
>> +	 * Add new profiling data structure to list and inform event
>> +	 * listener.
>> +	 */
>> +	info->next = gcov_info_head;
>> +	gcov_info_head = info;
>> +	if (gcov_events_enabled)
>> +		gcov_event(GCOV_ADD, info);
>> +	mutex_unlock(&gcov_lock);
>> +}
>> +EXPORT_SYMBOL(__gcov_init);
>> +
>>
>> ...
>>
>> +#ifdef CONFIG_MODULES
>> +static inline int within(void *addr, void *start, unsigned long size)
>> +{
>> +	return ((addr >= start) && (addr < start + size));
>> +}
> 
> That's our fourth implementation of within() (at least).  All basically
> identical.  Whine.

I know, I know.. and I will try to come up with another patch that 
consolidates all those within() implementations - but seeing that this 
might take more discussions to get right, I'd rather only start with 
that after making sure that the gcov code is finalized.

> 
>> ...
>>
>> +static int __init gcov_persist_setup(char *str)
>> +{
>> +	int val;
>> +	char delim;
>> +
>> +	if (sscanf(str, "%d %c", &val, &delim) != 1) {
>> +		pr_warning("invalid gcov_persist parameter '%s'\n", str);
>> +		return 0;
>> +	}
>> +	pr_info("setting gcov_persist to %d\n", val);
>> +	gcov_persist = val;
>> +
>> +	return 1;
>> +}
>> +__setup("gcov_persist=", gcov_persist_setup);
> 
> hm, what's the input format here?  It looks like
> 
> 	gcov_persist=1 x
> 
> and " x" is checked for, but ignored.
> 
> Confused.  What's this all for?  Can't we just us plain old strtoul(), etc?

Right - the sscanf would make sense if kernel parameters could contain 
spaces (in that case it catches <number><blanks><garbage> input) which 
it can't so strtoul() would indeed make more sense. I'll prepare an 
updated patch and send it out later today.

>> +/*
>> + * seq_file.start() implementation for gcov data files. Note that the
>> + * gcov_iterator interface is designed to be more restrictive than seq_file
>> + * (no start from arbitrary position, etc.), to simplify the iterator
>> + * implementation.
>> + */
>> +static void *gcov_seq_start(struct seq_file *seq, loff_t *pos)
>> +{
>> +	loff_t i;
>> +
>> +	gcov_iter_start(seq->private);
>> +	for (i = 0; i < *pos; i++) {
>> +		if (gcov_iter_next(seq->private))
>> +			return NULL;
>> +	}
>> +	return seq->private;
>> +}
> 
> This looks like it could be very expensive if used inappropriately.  I
> guess the answer to that is "don't use it inapprpriately", yes?

Yes :) The expensiveness is more than made up for by the reduced code 
complexity of the iterator interface. Also the extra effort is only 
spent when actually reading coverage files, i.e. not while performing a 
test.

>> ...
>>
>> +struct gcov_info *gcov_info_dup(struct gcov_info *info)
>> +{
>> +	struct gcov_info *dup;
>> +	unsigned int i;
>> +	unsigned int active;
>> +
>> +	/* 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?

>> +	if (!dup)
>> +		return NULL;
>> +	dup->version		= info->version;
>> +	dup->stamp		= info->stamp;
>> +	dup->n_functions	= info->n_functions;
>> +	dup->ctr_mask		= info->ctr_mask;
>> +	/* Duplicate filename. */
>> +	dup->filename		= kstrdup(info->filename, GFP_KERNEL);
>> +	if (!dup->filename)
>> +		goto err_free;
>> +	/* Duplicate table of functions. */
>> +	dup->functions = kmemdup(info->functions, info->n_functions *
>> +				 get_fn_size(info), GFP_KERNEL);
>> +	if (!dup->functions)
>> +		goto err_free;
>> +	/* Duplicate counter arrays. */
>> +	for (i = 0; i < active ; i++) {
>> +		struct gcov_ctr_info *ctr = &info->counts[i];
>> +		size_t size = ctr->num * sizeof(gcov_type);
>> +
>> +		dup->counts[i].num = ctr->num;
>> +		dup->counts[i].merge = ctr->merge;
>> +		dup->counts[i].values = kmemdup(ctr->values, size, GFP_KERNEL);
>> +		if (!dup->counts[i].values)
>> +			goto err_free;
>> +	}
>> +	return dup;
>> +
>> +err_free:
>> +	gcov_info_free(dup);
>> +	return NULL;
>> +}
>> +
> 
> It all looks very nice to me.

Thanks :)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists