Re: [patch 4/7] Linux Kernel Markers - Architecture Independent Code

[Christoph Hellwig <hch@infradead.org>]

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?