Re: [RFC PATCH 5/6] gcov: add gcov profiling infrastructure

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
From: Andrew Morton
Date: Monday, May 5, 2008 - 10:43 pm

On Mon, 05 May 2008 17:24:46 +0200 Peter Oberparleiter <peter.oberparleiter@de.ibm.com> wrote:


So this appears under the "kenrel hacking" menu.  Whereas oprofile and
markers and other such things appear under "General setup".  hm.


The #else isn't needed here.

Traditionally we put gcc version checking into init/main.c, so you could do
the above test over in that file, inside #ifdef CONFIG_GCOV_PROFILE.


hm, what does the output from this look like?  "gcc version 42", which is
really gcc version 66 only we didn't tell the user that we printed it in
hex?

Might need a bit more thought here?


We have an exported-to-modules symbol which is completely unreferenced from
within the kernel source code.  You might want to add a comment in this
file explaining what's going on; otherwise people will try to delete your
code ;)


Please add a (good) comment above the definition of gcov_callback
explaining what it does.  For this is quite unobvious from the
implementation.

It is also unobvious why we will only ever need to support a
single callback.


That is at least our fourth implementation of within(), and not all of them
have the same semantics.


This code fails to compile with CONFIG_MODULES=n.


It is inappropriate that this helper macro be buried down in kernel/gcov/

<linux/compiler-gcc.h> might be a suitable place.  Ideally as a standalone
patch.

There was some talk about making the CC version available at Kconfig-time
but afaik that hasn't happened yet.


Didn't we already do that?


Unneeded #else block.


Can we zap gcov_type completely and use u64?


It'd be nice to document the core data structures.


Not very nice, sorry.  These take a type-unsafe void* when their arguments
must be of type `struct iterator_t *' (hopefully to become `struct
gcov_iterator *').  This is because they happen to be passed
seq_file.private, but who is to say that this will be the case for all
callers in the future.

In fact if these are given a properly typed argument we can rely upon the
compiler's automatic void*-to-anything* conversion to avoid a typecast at
the calling sites.  And we can avoid a local and a typecast within the
implementation of these functions.


Adding the names of the variables in prototypes can sometimes add a little
documentarty benefit.


`struct gcov_link', please.


Well that's one way of doing it.


How many times do we need to check this ;)


Should be `struct node', but that is too generic a name.  gcov_node, perhaps?


Use LIST_HEAD() here, remove the runtime INIT_LIST_HEAD() from
gcov_fs_init().


Please document kernel parameters in Documentation/kernel-parameters.txt.


Caller must take node_lock.  That's worth a comment.


Again, some comments explaining what the code does wouldn't hurt.

The above might look a bit neater and more maintainable were it to use
kasprintf() (would need a bit of reorganising of the extension part).


<wonders what this is for>


hm, seems to be creating something in debugfs ;)


Seems to be doing something with nodes ;)

Really, I find this code harder to follow than it needs to be.


I trust this won't be called very frequently.


So... there's a reset file which resets something?

The proposed user interfaces should be documented, please. 
Documentation/gcov.txt, perhaps.


I don't know what all this ghosting stuff is.  Perhaps it is obvious to
those who know gcov internals (ie: approximately 0% of kernel developers).


This is our callback!

Given that the code only supports a single callback, and that a few lines
below we register gcov_cb() to consume that callback, why do we need a
callback indirect pointer at all?

And if I'm wondering about this today, it is fair to assume that others
will wonder in the future.  So a permanent fix is to document the callback
mechanism in code comments.  I'd suggest at the gcov_callback definition
site.


KERN_ERR, perhaps.


So... this file is used only when gcc-3.4 is being used to compile the kernel?

Again, we have no way of knowing why this is being done!


Might be able to use hweight32() here, although that's a bit pointless and
presumptuous.


I'm wondering if there might be races here with counters becoming active
while this code is running.  But I know nothing about what determines when
a counter becomes active.  What governs this?


The typecast worries me.  We have a `struct gcov_ctr_info *' and we cast
that to a gcov_type*?  But we just cast a pointer to `unsigned int num', I
think.

Maybe I got confused.  Can this code be cleaned up to be kinder to the C
type system?  Or commented?


struct gcov_iterator?


the casts and the masking aren't strictly needed here.


I guess we don't have any endianness concerns here.

--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
[RFC PATCH 5/6] gcov: add gcov profiling infrastructure, Peter Oberparleiter, (Mon May 5, 8:24 am)
Re: [RFC PATCH 5/6] gcov: add gcov profiling infrastructure, Andrew Morton, (Mon May 5, 10:43 pm)
Re: [RFC PATCH 5/6] gcov: add gcov profiling infrastructure, Peter Oberparleiter, (Wed May 7, 3:57 am)