[PATCH] interconnect: Add basic tracepoints

[PATCH] interconnect: Add basic tracepoints

[Georgi Djakov]

The tracepoints can help with understanding the system behavior of a
given interconnect path when the consumer drivers change their bandwidth
demands. This might be interesting when we want to monitor the requested
interconnect bandwidth for each client driver. The paths may share the
same nodes and this will help to understand "who and when is requesting
what". All this is useful for subsystem drivers developers and may also
provide hints when optimizing the power and performance profile of the
system.

Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
---
 MAINTAINERS                         |  1 +
 drivers/interconnect/core.c         |  9 +++++
 include/trace/events/interconnect.h | 52 +++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+)
 create mode 100644 include/trace/events/interconnect.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 55199ef7fa74..c307c4b8f677 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8526,6 +8526,7 @@ F:	drivers/interconnect/
 F:	include/dt-bindings/interconnect/
 F:	include/linux/interconnect-provider.h
 F:	include/linux/interconnect.h
+F:	include/trace/events/interconnect.h
 
 INVENSENSE MPU-3050 GYROSCOPE DRIVER
 M:	Linus Walleij <linus.walleij@linaro.org>
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 7b971228df38..e24092558c29 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -24,6 +24,9 @@ static LIST_HEAD(icc_providers);
 static DEFINE_MUTEX(icc_lock);
 static struct dentry *icc_debugfs_dir;
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/interconnect.h>
+
 /**
  * struct icc_req - constraints that are attached to each node
  * @req_node: entry in list of requests for the particular @node
@@ -449,6 +452,9 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
 
 		/* aggregate requests for this node */
 		aggregate_requests(node);
+
+		trace_icc_set_bw(node, dev_name(path->reqs[i].dev),
+				 avg_bw, peak_bw);
 	}
 
 	ret = apply_constraints(path);
@@ -461,6 +467,9 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
 			path->reqs[i].avg_bw = old_avg;
 			path->reqs[i].peak_bw = old_peak;
 			aggregate_requests(node);
+
+			trace_icc_set_bw(node, dev_name(path->reqs[i].dev),
+					 old_avg, old_peak);
 		}
 		apply_constraints(path);
 	}
diff --git a/include/trace/events/interconnect.h b/include/trace/events/interconnect.h
new file mode 100644
index 000000000000..8e001382e9b0
--- /dev/null
+++ b/include/trace/events/interconnect.h
@@ -0,0 +1,52 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2019, Linaro Ltd.
+ * Author: Georgi Djakov <georgi.djakov@linaro.org>
+ */
+
+#if !defined(_TRACE_INTERCONNECT_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_INTERCONNECT_H
+
+#include <linux/tracepoint.h>
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM interconnect
+
+struct icc_node;
+
+TRACE_EVENT(icc_set_bw,
+
+	TP_PROTO(struct icc_node *n, const char *cdev, u32 avg_bw, u32 peak_bw),
+
+	TP_ARGS(n, cdev, avg_bw, peak_bw),
+
+	TP_STRUCT__entry(
+		__string(node_name, n->name)
+		__field(u32, node_avg_bw)
+		__field(u32, node_peak_bw)
+		__string(cdev, cdev)
+		__field(u32, avg_bw)
+		__field(u32, peak_bw)
+	),
+
+	TP_fast_assign(
+		__assign_str(node_name, n->name);
+		__entry->node_avg_bw = n->avg_bw;
+		__entry->node_peak_bw = n->peak_bw;
+		__assign_str(cdev, cdev);
+		__entry->avg_bw = avg_bw;
+		__entry->peak_bw = peak_bw;
+	),
+
+	TP_printk("%s avg_bw=%u peak_bw=%u cdev=%s avg_bw=%u peak_bw=%u",
+		__get_str(node_name),
+		__entry->node_avg_bw,
+		__entry->node_peak_bw,
+		__get_str(cdev),
+		__entry->avg_bw,
+		__entry->peak_bw)
+);
+#endif /* _TRACE_INTERCONNECT_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>

Re: [PATCH] interconnect: Add basic tracepoints

[Steven Rostedt]

On Fri, 18 Oct 2019 17:02:24 +0300
Georgi Djakov <georgi.djakov@linaro.org> wrote:

> The tracepoints can help with understanding the system behavior of a
> given interconnect path when the consumer drivers change their bandwidth
> demands. This might be interesting when we want to monitor the requested
> interconnect bandwidth for each client driver. The paths may share the
> same nodes and this will help to understand "who and when is requesting
> what". All this is useful for subsystem drivers developers and may also
> provide hints when optimizing the power and performance profile of the
> system.
> 
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
>  MAINTAINERS                         |  1 +
>  drivers/interconnect/core.c         |  9 +++++
>  include/trace/events/interconnect.h | 52 +++++++++++++++++++++++++++++
>  3 files changed, 62 insertions(+)
>  create mode 100644 include/trace/events/interconnect.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 55199ef7fa74..c307c4b8f677 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8526,6 +8526,7 @@ F:	drivers/interconnect/
>  F:	include/dt-bindings/interconnect/
>  F:	include/linux/interconnect-provider.h
>  F:	include/linux/interconnect.h
> +F:	include/trace/events/interconnect.h
>  
>  INVENSENSE MPU-3050 GYROSCOPE DRIVER
>  M:	Linus Walleij <linus.walleij@linaro.org>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 7b971228df38..e24092558c29 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -24,6 +24,9 @@ static LIST_HEAD(icc_providers);
>  static DEFINE_MUTEX(icc_lock);
>  static struct dentry *icc_debugfs_dir;
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/interconnect.h>
> +
>  /**
>   * struct icc_req - constraints that are attached to each node
>   * @req_node: entry in list of requests for the particular @node
> @@ -449,6 +452,9 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>  
>  		/* aggregate requests for this node */
>  		aggregate_requests(node);
> +
> +		trace_icc_set_bw(node, dev_name(path->reqs[i].dev),
> +				 avg_bw, peak_bw);

BTW, it's best to do the "dev_name()" from the TP_fast_assign()
portion. It keeps the work out of the inlined code here. Although it
shouldn't be processed within the fast path, it still adds a cache
footprint.

		trace_icc_set_bw(node, path->reqs[i].dev, avg_bw, peak_bw);

>  	}
>  
>  	ret = apply_constraints(path);
> @@ -461,6 +467,9 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>  			path->reqs[i].avg_bw = old_avg;
>  			path->reqs[i].peak_bw = old_peak;
>  			aggregate_requests(node);
> +
> +			trace_icc_set_bw(node, dev_name(path->reqs[i].dev),
> +					 old_avg, old_peak);
>  		}
>  		apply_constraints(path);
>  	}
> diff --git a/include/trace/events/interconnect.h b/include/trace/events/interconnect.h
> new file mode 100644
> index 000000000000..8e001382e9b0
> --- /dev/null
> +++ b/include/trace/events/interconnect.h
> @@ -0,0 +1,52 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2019, Linaro Ltd.
> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
> + */
> +
> +#if !defined(_TRACE_INTERCONNECT_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_INTERCONNECT_H
> +
> +#include <linux/tracepoint.h>
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM interconnect
> +
> +struct icc_node;
> +
> +TRACE_EVENT(icc_set_bw,
> +
> +	TP_PROTO(struct icc_node *n, const char *cdev, u32 avg_bw, u32 peak_bw),

	TP_PROTO(struct icc_node *n, const struct device *cdev,

> +
> +	TP_ARGS(n, cdev, avg_bw, peak_bw),
> +
> +	TP_STRUCT__entry(
> +		__string(node_name, n->name)
> +		__field(u32, node_avg_bw)
> +		__field(u32, node_peak_bw)
> +		__string(cdev, cdev)

		__string(cdev, dev_name(cdev))

> +		__field(u32, avg_bw)
> +		__field(u32, peak_bw)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(node_name, n->name);
> +		__entry->node_avg_bw = n->avg_bw;
> +		__entry->node_peak_bw = n->peak_bw;
> +		__assign_str(cdev, cdev);

		__assign_str(cdev, dev_name(cdev));

-- Steve

> +		__entry->avg_bw = avg_bw;
> +		__entry->peak_bw = peak_bw;
> +	),
> +
> +	TP_printk("%s avg_bw=%u peak_bw=%u cdev=%s avg_bw=%u peak_bw=%u",
> +		__get_str(node_name),
> +		__entry->node_avg_bw,
> +		__entry->node_peak_bw,
> +		__get_str(cdev),
> +		__entry->avg_bw,
> +		__entry->peak_bw)
> +);
> +#endif /* _TRACE_INTERCONNECT_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>

Re: [PATCH] interconnect: Add basic tracepoints

[Bjorn Andersson]

On Fri 18 Oct 07:02 PDT 2019, Georgi Djakov wrote:

> The tracepoints can help with understanding the system behavior of a
> given interconnect path when the consumer drivers change their bandwidth
> demands. This might be interesting when we want to monitor the requested
> interconnect bandwidth for each client driver. The paths may share the
> same nodes and this will help to understand "who and when is requesting
> what". All this is useful for subsystem drivers developers and may also
> provide hints when optimizing the power and performance profile of the
> system.
> 

This is very useful, thanks for writing it up.

> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
[..]
> @@ -449,6 +452,9 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>  
>  		/* aggregate requests for this node */
>  		aggregate_requests(node);
> +
> +		trace_icc_set_bw(node, dev_name(path->reqs[i].dev),
> +				 avg_bw, peak_bw);

When I've been debugging interconnect things I've added a
kstrdup_const() of "name" in of_icc_get() and then included that here.

I find including the path name quite useful for devices with multiple
paths.

>  	}
>  
>  	ret = apply_constraints(path);
> @@ -461,6 +467,9 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>  			path->reqs[i].avg_bw = old_avg;
>  			path->reqs[i].peak_bw = old_peak;
>  			aggregate_requests(node);
> +
> +			trace_icc_set_bw(node, dev_name(path->reqs[i].dev),
> +					 old_avg, old_peak);
>  		}
>  		apply_constraints(path);

And analog to e.g. the clock traces I would suggest that you trace
device, path and "ret" here.

Regards,
Bjorn

Re: [PATCH] interconnect: Add basic tracepoints

[Steven Rostedt]

On Fri, 18 Oct 2019 09:44:03 -0700
Bjorn Andersson <bjorn.andersson@linaro.org> wrote:

> > @@ -449,6 +452,9 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> >  
> >  		/* aggregate requests for this node */
> >  		aggregate_requests(node);
> > +
> > +		trace_icc_set_bw(node, dev_name(path->reqs[i].dev),
> > +				 avg_bw, peak_bw);  
> 
> When I've been debugging interconnect things I've added a
> kstrdup_const() of "name" in of_icc_get() and then included that here.
> 
> I find including the path name quite useful for devices with multiple
> paths.
> 
> >  	}
> >  
> >  	ret = apply_constraints(path);
> > @@ -461,6 +467,9 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> >  			path->reqs[i].avg_bw = old_avg;
> >  			path->reqs[i].peak_bw = old_peak;
> >  			aggregate_requests(node);
> > +
> > +			trace_icc_set_bw(node, dev_name(path->reqs[i].dev),
> > +					 old_avg, old_peak);
> >  		}
> >  		apply_constraints(path);  
> 
> And analog to e.g. the clock traces I would suggest that you trace
> device, path and "ret" here.

If you are going to switch to device name and path, please just pass in
the path to the trace point. Then have the TP_fast_assign() do the rest
of the work.

Thanks!

-- Steve