Re: [RFC PATCH] Kernel Tracepoints

[Mathieu Desnoyers <mathieu dot desnoyers at polymtl dot ca>]

* Masami Hiramatsu (mhiramat@redhat.com) wrote:
> Hi Mathieu,
> 
> Thank you for making this so soon!
> 

Hi Masami,

Thanks for the comments, I will rework the patch accordingly.

Also, one thing I thought about yesterday which I dislike is that if we
have two modules declaring the same tracepoint in different headers with
different prototypes, each declaration will be valid but the
registration will try to connect a probe expecting wrong parameters to
the other tracepoint.

It would be the case if someone does :

drivers/somedrivera/mydriver1-trace.h

DECLARE_TRACE(really_generic_name, TPPTOTO(void), TPARGS()));


drivers/somedriverb/mydriver2-trace.h

DECLARE_TRACE(really_generic_name, TPPTOTO(struct somestruct *s), TPARGS(s)));

Do you think it's worth it to append the prototype string to the
tracepoint name ? I think it should fix the problem.

Mathieu

> Mathieu Desnoyers wrote:
> > ** note : this patch is submitted for early review. It applies after my
> > current unreleased 2.6.26-rc8 LTTng patchset. Comments are welcome.
> 
> Would you mean there is no tree on which we can test this patch?
> 
> >
> > Implementation of kernel tracepoints. Inspired from the Linux Kernel Markers.
> 
> What would you think redesigning markers on tracepoints? because most of the
> logic (scaning sections, multiple probe and activation) seems very similar
> to markers.
> 
> For example, (not complete, I just thought :-))
> 
>  struct tracepoint {
>  	const char *name;		/* Tracepoint name */
>  	DEFINE_IMV(char, state);	/* Immediate value state. */
>  	struct tracepoint_probe_closure *multi;	/* Closures */
> 	void * callsite_data;		/* private date from call site */
>  } __attribute__((aligned(8)));
> 
>  #define __tracepoint_block(generic, name, data, func, args)
>  	static const char __tpstrtab_##name[]			\
>  	__attribute__((section("__tracepoints_strings")))	\
>  	= #name;						\
>  	static struct tracepoint __tracepoint_##name		\
>  	__attribute__((section("__tracepoints"), aligned(8))) =	\
>  	{ __tpstrtab_##name, 0, NULL, data};			\
>  	if (!generic) {						\
>  		if (unlikely(imv_cond(__tracepoint_##name.state))) { \
>  			imv_cond_end();				\
>  			func(&__tracepoint_##name, args); \
>  		} else						\
>  			imv_cond_end();				\
>  	} else {						\
>  		if (unlikely(_imv_read(__tracepoint_##name.state))) \
>  			func(&__tracepoint_##name, args); \
>  	}
> 
>  struct marker {
>         const char *name;       /* Marker name */
>         const char *format;     /* Marker format string, describing the
>                                  * variable argument list.
>                                  */
>  } __attribute__((aligned(8)));
> 
>  #define trace_mark(name, fmt, args...)			\
>  do {							\
>  	static const char __mstrtab_##name[]		\
>  	__attribute__((section("__markers_strings")))	\
>  	= #name "\0" fmt;				\
>  	static struct marker __marker_##name		\
>  	__attribute__((section("__markers"), aligned(8))) =	\
>  	{ __mstrtab_##name, &__mstrtab_##name[sizeof(#name)]};	\ 	
>  	__tracepoint_block(1, name, __marker_##name, marker_probe_cb, args)	\
>  } while (0)
> 
> >
> > Allows complete typing verification. No format string required.
> >
> > TODO : Documentation/tracepoint.txt
> [...]
> > +/*
> > + * Note : the empty asm volatile with read constraint is used here instead of a
> > + * "used" attribute to fix a gcc 4.1.x bug.as
> > + * Make sure the alignment of the structure in the __markers section will
> > + * not add unwanted padding between the beginning of the section and the
> > + * structure. Force alignment to the same alignment as the section start.
> 
> this comment should be updated...
> 
> > + *
> > + * The "generic" argument controls which marker enabling mechanism must be used.
> > + * If generic is true, a variable read is used.
> > + * If generic is false, immediate values are used.
> > + */
> > +#define DEFINE_TRACE(name, proto, args)					\
> > +	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
> > +	{								\
> > +		int i;							\
> > +		struct tracepoint_probe_closure *multi;			\
> > +		preempt_disable();					\
> > +		multi = tp->multi;					\
> > +		smp_read_barrier_depends();				\
> > +		if (multi) {						\
> > +			for (i = 0; multi[i].func; i++) {		\
> > +				((void(*)(void *private_data, proto))	\
> > +				(multi[i].func))(multi[i].probe_private, args);\
> > +			}						\
> > +		}							\
> > +		preempt_enable();					\
> > +	}								\
> > +	static inline void __trace_##name(int generic, proto)		\
> > +	{								\
> > +		static const char __tpstrtab_##name[]			\
> > +		__attribute__((section("__tracepoints_strings")))	\
> > +		= #name;						\
> > +		static struct tracepoint __tracepoint_##name		\
> > +		__attribute__((section("__tracepoints"), aligned(8))) =	\
> > +		{ __tpstrtab_##name, 0, NULL };				\
> > +		if (!generic) {						\
> > +			if (unlikely(imv_cond(__tracepoint_##name.state))) { \
> > +				imv_cond_end();				\
> > +				_do_trace_##name(&__tracepoint_##name, args); \
> > +			} else						\
> > +				imv_cond_end();				\
> > +		} else {						\
> > +			if (unlikely(_imv_read(__tracepoint_##name.state))) \
> > +				_do_trace_##name(&__tracepoint_##name, args); \
> > +		}							\
> > +	}								\
> > +	static inline void trace_##name(proto)				\
> > +	{								\
> > +		__trace_##name(0, args);				\
> > +	}								\
> > +	static inline void _trace_##name(proto)				\
> > +	{								\
> > +		__trace_##name(1, args);				\
> > +	}								\
> > +	static inline int register_trace_##name(			\
> > +		void (*probe)(void *private_data, proto),		\
> > +		void *private_data)					\
> > +	{								\
> > +		return tracepoint_probe_register(#name, (void *)probe,	\
> > +			private_data);					\
> > +	}								\
> > +	static inline void unregister_trace_##name(			\
> > +		void (*probe)(void *private_data, proto),		\
> > +		void *private_data)					\
> > +	{								\
> > +		tracepoint_probe_unregister(#name, (void *)probe,	\
> > +			private_data);					\
> > +	}
> 
> Out of curiousity, what the private_data is for?
> 
> > +
> > +extern void tracepoint_update_probe_range(struct tracepoint *begin,
> > +	struct tracepoint *end);
> > +
> > +#else /* !CONFIG_TRACEPOINTS */
> > +#define DEFINE_TRACE(name, proto, args)			\
> > +	static inline void _do_trace_##name(struct tracepoint *tp, proto) \
> > +	{ }								\
> > +	static inline void __trace_##name(int generic, proto)		\
> > +	{ }								\
> > +	static inline void trace_##name(proto)				\
> > +	{ }								\
> > +	static inline void _trace_##name(proto)				\
> > +	{ }
> 
> By the way, I think you'd better add below two inlines.
> 
> 	static inline int register_trace_##name(			\
> 		void (*probe)(void *private_data, proto),		\
> 		void *private_data)					\
> 	{ return -ENOSYS; }
> 	static inline void unregister_trace_##name(			\
> 		void (*probe)(void *private_data, proto),		\
> 		void *private_data)					\
> 	{ }
> 
> 
> > +
> > +static inline void tracepoint_update_probe_range(struct tracepoint *begin,
> > +	struct tracepoint *end)
> > +{ }
> > +#endif /* CONFIG_TRACEPOINTS */
> > +
> > +/*
> > + * Connect a probe to a tracepoint.
> > + * Internal API, should not be used directly.
> > + */
> > +extern int tracepoint_probe_register(const char *name,
> > +	void *probe, void *probe_private);
> > +
> > +/*
> > + * Disconnect a probe from a tracepoint.
> > + * Internal API, should not be used directly.
> > + */
> > +extern int tracepoint_probe_unregister(const char *name,
> > +	void *probe, void *probe_private);
> > +
> > +struct tracepoint_iter {
> > +	struct module *module;
> > +	struct tracepoint *tracepoint;
> > +};
> > +
> > +extern void tracepoint_iter_start(struct tracepoint_iter *iter);
> > +extern void tracepoint_iter_next(struct tracepoint_iter *iter);
> > +extern void tracepoint_iter_stop(struct tracepoint_iter *iter);
> > +extern void tracepoint_iter_reset(struct tracepoint_iter *iter);
> > +extern int tracepoint_get_iter_range(struct tracepoint **tracepoint,
> > +	struct tracepoint *begin, struct tracepoint *end);
> > +
> > +#endif
> [...]
> 
> 
> Best regards,
> 
> -- 
> Masami Hiramatsu
> 
> Software Engineer
> Hitachi Computer Products (America) Inc.
> Software Solutions Division
> 
> e-mail: mhiramat@redhat.com
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68