Linus,
[
I rebased my entire queue on top of the last pull request you took from
me. This made it so that I didn't need to do a merge with commits in that
branch. It also included fixes needed for the __assign_str() update.
The rebase removed the ring buffer user space mmapping and that will be
worked on later to fix the issues you had with it, and hopefully that can
go into the next merge window. Preparations for mapping are still there
like zeroing out the pages (that should be done anyway) and the snapshot
accounting.
I added the __string() and __assign_str() checks with fixes to users that
had that broken. The second parameter of __assign_str() is no longer used
but it is now checked to make sure that its the same as what is passed into
__string() so that the next merge window than second parameter can be
removed without causing regressions.
]
Tracing updates for 6.9:
Main user visible change:
- User events can now have "multi formats"
The current user events have a single format. If another event is created
with a different format, it will fail to be created. That is, once an
event name is used, it cannot be used again with a different format. This
can cause issues if a library is using an event and updates its format.
An application using the older format will prevent an application using
the new library from registering its event.
A task could also DOS another application if it knows the event names, and
it creates events with different formats.
The multi-format event is in a different name space from the single
format. Both the event name and its format are the unique identifier.
This will allow two different applications to use the same user event name
but with different payloads.
- Added support to have ftrace_dump_on_oops dump out instances and
not just the main top level tracing buffer.
Other changes:
- Add eventfs_root_inode
Only the root inode has a dentry that is static (never goes away) and
stores it upon creation. There's no reason that the thousands of other
eventfs inodes should have a pointer that never gets set in its
descriptor. Create a eventfs_root_inode desciptor that has a eventfs_inode
descriptor and a dentry pointer, and only the root inode will use this.
- Added WARN_ON()s in eventfs
There's some conditionals remaining in eventfs that should never be hit,
but instead of removing them, add WARN_ON() around them to make sure that
they are never hit.
- Have saved_cmdlines allocation also include the map_cmdline_to_pid array
The saved_cmdlines structure allocates a large amount of data to hold its
mappings. Within it, it has three arrays. Two are already apart of it:
map_pid_to_cmdline[] and saved_cmdlines[]. More memory can be saved by
also including the map_cmdline_to_pid[] array as well.
- Restructure __string() and __assign_str() macros used in TRACE_EVENT().
Dynamic strings in TRACE_EVENT() are declared with:
__string(name, source)
And assigned with:
__assign_str(name, source)
In the tracepoint callback of the event, the __string() is used to get the
size needed to allocate on the ring buffer and __assign_str() is used to
copy the string into the ring buffer. There's a helper structure that is
created in the TRACE_EVENT() macro logic that will hold the string length
and its position in the ring buffer which is created by __string().
There are several trace events that have a function to create the string
to save. This function is executed twice. Once for __string() and again
for __assign_str(). There's no reason for this. The helper structure could
also save the string it used in __string() and simply copy that into
__assign_str() (it also already has its length).
By using the structure to store the source string for the assignment, it
means that the second argument to __assign_str() is no longer needed.
It will be removed in the next merge window, but for now add a warning if
the source string given to __string() is different than the source string
given to __assign_str(), as the source to __assign_str() isn't even used
and will be going away.
- Added checks to make sure that the source of __string() is also the
source of __assign_str() so that it can be safely removed in the next
merge window.
Included fixes that the above check found.
- Other minor clean ups and fixes
Please pull the latest trace-v6.9-2 tree, which can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git
trace-v6.9-2
Tag SHA1: f40626f0ff0f9241d1d565a355c3375c8a007c69
Head SHA1: 7604256cecef34a82333d9f78262d3180f4eb525
Alison Schofield (1):
cxl/trace: Properly initialize cxl_poison region name
Beau Belgrave (4):
tracing/user_events: Prepare find/delete for same name events
tracing/user_events: Introduce multi-format events
selftests/user_events: Test multi-format events
tracing/user_events: Document multi-format flag
Huang Yiwei (1):
tracing: Support to dump instance traces by ftrace_dump_on_oops
John Garry (1):
tracing: Use init_utsname()->release
Randy Dunlap (1):
ftrace: Fix most kernel-doc warnings
Steven Rostedt (Google) (22):
eventfs: Add WARN_ON_ONCE() to checks in eventfs_root_lookup()
eventfs: Create eventfs_root_inode to store dentry
tracing: Have saved_cmdlines arrays all in one allocation
tracing: Move open coded processing of tgid_map into helper function
tracing: Move saved_cmdline code into trace_sched_switch.c
ring-buffer: Make wake once of ring_buffer_wait() more robust
NFSD: Fix nfsd_clid_class use of __string_len() macro
drm/i915: Add missing ; to __assign_str() macros in tracepoint code
net: hns3: tracing: fix hclgevf trace event strings
tracing: Rework __assign_str() and __string() to not duplicate getting the string
tracing: Do not calculate strlen() twice for __string() fields
tracing: Use ? : shortcut in trace macros
tracing: Use EVENT_NULL_STR macro instead of open coding "(null)"
tracing: Fix snapshot counter going between two tracers that use it
tracing: Decrement the snapshot if the snapshot trigger fails to register
tracing: Remove __assign_str_len()
tracing: Add __string_len() example
tracing: Add warning if string in __assign_str() does not match __string()
tracing: Remove second parameter to __assign_rel_str()
tracepoints: Use WARN() and not WARN_ON() for warnings
tracing: Use strcmp() in __assign_str() WARN_ON() check
tracing: Add __string_src() helper to help compilers not to get confused
Thorsten Blum (1):
tracing: Use div64_u64() instead of do_div()
Vincent Donnefort (2):
ring-buffer: Zero ring-buffer sub-buffers
tracing: Add snapshot refcount
linke li (1):
ring-buffer: use READ_ONCE() to read cpu_buffer->commit_page in concurrent environment
----
Documentation/admin-guide/kernel-parameters.txt | 26 +-
Documentation/admin-guide/sysctl/kernel.rst | 30 +-
Documentation/trace/user_events.rst | 27 +-
drivers/cxl/core/trace.h | 14 +-
drivers/gpu/drm/i915/display/intel_display_trace.h | 6 +-
.../ethernet/hisilicon/hns3/hns3pf/hclge_trace.h | 8 +-
.../ethernet/hisilicon/hns3/hns3vf/hclgevf_trace.h | 8 +-
fs/nfsd/trace.h | 10 +-
fs/tracefs/event_inode.c | 70 +-
fs/tracefs/internal.h | 2 -
include/linux/ftrace.h | 4 +-
include/linux/kernel.h | 1 +
include/linux/trace_events.h | 3 +
include/linux/tracepoint.h | 6 +-
include/trace/events/sunrpc.h | 12 +-
include/trace/stages/stage2_data_offsets.h | 4 +-
include/trace/stages/stage5_get_offsets.h | 25 +-
include/trace/stages/stage6_event_callback.h | 29 +-
include/uapi/linux/user_events.h | 6 +-
kernel/sysctl.c | 4 +-
kernel/trace/ftrace.c | 90 +--
kernel/trace/ring_buffer.c | 45 +-
kernel/trace/trace.c | 768 ++++++---------------
kernel/trace/trace.h | 18 +-
kernel/trace/trace_benchmark.c | 5 +-
kernel/trace/trace_events_trigger.c | 63 +-
kernel/trace/trace_events_user.c | 209 ++++--
kernel/trace/trace_sched_switch.c | 515 ++++++++++++++
kernel/trace/trace_selftest.c | 2 +-
samples/trace_events/trace-events-sample.h | 18 +-
tools/testing/selftests/user_events/abi_test.c | 134 ++++
31 files changed, 1363 insertions(+), 799 deletions(-)
---------------------------
The pull request you sent on Mon, 18 Mar 2024 11:30:53 -0400:
> git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace.git trace-v6.9-2
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/ad584d73a22b2f6e6b4c928956fdece5c44cdb3e
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
On Mon, 18 Mar 2024 at 08:28, Steven Rostedt <[email protected]> wrote:
>
> - Added checks to make sure that the source of __string() is also the
> source of __assign_str() so that it can be safely removed in the next
> merge window.
Aargh.
I didn't notice this initially, because it doesn't happen with gcc (or
maybe not with allmodconfig), but with clang I get
CC [M] net/sunrpc/sched.o
In file included from net/sunrpc/sched.c:31:
In file included from ./include/trace/events/sunrpc.h:2524:
In file included from ./include/trace/define_trace.h:102:
In file included from ./include/trace/trace_events.h:419:
include/trace/events/sunrpc.h:707:4: error: result of comparison
against a string literal is unspecified (use an explicit string
comparison function instead) [-Werror,-Wstring-compare]
and then about 250 lines ot messy "explanations" for how it was
expanded because it happens on line 709 too in the same macro, and it
ends up being three macros deep or something.
So no, this all needs to be re-done. That
WARN_ON_ONCE(__builtin_constant_p(src) ? \
strcmp((src), __data_offsets.dst##_ptr_) : \
(src) != __data_offsets.dst##_ptr_); \
does *NOT* work.
Also, looking at that __assign_str() macro, it seems literally insane.
On the next line it will do
memcpy(__str__, __data_offsets.dst##_ptr_ ? : \
EVENT_NULL_STR, __len__); \
so now it checks "__data_offsets.dst##_ptr_" for NULL - but that's one
line after it just did that strcmp on it.
WTF?
This code is completely bogus.
Linus
On Tue, 19 Mar 2024 09:23:10 -0700
Linus Torvalds <[email protected]> wrote:
> On Mon, 18 Mar 2024 at 08:28, Steven Rostedt <[email protected]> wrote:
> >
> > - Added checks to make sure that the source of __string() is also the
> > source of __assign_str() so that it can be safely removed in the next
> > merge window.
>
> Aargh.
>
> I didn't notice this initially, because it doesn't happen with gcc (or
> maybe not with allmodconfig), but with clang I get
>
> CC [M] net/sunrpc/sched.o
> In file included from net/sunrpc/sched.c:31:
> In file included from ./include/trace/events/sunrpc.h:2524:
> In file included from ./include/trace/define_trace.h:102:
> In file included from ./include/trace/trace_events.h:419:
> include/trace/events/sunrpc.h:707:4: error: result of comparison
> against a string literal is unspecified (use an explicit string
> comparison function instead) [-Werror,-Wstring-compare]
>
> and then about 250 lines ot messy "explanations" for how it was
> expanded because it happens on line 709 too in the same macro, and it
> ends up being three macros deep or something.
>
> So no, this all needs to be re-done. That
>
> WARN_ON_ONCE(__builtin_constant_p(src) ? \
> strcmp((src), __data_offsets.dst##_ptr_) : \
> (src) != __data_offsets.dst##_ptr_); \
>
> does *NOT* work.
>
> Also, looking at that __assign_str() macro, it seems literally insane.
> On the next line it will do
>
> memcpy(__str__, __data_offsets.dst##_ptr_ ? : \
> EVENT_NULL_STR, __len__); \
>
> so now it checks "__data_offsets.dst##_ptr_" for NULL - but that's one
> line after it just did that strcmp on it.
>
> WTF?
>
> This code is completely bogus.
The WARN_ON_ONCE() was added separately and missed that we do now allow it
to be NULL.
I'll fix that.
-- Steve
On Tue, 19 Mar 2024 09:23:10 -0700
Linus Torvalds <[email protected]> wrote:
> On Mon, 18 Mar 2024 at 08:28, Steven Rostedt <[email protected]> wrote:
> >
> > - Added checks to make sure that the source of __string() is also the
> > source of __assign_str() so that it can be safely removed in the next
> > merge window.
>
> Aargh.
>
> I didn't notice this initially, because it doesn't happen with gcc (or
> maybe not with allmodconfig), but with clang I get
>
> CC [M] net/sunrpc/sched.o
> In file included from net/sunrpc/sched.c:31:
> In file included from ./include/trace/events/sunrpc.h:2524:
> In file included from ./include/trace/define_trace.h:102:
> In file included from ./include/trace/trace_events.h:419:
> include/trace/events/sunrpc.h:707:4: error: result of comparison
> against a string literal is unspecified (use an explicit string
> comparison function instead) [-Werror,-Wstring-compare]
>
> and then about 250 lines ot messy "explanations" for how it was
> expanded because it happens on line 709 too in the same macro, and it
> ends up being three macros deep or something.
>
> So no, this all needs to be re-done. That
>
> WARN_ON_ONCE(__builtin_constant_p(src) ? \
> strcmp((src), __data_offsets.dst##_ptr_) : \
> (src) != __data_offsets.dst##_ptr_); \
>
> does *NOT* work.
In most all cases, src is not a constant and should always equal to what was
passed to __string(), but if it is a constant like "some string" then clang
warns that comparing pointers to strings is UB.
That is,
__string(src, mystring)
[..]
__assign_str(src, mystring);
works, but if it has:
__string(src, "this string");
then
__assign_str(src, "this string");
is UB due to the compiler having two different pointers to "this string".
I originally just had the "src != str" check but then it was reported that
clang complained about it. It still complained with the
__builtin_constant_p() but the code that it produced did the right thing.
This is in the fast path (where the trace event happens), but I can make it
always do strcmp(), even though it will slow down what is being recorded,
as I plan on removing the parameter in the next merge window anyway.
-- Steve
On Tue, 19 Mar 2024 13:06:53 -0400
Steven Rostedt <[email protected]> wrote:
> This is in the fast path (where the trace event happens), but I can make it
> always do strcmp(), even though it will slow down what is being recorded,
> as I plan on removing the parameter in the next merge window anyway.
I'll change it to this:
#define __assign_str(dst, src) \
do { \
char *__str__ = __get_str(dst); \
int __len__ = __get_dynamic_array_len(dst) - 1; \
WARN_ON_ONCE(!src != !__data_offsets.dst##_ptr_); \
WARN_ON_ONCE(src && strcmp((src), __data_offsets.dst##_ptr_)); \
memcpy(__str__, __data_offsets.dst##_ptr_ ? : \
EVENT_NULL_STR, __len__); \
__str__[__len__] = '\0'; \
} while (0)
The first WARN_ON() will report the bug if src is not NULL and the pointer
is even though the strcmp() will likely crash in that case. But that's a
bug if that happens anyway.
-- Steve
On Tue, Mar 19, 2024 at 01:13:33PM -0400, Steven Rostedt wrote:
> On Tue, 19 Mar 2024 13:06:53 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > This is in the fast path (where the trace event happens), but I can make it
> > always do strcmp(), even though it will slow down what is being recorded,
> > as I plan on removing the parameter in the next merge window anyway.
>
> I'll change it to this:
>
> #define __assign_str(dst, src) \
> do { \
> char *__str__ = __get_str(dst); \
> int __len__ = __get_dynamic_array_len(dst) - 1; \
> WARN_ON_ONCE(!src != !__data_offsets.dst##_ptr_); \
> WARN_ON_ONCE(src && strcmp((src), __data_offsets.dst##_ptr_)); \
> memcpy(__str__, __data_offsets.dst##_ptr_ ? : \
> EVENT_NULL_STR, __len__); \
> __str__[__len__] = '\0'; \
> } while (0)
>
> The first WARN_ON() will report the bug if src is not NULL and the pointer
> is even though the strcmp() will likely crash in that case. But that's a
> bug if that happens anyway.
For what it's worth, I applied that change and built ARCH=x86_64
defconfig with LLVM 18.1.1 from [1] but it does not appear to help the
instances of -Wstring-compare; in fact, it adds some additional warnings
that I have not seen before. I have attached the full build log.
[1]: https://mirrors.edge.kernel.org/pub/tools/llvm/
Cheers,
Nathan
On Tue, 19 Mar 2024 at 14:03, Nathan Chancellor <[email protected]> wrote:
>
> For what it's worth, I applied that change and built ARCH=x86_64
> defconfig with LLVM 18.1.1 from [1] but it does not appear to help the
> instances of -Wstring-compare; in fact, it adds some additional warnings
> that I have not seen before. I have attached the full build log.
Hmm. I'm no longer seeing any problems with commit 24f5bb9f24ad
("tracing: Just use strcmp() for testing __string() and __assign_str()
match").
But that's clang 17.0.6.
The patch that Steven sent out (and that I applied) is a bit different
from his "I'll change it to this" email, though. A couple of casts and
parentheses different.
So maybe current -git works for you?
Linus
On Tue, 19 Mar 2024 14:03:29 -0700
Nathan Chancellor <[email protected]> wrote:
> > #define __assign_str(dst, src) \
> > do { \
> > char *__str__ = __get_str(dst); \
> > int __len__ = __get_dynamic_array_len(dst) - 1; \
> > WARN_ON_ONCE(!src != !__data_offsets.dst##_ptr_); \
> > WARN_ON_ONCE(src && strcmp((src), __data_offsets.dst##_ptr_)); \
> > memcpy(__str__, __data_offsets.dst##_ptr_ ? : \
> > EVENT_NULL_STR, __len__); \
> > __str__[__len__] = '\0'; \
> > } while (0)
> >
> > The first WARN_ON() will report the bug if src is not NULL and the pointer
> > is even though the strcmp() will likely crash in that case. But that's a
> > bug if that happens anyway.
>
> For what it's worth, I applied that change and built ARCH=x86_64
> defconfig with LLVM 18.1.1 from [1] but it does not appear to help the
> instances of -Wstring-compare; in fact, it adds some additional warnings
> that I have not seen before. I have attached the full build log.
That was just me posting what I was going to do before testing it. You want
the actual patch that Linus pulled in.
https://lore.kernel.org/all/[email protected]/
-- Steve
On Tue, Mar 19, 2024 at 02:22:27PM -0700, Linus Torvalds wrote:
> On Tue, 19 Mar 2024 at 14:03, Nathan Chancellor <[email protected]> wrote:
> >
> > For what it's worth, I applied that change and built ARCH=x86_64
> > defconfig with LLVM 18.1.1 from [1] but it does not appear to help the
> > instances of -Wstring-compare; in fact, it adds some additional warnings
> > that I have not seen before. I have attached the full build log.
>
> Hmm. I'm no longer seeing any problems with commit 24f5bb9f24ad
> ("tracing: Just use strcmp() for testing __string() and __assign_str()
> match").
>
> But that's clang 17.0.6.
>
> The patch that Steven sent out (and that I applied) is a bit different
> from his "I'll change it to this" email, though. A couple of casts and
> parentheses different.
>
> So maybe current -git works for you?
Ah, I did not notice your tree was updated, I was working off of
b3603fcb79b1. Works for me here, thanks for letting me know.
Cheers,
Nathan