From: Tom Zanussi <zanussi@kernel.org>
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: rostedt@goodmis.org, artem.bityutskiy@linux.intel.com,
linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org
Subject: Re: [PATCH 0/7] tracing: Add support for in-kernel synthetic event API
Date: Thu, 19 Dec 2019 10:24:27 -0600 [thread overview]
Message-ID: <1576772667.2236.17.camel@kernel.org> (raw)
In-Reply-To: <20191219234511.bb499b3d1590059506db6982@kernel.org>
Hi Masami,
On Thu, 2019-12-19 at 23:45 +0900, Masami Hiramatsu wrote:
> Hello Tom,
>
> On Wed, 18 Dec 2019 09:27:36 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
>
> > Hi,
> >
> > I've recently had several requests and suggestions from users to
> > add
> > support for the creation and generation of synthetic events from
> > kernel code such as modules, and not just from the available
> > command
> > line commands.
>
> This reminds me my recent series of patches.
>
> Could you use synth_event_run_command() for this purpose as I did
> in boot-time tracing series?
>
> https://lkml.kernel.org/r/157528179955.22451.16317363776831311868.stg
> it@devnote2
>
> Above example uses a command string as same as command string, but I
> think
> we can introduce some macros to construct the command string for
> easier
> definition.
>
> Or maybe it is possible to pass the const char *args[] array to that
> API,
> instead of single char *cmd. (for dynamic event definiton)
>
> Maybe we should consider more generic APIs for modules to
> create/delete
> dynamic-event including synthetic and probes, and those might reuse
> existing command parsers.
>
I'll have to look at your patches more closely, but I think it should
be possible to generate the command string synth_event_run_command()
needs, or the equivalent const char *args[] array you mentioned, from
the higher-level event definition in my patches.
So the two ways of defining an event in my patches is 1) from a static
array known at build-time defined like this:
static struct synth_field_desc synthtest_fields[] = {
{ .type = "pid_t", .name = "next_pid_field" },
{ .type = "char[16]", .name = "next_comm_field" },
{ .type = "u64", .name = "ts_ns" },
{ .type = "u64", .name = "ts_ms" },
{ .type = "unsigned int", .name = "cpu" },
{ .type = "char[64]", .name = "my_string_field" },
{ .type = "int", .name = "my_int_field" },
};
which is then passed to create_synth_event(&synthtest_fields)
or 2) at run-time by adding fields individually as they become known:
add_synth_field("type", "name")
which requires some sort of start/end("event name").
I think that should all be possible using your patches, and maybe
generalizable to not just synth events by removing _synth_ from
everything? Let me know what you think. If that's correct, I can go
and rewrite the event creation part on top of your patches.
> > This patchset adds support for that. The first three patches add
> > some
> > minor preliminary setup, followed by the two main patches that add
> > the
> > ability to create and generate synthetic events from the
> > kernel. The
> > next patch adds a test module that demonstrates actual use of the
> > API
> > and verifies that it works as intended, followed by Documentation.
>
> Could you also check the locks are correctly acquired? It seems that
> your APIs doesn't lock it.
>
I did notice that I said that trace_types_lock and event_mutex should
be held for trace_array_find() but it should only be trace_types_lock,
and then I missed doing that in get_event_file() in one place. And I
also don't really need the nolock versions, so will simplify and remove
them. I think elsewhere event_mutex is taken where needed. But if
talking about something else, please let me know.
Thanks,
Tom
>
> Thank you,
>
>
next prev parent reply other threads:[~2019-12-19 16:24 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-18 15:27 [PATCH 0/7] tracing: Add support for in-kernel synthetic event API Tom Zanussi
2019-12-18 15:27 ` [PATCH 1/7] tracing: Add trace_array_find() to find instance trace arrays Tom Zanussi
2019-12-18 15:27 ` [PATCH 2/7] tracing: Add get/put_event_file() Tom Zanussi
2019-12-18 15:27 ` [PATCH 3/7] tracing: Add delete_synth_event() Tom Zanussi
2019-12-18 15:27 ` [PATCH 4/7] tracing: Add create_synth_event() Tom Zanussi
2019-12-18 15:27 ` [PATCH 5/7] tracing: Add generate_synth_event() and related functions Tom Zanussi
2019-12-18 15:27 ` [PATCH 6/7] tracing: Add synth event generation test module Tom Zanussi
2019-12-18 15:27 ` [PATCH 7/7] tracing: Documentation for in-kernel synthetic event API Tom Zanussi
2019-12-19 14:45 ` [PATCH 0/7] tracing: Add support " Masami Hiramatsu
2019-12-19 16:24 ` Tom Zanussi [this message]
2019-12-20 8:41 ` Masami Hiramatsu
2019-12-20 16:24 ` Tom Zanussi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1576772667.2236.17.camel@kernel.org \
--to=zanussi@kernel.org \
--cc=artem.bityutskiy@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rt-users@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).