On Thu, 06 Sep 2007 16:07:34 -0400 Mathieu Desnoyers wrote:

quoted text
> Changelog:
> - markers_mutex now nests inside module_mutex rather than the opposite.
> 
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> Reviewed-by: Christoph Hellwig <hch@infradead.org>
> Reviewed-by: Rusty Russell <rusty@rustcorp.com.au>
> Reviewed-by: "Frank Ch. Eigler" <fche@redhat.com>

Documentation/SubmittingPatches recognizes Signed-off-by: and
Acked-by:.  Nothing about Reviewed-by.  Quote:

  Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
  has at least reviewed the patch and has indicated acceptance.

Reviewed-by: carries no such connotation or indication IMO.
Please use Acked-by if that's what these reviewers have done.


quoted text
> ---
> 
>  include/asm-generic/vmlinux.lds.h |   11 
>  include/linux/marker.h            |  169 +++++++++
>  include/linux/module.h            |    5 
>  kernel/marker.c                   |  699 ++++++++++++++++++++++++++++++++++++++
>  kernel/module.c                   |   11 
>  5 files changed, 894 insertions(+), 1 deletion(-)


quoted text
> Index: linux-2.6-lttng/include/linux/marker.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6-lttng/include/linux/marker.h	2007-09-06 15:07:28.000000000 -0400
> @@ -0,0 +1,169 @@

quoted text
> +/*
> + * Connect a probe to a markers.

                           marker.

quoted text
> + * pdata must be a valid allocated memory address, or NULL.
> + */
> +extern int marker_probe_register(const char *name, const char *format,
> +				marker_probe_func *probe, void *pdata);

quoted text
> Index: linux-2.6-lttng/kernel/marker.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6-lttng/kernel/marker.c	2007-09-06 15:07:28.000000000 -0400
> @@ -0,0 +1,699 @@
> +/*
> + * Marker deferred synchronization.
> + * Upon marker probe_unregister, we delay call to synchronize_sched() to
> + * accelerate mass unregistration (only when there is no more reference to a
> + * give module do we call synchronize_sched()). However, we need to make sure

      given

quoted text
> + * every critical region have ended before we re-arm a marker that has been

                            has

quoted text
> + * unregistered and then registered back with a different probe data.
> + */
> +static int deferred_sync;
> +
> +/*
> + * Marker hash table, containing the active markers.
> + * Protected by module_mutex.
> + */
> +#define MARKER_HASH_BITS 6
> +#define MARKER_TABLE_SIZE (1 << MARKER_HASH_BITS)
> +
> +struct marker_entry {
> +	struct hlist_node hlist;
> +	char *format;
> +	marker_probe_func *probe;
> +	void *pdata;
> +	int refcount;	/* Number of times armed. 0 if disarmed. */
> +	char name[0];	/* Contains name'__PLACEHOLDER__7_'format'__PLACEHOLDER__7_' */
> +};
> +
> +static struct hlist_head marker_table[MARKER_TABLE_SIZE];
> +
> +/**
> + * __mark_empty_function - Empty probe callback
> + * @mdata: pointer of type const struct __mark_marker
> + * @fmt: format string
> + * @...: variable argument list
> + *
> + * Empty callback provided as a probe to the markers. By providing this to a
> + * disabled marker, we makes sure the  execution flow is always valid even

                          make

quoted text
> + * though the function pointer change and the marker enabling are two distinct
> + * operations that modifies the execution flow of preemptible code.
> + */
> +void __mark_empty_function(const struct __mark_marker *mdata,
> +	void *private_data,
> +	const char *fmt, ...)
> +{ }
> +EXPORT_SYMBOL_GPL(__mark_empty_function);


quoted text
> +/*
> + * Sets the probe callback corresponding to one marker.
> + */
> +static int set_marker(struct marker_entry **entry,
> +			struct __mark_marker *elem)
> +{
> +	int ret;
> +	BUG_ON(strcmp((*entry)->name, elem->name) != 0);

You can't return -ENOENT here?

quoted text
> +
> +	if ((*entry)->format) {
> +		if (strcmp((*entry)->format, elem->format) != 0) {
> +			printk(KERN_NOTICE
> +				"Format mismatch for probe %s "
> +				"(%s), marker (%s)\n",
> +				(*entry)->name,
> +				(*entry)->format,
> +				elem->format);
> +			return -EPERM;
> +		}
> +	} else {
> +		ret = marker_set_format(entry, elem->format);
> +		if (ret)
> +			return ret;
> +	}
> +	elem->call = (*entry)->probe;
> +	elem->pdata = (*entry)->pdata;
> +	_immediate_set(&elem->state, 1);
> +	return 0;
> +}
> +
> +/*
> + * Disable a marker and its probe callback.
> + * Note: only after a synchronize_sched() issued after setting elem->call to the
> + * empty function insures that the original callback is not used anymore. This
> + * insured by preemption disabling around the call site.
> + */
> +static void disable_marker(struct __mark_marker *elem)
> +{
> +	_immediate_set(&elem->state, 0);
> +	elem->call = __mark_empty_function;
> +	/*
> +	 * Leave the pdata and id there, because removal is racy and should be
> +	 * done only after a synchronize_sched(). There are never used until

                                                  These

quoted text
> +	 * the next initialization anyway.
> +	 */
> +}
> +

quoted text
> +#ifdef CONFIG_MODULES
> +/*
> + * Update module probes.
> + * Must be called with markers_mutex held.
> + */
> +static inline void marker_update_probes_modules(struct module *probe_module,
> +	int *refcount)
> +{
> +	struct module *mod;
> +
> +	list_for_each_entry(mod, &modules, list) {
> +		if (!mod->taints) {
> +			marker_update_probe_range(mod->markers,
> +				mod->markers+mod->num_markers,
> +				probe_module, refcount);
> +		}
> +	}
> +}

hm, so if the module is tainted, markers won't be applied to it?
That's OK IMO.  However, I tend to think that something that
modifies code on the fly (like kprobes or text edit) should set some
flag somewhere so that when the system crashes, we can know that
code was modified.  This could be done via taints flags or some other
mechanism.  Has this already been discussed?


quoted text
> +#else
> +static inline void marker_update_probes_modules(struct module *probe_module,
> +	int *refcount)
> +{ }
> +#endif
> +
> +/*
> + * 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)
> +{
> +	int refcount = 0;
> +
> +	/* Core kernel markers */
> +	marker_update_probe_range(__start___markers,
> +			__stop___markers, probe_module, &refcount);
> +	/* Markers in modules. */
> +	marker_update_probes_modules(probe_module, &refcount);
> +	if (probe_module && refcount == 0) {
> +		synchronize_sched();
> +		deferred_sync = 0;
> +	}
> +}

quoted text
> +/**
> + * marker_probe_unregister -  Disconnect a probe from a marker

wrong function name above.

quoted text
> + * @pdata: probe private data
> + *
> + * Unregister a marker by providing the registered pdata.
> + * Returns the pdata given to marker_probe_register, or an ERR_PTR().
> + */
> +void *marker_probe_unregister_pdata(void *pdata)
> +{
  ...
quoted text
> +}
> +EXPORT_SYMBOL_GPL(marker_probe_unregister_pdata);

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
-
unsubscribe notice
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/