| Message ID | 3ef9cb911e9b51be55a874cacc847d44bca9877e.1643033113.git.bristot@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/8] rtla: Follow kernel version | expand |
On Mon, 24 Jan 2022 15:24:06 +0100 Daniel Bristot de Oliveira <bristot@kernel.org> wrote: > rtla osnoise top is causing a segmentation fault when running with > the --trace option on a kernel that does not support multiple > instances. For example: > > [root@f34 rtla]# rtla osnoise top -t > failed to enable the tracer osnoise > Could not enable osnoiser tracer for tracing > Failed to enable the trace instance > Segmentation fault (core dumped) > > This error happens because the exit code of the tools is trying > to destroy the trace instance that failed to be created. > > Rearrange the order in which trace instances are destroyed to avoid > this problem. > > Fixes: 1eceb2fc2ca5 ("rtla/osnoise: Add osnoise top mode") > Cc: Daniel Bristot de Oliveira <bristot@kernel.org> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: linux-kernel@vger.kernel.org > Cc: linux-trace-devel@vger.kernel.org > Signed-off-by: Daniel Bristot de Oliveira <bristot@kernel.org> > --- > tools/tracing/rtla/src/osnoise_top.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/tools/tracing/rtla/src/osnoise_top.c b/tools/tracing/rtla/src/osnoise_top.c > index 332b2ac205fc..546769bc7ff7 100644 > --- a/tools/tracing/rtla/src/osnoise_top.c > +++ b/tools/tracing/rtla/src/osnoise_top.c > @@ -546,7 +546,7 @@ int osnoise_top_main(int argc, char **argv) > trace); > if (retval < 0) { > err_msg("Error iterating on events\n"); > - goto out_top; > + goto out_trace; > } > > if (!params->quiet) > @@ -569,11 +569,12 @@ int osnoise_top_main(int argc, char **argv) > } > } > > +out_trace: > + if (params->trace_output) > + osnoise_destroy_tool(record); > out_top: > osnoise_free_top(tool->data); > osnoise_destroy_tool(tool); > - if (params->trace_output) > - osnoise_destroy_tool(record); Wouldn't these four patches be more robust if you just initialized record (and tool) to NULL, and change osnoise_destroy_tool() to: void osnoise_destroy_tool(struct osnoise_tool *top) { if (!top) return; trace_instance_destroy(&top->trace); if (top->context) osnoise_put_context(top->context); free(top); } Then you don't need these extra labels and if statements in the main code. -- Steve > out_exit: > exit(return_value); > }
On Thu, 3 Feb 2022 11:41:26 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > Wouldn't these four patches be more robust if you just initialized record > (and tool) to NULL, and change osnoise_destroy_tool() to: And if you do this, it should be one patch, not four. -- Steve > > void osnoise_destroy_tool(struct osnoise_tool *top) > { > if (!top) > return; > > trace_instance_destroy(&top->trace); > > if (top->context) > osnoise_put_context(top->context); > > free(top); > } > > Then you don't need these extra labels and if statements in the main code.
On 2/3/22 17:43, Steven Rostedt wrote: > On Thu, 3 Feb 2022 11:41:26 -0500 > Steven Rostedt <rostedt@goodmis.org> wrote: > >> Wouldn't these four patches be more robust if you just initialized record >> (and tool) to NULL, and change osnoise_destroy_tool() to: > And if you do this, it should be one patch, not four. Yeah, it works. The order would still be wrong, but it would be just an esthetic thing. I will send a v2 removing these four patches, and adding a patch with your suggestion. [ thinking aloud, is it possible to have multiple Fixes:? well, adding just one would also solve the issue, and... we are still in the same release ] Thanks, -- Daniel
On Thu, 3 Feb 2022 18:30:39 +0100 Daniel Bristot de Oliveira <bristot@kernel.org> wrote: > [ thinking aloud, is it possible to have multiple Fixes:? well, adding just one > would also solve the issue, and... we are still in the same release ] I've added multiple Fixes tags before. So sure. -- Steve
diff --git a/tools/tracing/rtla/src/osnoise_top.c b/tools/tracing/rtla/src/osnoise_top.c index 332b2ac205fc..546769bc7ff7 100644 --- a/tools/tracing/rtla/src/osnoise_top.c +++ b/tools/tracing/rtla/src/osnoise_top.c @@ -546,7 +546,7 @@ int osnoise_top_main(int argc, char **argv) trace); if (retval < 0) { err_msg("Error iterating on events\n"); - goto out_top; + goto out_trace; } if (!params->quiet) @@ -569,11 +569,12 @@ int osnoise_top_main(int argc, char **argv) } } +out_trace: + if (params->trace_output) + osnoise_destroy_tool(record); out_top: osnoise_free_top(tool->data); osnoise_destroy_tool(tool); - if (params->trace_output) - osnoise_destroy_tool(record); out_exit: exit(return_value); }
rtla osnoise top is causing a segmentation fault when running with the --trace option on a kernel that does not support multiple instances. For example: [root@f34 rtla]# rtla osnoise top -t failed to enable the tracer osnoise Could not enable osnoiser tracer for tracing Failed to enable the trace instance Segmentation fault (core dumped) This error happens because the exit code of the tools is trying to destroy the trace instance that failed to be created. Rearrange the order in which trace instances are destroyed to avoid this problem. Fixes: 1eceb2fc2ca5 ("rtla/osnoise: Add osnoise top mode") Cc: Daniel Bristot de Oliveira <bristot@kernel.org> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: linux-kernel@vger.kernel.org Cc: linux-trace-devel@vger.kernel.org Signed-off-by: Daniel Bristot de Oliveira <bristot@kernel.org> --- tools/tracing/rtla/src/osnoise_top.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)