| Message ID | 20221018181053.434508-2-urezki@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add basic trace events for vmap/vmalloc (v2) | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
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>
> 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 --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>
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