Re: [PATCH] tracing: Add register read and write tracing support

[Steven Rostedt <rostedt@xxxxxxxxxxx>]

On Sun, 27 Sep 2020 17:34:50 -0700
Prasad Sodagudi <psodagud@xxxxxxxxxxxxxx> wrote:

> Add register read/write operations tracing support.
> ftrace events helps trace register read and write
> location details of memory mapped IO registers. Also
> add _no_log variants the writel_relaxed/readl_relaed
> APIs to avoid excessive logging for certain register
> operations.

As mentioned elsewhere, I don't see a reason for "nolog" variants if it
is just to avoid logging too much. You can easily filter on the
recording side.


> --- /dev/null
> +++ b/include/linux/iorw.h
> @@ -0,0 +1,20 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + *
> + */
> +#ifndef __LOG_IORW_H__
> +#define __LOG_IORW_H__
> +
> +#include <linux/types.h>
> +
> +#if IS_ENABLED(CONFIG_TRACE_RW)
> +void log_write_io(volatile void __iomem *addr);
> +void log_read_io(const volatile void __iomem *addr);

So basically, this is always doing a function call, even when tracing
is not enabled. You may want to turn this into a macro, and use the new
interface I'm about to push:

 See https://lore.kernel.org/r/20200925211206.423598568@xxxxxxxxxxx

Although I'm about to push a v3 (found a config that breaks msr.h)

#if IS_ENABLED(CONFIG_TRACE_RW)
#include <linux/atomic.h>
#include <linux/tracepoint-defs.h>

DECLARE_TRACEPOINT(rwio_write);
DECLARE_TRACEPOINT(rwio_read);

void __log_write_io(volatile void __iomem *addr);
void __log_read_io(const volatile void __iomem *addr);

#define log_write_io(addr) \
	if (tracepoint_enabled(rwio_write)
		__log_write_io(addr)

#define log_read_io(addr) \
	if (tracepoint_enabled(rwio_read)
		__log_read_io(addr)


> +#else
> +static inline void log_write_io(volatile void __iomem *addr)
> +{ }
> +static inline void log_read_io(const volatile void __iomem *addr)
> +{ }
> +#endif /* CONFIG_TRACE_RW */
> +
> +#endif /* __LOG_IORW_H__  */
> diff --git a/include/trace/events/rwio.h b/include/trace/events/rwio.h
> new file mode 100644
> index 0000000..b829629
> --- /dev/null
> +++ b/include/trace/events/rwio.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM rwio
> +
> +#if !defined(_TRACE_RWIO_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_RWIO_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(raw_write,

"raw" is too generic. Call this rwio_write.

> +
> +	TP_PROTO(unsigned long fn, volatile void __iomem *addr),
> +
> +	TP_ARGS(fn, addr),
> +
> +	TP_STRUCT__entry(
> +		__field(u64, fn)
> +		__field(u64, addr)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->fn = fn;
> +		__entry->addr = (u64)addr;
> +	),
> +
> +	TP_printk("%pS write addr=%p\n", __entry->fn, __entry->addr)
> +);
> +
> +TRACE_EVENT(raw_read,

And this "rwio_read"

-- Steve

> +
> +	TP_PROTO(unsigned long fn, const volatile void __iomem *addr),
> +
> +	TP_ARGS(fn, addr),
> +
> +	TP_STRUCT__entry(
> +		__field(u64, fn)
> +		__field(u64, addr)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->fn = fn;
> +		__entry->addr = (u64)addr;
> +	),
> +
> +	TP_printk("%pS read addr=%p\n", __entry->fn, __entry->addr)
> +);
> +
> +#endif /* _TRACE_PREEMPTIRQ_H */
> +