diff mbox series

[v2,1/7] mm: vmalloc: Add alloc_vmap_area trace event

Message ID 20221018181053.434508-2-urezki@gmail.com
State New
Headers
Series Add basic trace events for vmap/vmalloc (v2) |

Commit Message

Uladzislau Rezki Oct. 18, 2022, 6:10 p.m. UTC 
  It is for a debug purpose and for validation of passed parameters.

Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
---
 include/trace/events/vmalloc.h | 56 ++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 include/trace/events/vmalloc.h

Comments

Christoph Hellwig Oct. 20, 2022, 11:22 a.m. UTC | #1 
Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Steven Rostedt Nov. 14, 2022, 3:53 p.m. UTC | #2 
On Tue, 18 Oct 2022 20:10:47 +0200
"Uladzislau Rezki (Sony)" <urezki@gmail.com> wrote:

> It is for a debug purpose and for validation of passed parameters.
> 
> Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> ---
>  include/trace/events/vmalloc.h | 56 ++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 include/trace/events/vmalloc.h
> 
> diff --git a/include/trace/events/vmalloc.h b/include/trace/events/vmalloc.h
> new file mode 100644
> index 000000000000..39fbd77c91e7
> --- /dev/null
> +++ b/include/trace/events/vmalloc.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM vmalloc
> +
> +#if !defined(_TRACE_VMALLOC_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_VMALLOC_H
> +
> +#include <linux/tracepoint.h>
> +
> +/**
> + * alloc_vmap_area - called when a new vmap allocation occurs
> + * @addr:	an allocated address
> + * @size:	a requested size
> + * @align:	a requested alignment
> + * @vstart:	a requested start range
> + * @vend:	a requested end range
> + * @failed:	an allocation failed or not
> + *
> + * This event is used for a debug purpose, it can give an extra
> + * information for a developer about how often it occurs and which
> + * parameters are passed for further validation.
> + */
> +TRACE_EVENT(alloc_vmap_area,
> +
> +	TP_PROTO(unsigned long addr, unsigned long size, unsigned long align,
> +		unsigned long vstart, unsigned long vend, int failed),
> +
> +	TP_ARGS(addr, size, align, vstart, vend, failed),

The above is passed in via (from patch 4):


@@ -1621,6 +1624,8 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
 		size, align, vstart, vend);
 	spin_unlock(&free_vmap_area_lock);
 
+	trace_alloc_vmap_area(addr, size, align, vstart, vend, addr == vend);
+
 	/*
 	 * If an allocation fails, the "vend" address is
 	 * returned. Therefore trigger the overflow path.

> +
> +	TP_STRUCT__entry(
> +		__field(unsigned long, addr)
> +		__field(unsigned long, size)
> +		__field(unsigned long, align)
> +		__field(unsigned long, vstart)
> +		__field(unsigned long, vend)

> +		__field(int, failed)

I would drop the failed field...

> +	),
> +
> +	TP_fast_assign(
> +		__entry->addr = addr;
> +		__entry->size = size;
> +		__entry->align = align;
> +		__entry->vstart = vstart;
> +		__entry->vend = vend;

And instead have:

		__entry->failed = addr == vend;

Why pass in a parameter that can be calculated in the trace event logic?

Other than that, from a tracing perspective:

  Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>

for the series.

-- Steve


> +		__entry->failed = failed;
> +	),
> +
> +	TP_printk("va_start: %lu size=%lu align=%lu vstart=0x%lx vend=0x%lx failed=%d",
> +		__entry->addr, __entry->size, __entry->align,
> +		__entry->vstart, __entry->vend, __entry->failed)
> +);
> +
> +#endif /*  _TRACE_VMALLOC_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
Uladzislau Rezki Nov. 14, 2022, 4:55 p.m. UTC | #3 
> On Tue, 18 Oct 2022 20:10:47 +0200
> "Uladzislau Rezki (Sony)" <urezki@gmail.com> wrote:
> 
> > It is for a debug purpose and for validation of passed parameters.
> > 
> > Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
> > ---
> >  include/trace/events/vmalloc.h | 56 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >  create mode 100644 include/trace/events/vmalloc.h
> > 
> > diff --git a/include/trace/events/vmalloc.h b/include/trace/events/vmalloc.h
> > new file mode 100644
> > index 000000000000..39fbd77c91e7
> > --- /dev/null
> > +++ b/include/trace/events/vmalloc.h
> > @@ -0,0 +1,56 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM vmalloc
> > +
> > +#if !defined(_TRACE_VMALLOC_H) || defined(TRACE_HEADER_MULTI_READ)
> > +#define _TRACE_VMALLOC_H
> > +
> > +#include <linux/tracepoint.h>
> > +
> > +/**
> > + * alloc_vmap_area - called when a new vmap allocation occurs
> > + * @addr:	an allocated address
> > + * @size:	a requested size
> > + * @align:	a requested alignment
> > + * @vstart:	a requested start range
> > + * @vend:	a requested end range
> > + * @failed:	an allocation failed or not
> > + *
> > + * This event is used for a debug purpose, it can give an extra
> > + * information for a developer about how often it occurs and which
> > + * parameters are passed for further validation.
> > + */
> > +TRACE_EVENT(alloc_vmap_area,
> > +
> > +	TP_PROTO(unsigned long addr, unsigned long size, unsigned long align,
> > +		unsigned long vstart, unsigned long vend, int failed),
> > +
> > +	TP_ARGS(addr, size, align, vstart, vend, failed),
> 
> The above is passed in via (from patch 4):
> 
> 
> @@ -1621,6 +1624,8 @@ static struct vmap_area *alloc_vmap_area(unsigned long size,
>  		size, align, vstart, vend);
>  	spin_unlock(&free_vmap_area_lock);
>  
> +	trace_alloc_vmap_area(addr, size, align, vstart, vend, addr == vend);
> +
>  	/*
>  	 * If an allocation fails, the "vend" address is
>  	 * returned. Therefore trigger the overflow path.
> 
> > +
> > +	TP_STRUCT__entry(
> > +		__field(unsigned long, addr)
> > +		__field(unsigned long, size)
> > +		__field(unsigned long, align)
> > +		__field(unsigned long, vstart)
> > +		__field(unsigned long, vend)
> 
> > +		__field(int, failed)
> 
> I would drop the failed field...
> 
> > +	),
> > +
> > +	TP_fast_assign(
> > +		__entry->addr = addr;
> > +		__entry->size = size;
> > +		__entry->align = align;
> > +		__entry->vstart = vstart;
> > +		__entry->vend = vend;
> 
> And instead have:
> 
> 		__entry->failed = addr == vend;
> 
> Why pass in a parameter that can be calculated in the trace event logic?
>
It can be. A condition about when it is failed or not is taken on upper
level because it might be changed afterwards. So a trace event is not
aware about it thus no need in adaptation.

But i do not have a strong opinion here. I can prepare a patch to
eliminate it.

What is your preference?

--
Uladzislau Rezki
diff mbox series

Patch

diff --git a/include/trace/events/vmalloc.h b/include/trace/events/vmalloc.h
new file mode 100644
index 000000000000..39fbd77c91e7
--- /dev/null
+++ b/include/trace/events/vmalloc.h
@@ -0,0 +1,56 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM vmalloc
+
+#if !defined(_TRACE_VMALLOC_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_VMALLOC_H
+
+#include <linux/tracepoint.h>
+
+/**
+ * alloc_vmap_area - called when a new vmap allocation occurs
+ * @addr:	an allocated address
+ * @size:	a requested size
+ * @align:	a requested alignment
+ * @vstart:	a requested start range
+ * @vend:	a requested end range
+ * @failed:	an allocation failed or not
+ *
+ * This event is used for a debug purpose, it can give an extra
+ * information for a developer about how often it occurs and which
+ * parameters are passed for further validation.
+ */
+TRACE_EVENT(alloc_vmap_area,
+
+	TP_PROTO(unsigned long addr, unsigned long size, unsigned long align,
+		unsigned long vstart, unsigned long vend, int failed),
+
+	TP_ARGS(addr, size, align, vstart, vend, failed),
+
+	TP_STRUCT__entry(
+		__field(unsigned long, addr)
+		__field(unsigned long, size)
+		__field(unsigned long, align)
+		__field(unsigned long, vstart)
+		__field(unsigned long, vend)
+		__field(int, failed)
+	),
+
+	TP_fast_assign(
+		__entry->addr = addr;
+		__entry->size = size;
+		__entry->align = align;
+		__entry->vstart = vstart;
+		__entry->vend = vend;
+		__entry->failed = failed;
+	),
+
+	TP_printk("va_start: %lu size=%lu align=%lu vstart=0x%lx vend=0x%lx failed=%d",
+		__entry->addr, __entry->size, __entry->align,
+		__entry->vstart, __entry->vend, __entry->failed)
+);
+
+#endif /*  _TRACE_VMALLOC_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>