Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <cover.1602255803.git.zanussi@kernel.org>
In-Reply-To: <cover.1602255803.git.zanussi@kernel.org>
From:   Axel Rasmussen <axelrasmussen@google.com>
Date:   Fri, 9 Oct 2020 13:35:14 -0700
Message-ID: <CAJHvVchmmJ4LF-wY=BJgJSaEo7w6AhgKzsF9Q5HCDN276uv=HA@mail.gmail.com>
Subject: Re: [PATCH 0/5] tracing: Synthetic event dynamic string fixes
To:     Tom Zanussi <zanussi@kernel.org>
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Oct 9, 2020 at 8:17 AM Tom Zanussi <zanussi@kernel.org> wrote:
>
> These patches provide fixes for the problems observed by Masami in the
> new synthetic event dynamic string patchset.
>
> The first patch (tracing: Don't show dynamic string internals in
> synthetic event description) removes the __data_loc from the event
> description but leaves it in the format.
>
> The patch (tracing: Add synthetic event error logging) addresses the
> lack of error messages when parse errors occur.
>
> The remaining three patches address the other problems Masami noted
> which result from allowing illegal characters in synthetic event and
> field names when defining an event.  The is_good_name() function is
> used to check that's not possible for the probe events, but should
> also be used for the synthetic events as well.
>
> (tracing: Move is_good_name() from trace_probe.h to trace.h) makes
> that function available to other trace subsystems by putting it in
> trace.h.  (tracing: Check that the synthetic event and field names are
> legal) applies it to the synthetic events, and (selftests/ftrace:
> Change synthetic event name for inter-event-combined test) changes a
> testcase that now fails because it uses an illegal name.
>
> The following changes since commit 848183553e431e6e9c2ea2f72421a7a1bbc6532e:
>
>   tracing: Fix synthetic print fmt check for use of __get_str() (2020-10-08 15:29:07 -0400)
>
> are available in the Git repository at:
>
>   git://git.kernel.org/pub/scm/linux/kernel/git/zanussi/linux-trace.git ftrace/dynstring-fixes-v1

I applied this series, and then my mmap_lock tracepoints series, onto
v5.9-rc8. I played with the edge cases Masami raised in the other
thread, and I also tried constructing a synthetic event as we
discussed on the thread about my series.

As far as I can see, this addresses the edge cases Masami pointed out,
and it all seems to work as intended. It works fine with the kind of
synthetic event I'm hoping to define for my particular use case.

So, for what it's worth:

Tested-By: Axel Rasmussen <axelrasmussen@google.com>

>
> Tom Zanussi (5):
>   tracing: Don't show dynamic string internals in synthetic event
>     description
>   tracing: Move is_good_name() from trace_probe.h to trace.h
>   tracing: Check that the synthetic event and field names are legal
>   tracing: Add synthetic event error logging
>   selftests/ftrace: Change synthetic event name for inter-event-combined
>     test
>
>  kernel/trace/trace.h                          |  13 ++
>  kernel/trace/trace_events_synth.c             | 133 +++++++++++++++++-
>  kernel/trace/trace_probe.h                    |  13 --
>  .../trigger-inter-event-combined-hist.tc      |   8 +-
>  4 files changed, 147 insertions(+), 20 deletions(-)
>
> --
> 2.17.1
>