[PATCH] tracing: Kernel access to Ftrace instances.
Please review the patch that follows. Below are some details providing
the goal and justification for the patch.
=======================================================================
Goal:
Ftrace provides the feature “instances” that provides the capability to
create multiple Ftrace ring buffers. However, currently these buffers
are created/accessed via userspace only. The kernel APIs providing these
features are not exported, hence cannot be used by other kernel
components. We want to extend this infrastructure to provide the
flexibility to create/log/remove/ enable-disable existing trace events
to these buffers from within the kernel.
Justification:
1. We need module-specific/use-case specific ring buffers (apart
from the global trace buffer) to avoid overwrite by other components.
Hence, the need to use Ftrace "instances".
2. Flexibility to add additional logging to these module-specific
buffers via ksplice/live patch - Having a trace_printk counterpart for
these additional ring buffers.
3. Most often certain issues and events can be best monitored
within kernel.
4. Time sensitivity - We need the capability to dynamically enable
and disable tracing from within kernel to extract relevant debugging
info for the right time-window.
Example:
When the kernel detects an unexpected event such as connection drop (Eg:
RDS/NFS connection drops), we need the capability to enable specific
event tracing to capture relevant info during reconnect. This event
tracing will help us diagnose issues that occur during reconnect like
RCA longer reconnect times. In such cases we also want to disable the
tracing at the right moment and capture a snapshot from within kernel
to make sure we have the relevant diagnostics data and nothing is
overwritten or lost.
Note: The additional logging is not part of the kernel. We intend to
only provide the flexibility to add the logging as part of diagnostics
via ksplice/live-patch on need-basis.
Please find below the compilation of APIs to be introduced or exported as is.
We propose adding two new functions:
1. struct trace_array *trace_array_create(const char *name);
2. int trace_array_destroy(struct trace_array *tr);
In addition, we need to export functions:
3. int trace_array_printk(struct trace_array *tr, unsigned long ip,
const char *fmt, ...);
4. int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set);
5. void trace_printk_init_buffers(void);
To workaround the redundancy due to the newly introduced APIs, we propose the
following restructuring -
1. Move the contents of instance_mkdir to the new API.
static int instance_mkdir(const char *name)
{
return PTR_ERR_OR_ZERO(trace_array_create(name));
}
2. Introduce internal static function: __remove_instance(struct trace_array *tr)
This will be almost similar to old instance_rmdir which
identified the trace_array to be removed based on the name.
Modify existing API to use the internal function:
static int instance_rmdir(const char *name)
{
struct trace_array *tr;
int err = -ENODEV;
mutex_lock(&event_mutex);
mutex_lock(&trace_types_lock);
list_for_each_entry(tr, &ftrace_trace_arrays, list) {
if (tr->name && strcmp(tr->name, name) == 0) {
err = __remove_instance(tr);
break;
}
}
mutex_unlock(&trace_types_lock);
mutex_unlock(&event_mutex);
return err;
}
New API to be exported:
int trace_array_destroy(struct trace_array *tr)
{
int err;
mutex_lock(&event_mutex);
mutex_lock(&trace_types_lock);
err = __remove_instance(tr);
mutex_unlock(&trace_types_lock);
mutex_unlock(&event_mutex);
return err;
}
====================================================================================
Thanks,
Divya
Ftrace provides the feature “instances” that provides the capability to
create multiple Ftrace ring buffers. However, currently these buffers
are created/accessed via userspace only. The kernel APIs providing these
features are not exported, hence cannot be used by other kernel
components.
This patch aims to extend this infrastructure to provide the
flexibility to create/log/remove/ enable-disable existing trace events
to these buffers from within the kernel.
Signed-off-by: Divya Indi <[email protected]>
Reviewed-by: Joe Jin <[email protected]>
---
kernel/trace/trace.c | 74 ++++++++++++++++++++++++++++++---------------
kernel/trace/trace_events.c | 1 +
2 files changed, 51 insertions(+), 24 deletions(-)
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index c4238b4..eaf163a 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -2878,6 +2878,7 @@ void trace_printk_init_buffers(void)
if (global_trace.trace_buffer.buffer)
tracing_start_cmdline_record();
}
+EXPORT_SYMBOL_GPL(trace_printk_init_buffers);
void trace_printk_start_comm(void)
{
@@ -3038,6 +3039,7 @@ int trace_array_printk(struct trace_array *tr,
va_end(ap);
return ret;
}
+EXPORT_SYMBOL_GPL(trace_array_printk);
__printf(3, 4)
int trace_array_printk_buf(struct ring_buffer *buffer,
@@ -7832,7 +7834,7 @@ static void update_tracer_options(struct trace_array *tr)
mutex_unlock(&trace_types_lock);
}
-static int instance_mkdir(const char *name)
+struct trace_array *trace_array_create(const char *name)
{
struct trace_array *tr;
int ret;
@@ -7896,7 +7898,7 @@ static int instance_mkdir(const char *name)
mutex_unlock(&trace_types_lock);
mutex_unlock(&event_mutex);
- return 0;
+ return tr;
out_free_tr:
free_trace_buffers(tr);
@@ -7908,33 +7910,21 @@ static int instance_mkdir(const char *name)
mutex_unlock(&trace_types_lock);
mutex_unlock(&event_mutex);
- return ret;
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(trace_array_create);
+static int instance_mkdir(const char *name)
+{
+ return PTR_ERR_OR_ZERO(trace_array_create(name));
}
-static int instance_rmdir(const char *name)
+static int __remove_instance(struct trace_array *tr)
{
- struct trace_array *tr;
- int found = 0;
- int ret;
int i;
- mutex_lock(&event_mutex);
- mutex_lock(&trace_types_lock);
-
- ret = -ENODEV;
- list_for_each_entry(tr, &ftrace_trace_arrays, list) {
- if (tr->name && strcmp(tr->name, name) == 0) {
- found = 1;
- break;
- }
- }
- if (!found)
- goto out_unlock;
-
- ret = -EBUSY;
if (tr->ref || (tr->current_trace && tr->current_trace->ref))
- goto out_unlock;
+ return -EBUSY;
list_del(&tr->list);
@@ -7960,10 +7950,46 @@ static int instance_rmdir(const char *name)
free_cpumask_var(tr->tracing_cpumask);
kfree(tr->name);
kfree(tr);
+ tr = NULL;
- ret = 0;
+ return 0;
+}
+
+int trace_array_destroy(struct trace_array *tr)
+{
+ int ret;
+
+ if (!tr)
+ return -EINVAL;
+
+ mutex_lock(&event_mutex);
+ mutex_lock(&trace_types_lock);
+
+ ret = __remove_instance(tr);
+
+ mutex_unlock(&trace_types_lock);
+ mutex_unlock(&event_mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(trace_array_destroy);
+
+static int instance_rmdir(const char *name)
+{
+ struct trace_array *tr;
+ int ret;
+
+ mutex_lock(&event_mutex);
+ mutex_lock(&trace_types_lock);
+
+ ret = -ENODEV;
+ list_for_each_entry(tr, &ftrace_trace_arrays, list) {
+ if (tr->name && strcmp(tr->name, name) == 0) {
+ ret = __remove_instance(tr);
+ break;
+ }
+ }
- out_unlock:
mutex_unlock(&trace_types_lock);
mutex_unlock(&event_mutex);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 5b3b0c3..81c038e 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -832,6 +832,7 @@ static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
return ret;
}
+EXPORT_SYMBOL_GPL(ftrace_set_clr_event);
/**
* trace_set_clr_event - enable or disable an event
--
1.8.3.1
On Wed, 20 Mar 2019 11:28:51 -0700
Divya Indi <[email protected]> wrote:
> Ftrace provides the feature “instances” that provides the capability to
> create multiple Ftrace ring buffers. However, currently these buffers
> are created/accessed via userspace only. The kernel APIs providing these
> features are not exported, hence cannot be used by other kernel
> components.
>
> This patch aims to extend this infrastructure to provide the
> flexibility to create/log/remove/ enable-disable existing trace events
> to these buffers from within the kernel.
>
> Signed-off-by: Divya Indi <[email protected]>
> Reviewed-by: Joe Jin <[email protected]>
> ---
I added this to my queue, for 5.2 and then we'll see what it will do.
-- Steve
HI Divya,
On Wed, 20 Mar 2019 11:28:51 -0700
Divya Indi <[email protected]> wrote:
> Ftrace provides the feature $B!H(Binstances$B!I(B that provides the capability to
> create multiple Ftrace ring buffers. However, currently these buffers
> are created/accessed via userspace only. The kernel APIs providing these
> features are not exported, hence cannot be used by other kernel
> components.
>
> This patch aims to extend this infrastructure to provide the
> flexibility to create/log/remove/ enable-disable existing trace events
> to these buffers from within the kernel.
>
> Signed-off-by: Divya Indi <[email protected]>
> Reviewed-by: Joe Jin <[email protected]>
Would you tested these APIs with your module? Since,
[...]
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 5b3b0c3..81c038e 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -832,6 +832,7 @@ static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(ftrace_set_clr_event);
I found this exports a static function to module. Did it work?
Thank you,
--
Masami Hiramatsu <[email protected]>
Hi Masami,
Thanks for pointing it out.
Yes, it should not be static.
On 5/15/19 5:09 PM, Masami Hiramatsu wrote:
> HI Divya,
>
> On Wed, 20 Mar 2019 11:28:51 -0700
> Divya Indi <[email protected]> wrote:
>
>> Ftrace provides the feature “instances” that provides the capability to
>> create multiple Ftrace ring buffers. However, currently these buffers
>> are created/accessed via userspace only. The kernel APIs providing these
>> features are not exported, hence cannot be used by other kernel
>> components.
>>
>> This patch aims to extend this infrastructure to provide the
>> flexibility to create/log/remove/ enable-disable existing trace events
>> to these buffers from within the kernel.
>>
>> Signed-off-by: Divya Indi <[email protected]>
>> Reviewed-by: Joe Jin <[email protected]>
> Would you tested these APIs with your module? Since,
> [...]
>> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
>> index 5b3b0c3..81c038e 100644
>> --- a/kernel/trace/trace_events.c
>> +++ b/kernel/trace/trace_events.c
>> @@ -832,6 +832,7 @@ static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
>>
>> return ret;
>> }
>> +EXPORT_SYMBOL_GPL(ftrace_set_clr_event);
> I found this exports a static function to module. Did it work?
I had tested the changes with my module. This change to static was added
in the test patch, but somehow missed it in the final patch that was
sent out.
Will send a new patch along with a few additional ones to add some NULL
checks to ensure safe usage by modules and add the APIs to a header file
that can be used by the modules.
Thanks,
Divya
>
> Thank you,
>
Hi,
On Wed, 15 May 2019 18:51:23 -0700
Divya Indi <[email protected]> wrote:
> Hi Masami,
>
> Thanks for pointing it out.
>
> Yes, it should not be static.
>
> On 5/15/19 5:09 PM, Masami Hiramatsu wrote:
> > HI Divya,
> >
> > On Wed, 20 Mar 2019 11:28:51 -0700
> > Divya Indi <[email protected]> wrote:
> >
> >> Ftrace provides the feature $B!H(Binstances$B!I(B that provides the capability to
> >> create multiple Ftrace ring buffers. However, currently these buffers
> >> are created/accessed via userspace only. The kernel APIs providing these
> >> features are not exported, hence cannot be used by other kernel
> >> components.
> >>
> >> This patch aims to extend this infrastructure to provide the
> >> flexibility to create/log/remove/ enable-disable existing trace events
> >> to these buffers from within the kernel.
> >>
> >> Signed-off-by: Divya Indi <[email protected]>
> >> Reviewed-by: Joe Jin <[email protected]>
> > Would you tested these APIs with your module? Since,
> > [...]
> >> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> >> index 5b3b0c3..81c038e 100644
> >> --- a/kernel/trace/trace_events.c
> >> +++ b/kernel/trace/trace_events.c
> >> @@ -832,6 +832,7 @@ static int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
> >>
> >> return ret;
> >> }
> >> +EXPORT_SYMBOL_GPL(ftrace_set_clr_event);
> > I found this exports a static function to module. Did it work?
>
> I had tested the changes with my module. This change to static was added
> in the test patch, but somehow missed it in the final patch that was
> sent out.
If you can send some example module patch under samples/, that is more
helpful for us to check it. And it is possible to use in kselftest too.
>
> Will send a new patch along with a few additional ones to add some NULL
> checks to ensure safe usage by modules and add the APIs to a header file
> that can be used by the modules.
It seems your's already in Steve's ftrace/core branch, so I think you can make
additional patch to fix it. Steve, is that OK?
Thank you,
>
> Thanks,
>
> Divya
>
> >
> > Thank you,
> >
--
Masami Hiramatsu <[email protected]>
On May 15, 2019 8:05:29 PM PDT, Masami Hiramatsu <[email protected]> wrote:
>
>> >> diff --git a/kernel/trace/trace_events.c
>b/kernel/trace/trace_events.c
>> >> index 5b3b0c3..81c038e 100644
>> >> --- a/kernel/trace/trace_events.c
>> >> +++ b/kernel/trace/trace_events.c
>> >> @@ -832,6 +832,7 @@ static int ftrace_set_clr_event(struct
>trace_array *tr, char *buf, int set)
>> >>
>> >> return ret;
>> >> }
>> >> +EXPORT_SYMBOL_GPL(ftrace_set_clr_event);
>> > I found this exports a static function to module. Did it work?
>>
>> I had tested the changes with my module. This change to static was
>added
>> in the test patch, but somehow missed it in the final patch that was
>> sent out.
>
>If you can send some example module patch under samples/, that is more
>helpful for us to check it. And it is possible to use in kselftest too.
>
>>
>> Will send a new patch along with a few additional ones to add some
>NULL
>> checks to ensure safe usage by modules and add the APIs to a header
>file
>> that can be used by the modules.
>
>It seems your's already in Steve's ftrace/core branch, so I think you
>can make
>additional patch to fix it. Steve, is that OK?
>
Yes. In fact I already sent a pull request to Linus. Please send a patch on top of my ftrace/core branch.
Thanks,
-- Steve
>Thank you,
>
>>
>> Thanks,
>>
>> Divya
>>
>> >
>> > Thank you,
>> >
--
Sent from my Android device with K-9 Mail. Please excuse my brevity and top posting.
On Wed, 15 May 2019 20:24:43 -0700
Steven Rostedt <[email protected]> wrote:
>
> >It seems your's already in Steve's ftrace/core branch, so I think you
> >can make
> >additional patch to fix it. Steve, is that OK?
> >
>
> Yes. In fact I already sent a pull request to Linus. Please send a patch on top of my ftrace/core branch.
>
And now it is in mainline (v5.2-rc1). Please send a patch with a sample
module (as Masami requested). Also, that function not only needs to be
changed to not being static, you need to add it to a header
(include/linux/trace_events.h?)
Thanks!
-- Steve
On 05/20/2019 08:23 AM, Steven Rostedt wrote:
> On Wed, 15 May 2019 20:24:43 -0700
> Steven Rostedt <[email protected]> wrote:
>>> It seems your's already in Steve's ftrace/core branch, so I think you
>>> can make
>>> additional patch to fix it. Steve, is that OK?
>>>
>> Yes. In fact I already sent a pull request to Linus. Please send a patch on top of my ftrace/core branch.
>>
> And now it is in mainline (v5.2-rc1). Please send a patch with a sample
> module (as Masami requested). Also, that function not only needs to be
> changed to not being static, you need to add it to a header
> (include/linux/trace_events.h?)
>
> Thanks!
Working on it. Will send it out soon.
Thanks,
Divya
>
> -- Steve