Re: [patch 1/6] Linux Kernel Markers - Architecture Independent Code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> Changelog:
> - markers_mutex now nests inside module_mutex rather than the opposite.
> 
> Signed-off-by: Mathieu Desnoyers <[email protected]>
> Reviewed-by: Christoph Hellwig <[email protected]>
> Reviewed-by: Rusty Russell <[email protected]>
> Reviewed-by: "Frank Ch. Eigler" <[email protected]>

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.


> ---
> 
>  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(-)


> 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 @@

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

                           marker.

> + * 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);

> 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

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

                            has

> + * 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'\0'format'\0' */
> +};
> +
> +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

> + * 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);


> +/*
> + * 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?

> +
> +	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

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

> +#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?


> +#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;
> +	}
> +}

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

wrong function name above.

> + * @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)
> +{
  ...
> +}
> +EXPORT_SYMBOL_GPL(marker_probe_unregister_pdata);

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux