Messages in this thread | | | Date | Mon, 24 Sep 2007 18:48:17 +0100 | From | Christoph Hellwig <> | Subject | Re: [patch 4/7] Linux Kernel Markers - Architecture Independent Code |
| |
On Mon, Sep 24, 2007 at 12:49:54PM -0400, Mathieu Desnoyers wrote: > +struct __mark_marker { > + const char *name; /* Marker name */ > + const char *format; /* Marker format string, describing the > + * variable argument list. > + */ > + char state; /* Marker state. */ > + marker_probe_func *call;/* Probe handler function pointer */ > + void *pdata; /* Private probe data */
This is normally called private in the kernel, and keeping this consistant would be nice.
> +} __attribute__((aligned(8)));
Why do we care about the alignment here?
> +/* To be used for string format validity checking with gcc */ > +static inline void __attribute__ ((format (printf, 1, 2))) > + __mark_check_format(const char *fmt, ...) { }
Please put each of the curly braces on a line of it's own, so it's clear this is an empty inline from the 1000 feet few, as it first looks like a prototype. Also aren't __attributes__ normally afer the function identifier, ala:
static inline void __mark_check_format(const char *fmt, ...) __attribute__ ((format (printf, 1, 2))) { }
or is this after notation only for prototypes but not actual implementations? (yeah, gnu C extensions sometimes have syntax that odd)
> +#ifdef CONFIG_MARKERS > +void module_update_markers(struct module *probe_module, int *refcount) > +{ > + struct module *mod; > + > + mutex_lock(&module_mutex); > + list_for_each_entry(mod, &modules, list) > + if (!mod->taints) > + marker_update_probe_range(mod->markers, > + mod->markers + mod->num_markers, > + probe_module, refcount); > + mutex_unlock(&module_mutex); > +} > +EXPORT_SYMBOL_GPL(module_update_markers);
Why is this exported? The markers code is always built into the kernel, isn't it?
> +EXPORT_SYMBOL_GPL(module_get_iter_markers);
Same here.
> +/* > + * Add the marker to the marker hash table. Must be called with markers_mutex > + * held. > + */ > +static int add_marker(const char *name, > + const char *format, marker_probe_func *probe, void *pdata)
static int add_marker(const char *name, const char *format, marker_probe_func *probe, void *private)
> +void marker_update_probe_range( > + struct __mark_marker *begin, > + struct __mark_marker *end, > + struct module *probe_module, > + int *refcount)
void marker_update_probe_range(struct __mark_marker *begin, struct __mark_marker *end, struct module *probe_module, int *refcount)
> +EXPORT_SYMBOL_GPL(marker_update_probe_range);
What is this one exported for?
> +/* > + * Update probes, removing the faulty probes. > + * Issues a synchronize_sched() when no reference to the module passed > + * as parameter is found in the probes so the probe module can be > + * safely unloaded from now on. > + */ > +static inline void marker_update_probes(struct module *probe_module)
no need to mark this inline, the compiler takes care of that for you if nessecary.
> +int marker_get_iter_range(struct __mark_marker **marker, > + struct __mark_marker *begin, > + struct __mark_marker *end)
int marker_get_iter_range(struct __mark_marker **marker, struct __mark_marker *begin, struct __mark_marker *end)
> + int found = 0; > + > + if (!*marker && begin != end) { > + found = 1; > + *marker = begin; > + } else if (*marker >= begin && *marker < end) { > + found = 1; > + /* > + * *marker is known to be a valid marker from now on. > + */ > + } > + return found;
if (!*marker && begin != end) { *marker = begin; return 1; }
if (*marker >= begin && *marker < end) return 1; return 0;
?
There seem to be a lot of exports and some functions that don't seem to be used by the obvious marker use-cases like your example, blktrace or sputrace. Care to explain why we'd really want them or better cut them out for this first submission? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |