* [PATCH V2] cgroup: Trace event cgroup id fields should be u64 @ 2021-12-01 16:07 William Kucharski 2021-12-01 16:14 ` Steven Rostedt 0 siblings, 1 reply; 4+ messages in thread From: William Kucharski @ 2021-12-01 16:07 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar, Tejun Heo, Greg Kroah-Hartman, LKML Cc: william.kucharski Various trace event fields that store cgroup IDs were declared as ints, but cgroup_id(() returns a u64 and the structures and associated TP_printk() calls were not updated to reflect this. Fixes: 743210386c03 ("cgroup: use cgrp->kn->id as the cgroup ID") Signed-off-by: William Kucharski <william.kucharski@oracle.com> --- V2: Do not remove spaces from macro arguments include/trace/events/cgroup.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/include/trace/events/cgroup.h b/include/trace/events/cgroup.h index 7f42a3de59e6..1b68c842ac46 100644 --- a/include/trace/events/cgroup.h +++ b/include/trace/events/cgroup.h @@ -59,7 +59,7 @@ DECLARE_EVENT_CLASS(cgroup, TP_STRUCT__entry( __field( int, root ) - __field( int, id ) + __field( u64, id ) __field( int, level ) __string( path, path ) ), @@ -71,7 +71,7 @@ DECLARE_EVENT_CLASS(cgroup, __assign_str(path, path); ), - TP_printk("root=%d id=%d level=%d path=%s", + TP_printk("root=%d id=%llu level=%d path=%s", __entry->root, __entry->id, __entry->level, __get_str(path)) ); @@ -126,7 +126,7 @@ DECLARE_EVENT_CLASS(cgroup_migrate, TP_STRUCT__entry( __field( int, dst_root ) - __field( int, dst_id ) + __field( u64, dst_id ) __field( int, dst_level ) __field( int, pid ) __string( dst_path, path ) @@ -142,7 +142,7 @@ DECLARE_EVENT_CLASS(cgroup_migrate, __assign_str(comm, task->comm); ), - TP_printk("dst_root=%d dst_id=%d dst_level=%d dst_path=%s pid=%d comm=%s", + TP_printk("dst_root=%d dst_id=%llu dst_level=%d dst_path=%s pid=%d comm=%s", __entry->dst_root, __entry->dst_id, __entry->dst_level, __get_str(dst_path), __entry->pid, __get_str(comm)) ); @@ -171,7 +171,7 @@ DECLARE_EVENT_CLASS(cgroup_event, TP_STRUCT__entry( __field( int, root ) - __field( int, id ) + __field( u64, id ) __field( int, level ) __string( path, path ) __field( int, val ) @@ -185,7 +185,7 @@ DECLARE_EVENT_CLASS(cgroup_event, __entry->val = val; ), - TP_printk("root=%d id=%d level=%d path=%s val=%d", + TP_printk("root=%d id=%llu level=%d path=%s val=%d", __entry->root, __entry->id, __entry->level, __get_str(path), __entry->val) ); -- 2.33.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH V2] cgroup: Trace event cgroup id fields should be u64 2021-12-01 16:07 [PATCH V2] cgroup: Trace event cgroup id fields should be u64 William Kucharski @ 2021-12-01 16:14 ` Steven Rostedt 2021-12-01 16:27 ` William Kucharski 0 siblings, 1 reply; 4+ messages in thread From: Steven Rostedt @ 2021-12-01 16:14 UTC (permalink / raw) To: William Kucharski; +Cc: Ingo Molnar, Tejun Heo, Greg Kroah-Hartman, LKML On Wed, 1 Dec 2021 09:07:46 -0700 William Kucharski <william.kucharski@oracle.com> wrote: > Various trace event fields that store cgroup IDs were declared as > ints, but cgroup_id(() returns a u64 and the structures and associated > TP_printk() calls were not updated to reflect this. > > Fixes: 743210386c03 ("cgroup: use cgrp->kn->id as the cgroup ID") > Signed-off-by: William Kucharski <william.kucharski@oracle.com> > --- > V2: Do not remove spaces from macro arguments > > include/trace/events/cgroup.h | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/include/trace/events/cgroup.h b/include/trace/events/cgroup.h > index 7f42a3de59e6..1b68c842ac46 100644 > --- a/include/trace/events/cgroup.h > +++ b/include/trace/events/cgroup.h > @@ -59,7 +59,7 @@ DECLARE_EVENT_CLASS(cgroup, > > TP_STRUCT__entry( > __field( int, root ) > - __field( int, id ) > + __field( u64, id ) > __field( int, level ) It's best to move them around to prevent holes. This is not a packed structure, and the above will create a structure on the ring buffer that looks like: int root; u64 id; int level; short path_offset; short path_size; (the string() macro is a 4 byte word where the half is the offset of the actual string and the other half is the size of the string). Having a 8 byte word between two 4 byte words, will add a 4 byte padding after the first 4 byte word (before the 8 byte word), and waste space on the ring buffer. Better to move it around to be: TP_STRUCT__entry( __field( int, root ) __field( int, level ) __field( u64, id ) That way the two 4 byte words will be together followed directly by the 8 byte word. > __string( path, path ) > ), > @@ -71,7 +71,7 @@ DECLARE_EVENT_CLASS(cgroup, > __assign_str(path, path); > ), > > - TP_printk("root=%d id=%d level=%d path=%s", > + TP_printk("root=%d id=%llu level=%d path=%s", > __entry->root, __entry->id, __entry->level, __get_str(path)) > ); > > @@ -126,7 +126,7 @@ DECLARE_EVENT_CLASS(cgroup_migrate, > > TP_STRUCT__entry( > __field( int, dst_root ) > - __field( int, dst_id ) > + __field( u64, dst_id ) Same here. Just move dst_level above dst_id. > __field( int, dst_level ) > __field( int, pid ) > __string( dst_path, path ) > @@ -142,7 +142,7 @@ DECLARE_EVENT_CLASS(cgroup_migrate, > __assign_str(comm, task->comm); > ), > > - TP_printk("dst_root=%d dst_id=%d dst_level=%d dst_path=%s pid=%d comm=%s", > + TP_printk("dst_root=%d dst_id=%llu dst_level=%d dst_path=%s pid=%d comm=%s", > __entry->dst_root, __entry->dst_id, __entry->dst_level, > __get_str(dst_path), __entry->pid, __get_str(comm)) > ); > @@ -171,7 +171,7 @@ DECLARE_EVENT_CLASS(cgroup_event, > > TP_STRUCT__entry( > __field( int, root ) > - __field( int, id ) > + __field( u64, id ) And here. > __field( int, level ) > __string( path, path ) > __field( int, val ) > @@ -185,7 +185,7 @@ DECLARE_EVENT_CLASS(cgroup_event, > __entry->val = val; > ), > > - TP_printk("root=%d id=%d level=%d path=%s val=%d", > + TP_printk("root=%d id=%llu level=%d path=%s val=%d", > __entry->root, __entry->id, __entry->level, __get_str(path), > __entry->val) > ); -- Steve ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V2] cgroup: Trace event cgroup id fields should be u64 2021-12-01 16:14 ` Steven Rostedt @ 2021-12-01 16:27 ` William Kucharski 2021-12-01 16:44 ` Steven Rostedt 0 siblings, 1 reply; 4+ messages in thread From: William Kucharski @ 2021-12-01 16:27 UTC (permalink / raw) To: Steven Rostedt; +Cc: Ingo Molnar, Tejun Heo, Greg Kroah-Hartman, LKML I had pondered that as a consequence for two of the three uses but wasn't sure if it was worth reordering things; I can easily do so. What do you suggest for cgroup_migrate as that will have a hole either way as it's: TP_STRUCT__entry( __field( int, dst_root ) __field( u64, dst_id ) __field( int, dst_level ) __field( int, pid ) __string( dst_path, path ) __string( comm, task->comm ) ), if I put dst_level above dst_id, the int for pid field will leave a hole anyway because the string pointer for dst_path will want to be 64-bit aligned. Thanks in advance. > On Dec 1, 2021, at 9:14 AM, Steven Rostedt <rostedt@goodmis.org> wrote: > > On Wed, 1 Dec 2021 09:07:46 -0700 > William Kucharski <william.kucharski@oracle.com> wrote: > >> Various trace event fields that store cgroup IDs were declared as >> ints, but cgroup_id(() returns a u64 and the structures and associated >> TP_printk() calls were not updated to reflect this. >> >> Fixes: 743210386c03 ("cgroup: use cgrp->kn->id as the cgroup ID") >> Signed-off-by: William Kucharski <william.kucharski@oracle.com> >> --- >> V2: Do not remove spaces from macro arguments >> >> include/trace/events/cgroup.h | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) >> >> diff --git a/include/trace/events/cgroup.h b/include/trace/events/cgroup.h >> index 7f42a3de59e6..1b68c842ac46 100644 >> --- a/include/trace/events/cgroup.h >> +++ b/include/trace/events/cgroup.h >> @@ -59,7 +59,7 @@ DECLARE_EVENT_CLASS(cgroup, >> >> TP_STRUCT__entry( >> __field( int, root ) >> - __field( int, id ) >> + __field( u64, id ) >> __field( int, level ) > > It's best to move them around to prevent holes. This is not a packed > structure, and the above will create a structure on the ring buffer that > looks like: > > int root; > u64 id; > int level; > short path_offset; > short path_size; > > (the string() macro is a 4 byte word where the half is the offset of the > actual string and the other half is the size of the string). > > Having a 8 byte word between two 4 byte words, will add a 4 byte padding > after the first 4 byte word (before the 8 byte word), and waste space on > the ring buffer. > > Better to move it around to be: > > TP_STRUCT__entry( > __field( int, root ) > __field( int, level ) > __field( u64, id ) > > That way the two 4 byte words will be together followed directly by the 8 > byte word. > >> __string( path, path ) >> ), >> @@ -71,7 +71,7 @@ DECLARE_EVENT_CLASS(cgroup, >> __assign_str(path, path); >> ), >> >> - TP_printk("root=%d id=%d level=%d path=%s", >> + TP_printk("root=%d id=%llu level=%d path=%s", >> __entry->root, __entry->id, __entry->level, __get_str(path)) >> ); >> >> @@ -126,7 +126,7 @@ DECLARE_EVENT_CLASS(cgroup_migrate, >> >> TP_STRUCT__entry( >> __field( int, dst_root ) >> - __field( int, dst_id ) >> + __field( u64, dst_id ) > > Same here. Just move dst_level above dst_id. > >> __field( int, dst_level ) >> __field( int, pid ) >> __string( dst_path, path ) >> @@ -142,7 +142,7 @@ DECLARE_EVENT_CLASS(cgroup_migrate, >> __assign_str(comm, task->comm); >> ), >> >> - TP_printk("dst_root=%d dst_id=%d dst_level=%d dst_path=%s pid=%d comm=%s", >> + TP_printk("dst_root=%d dst_id=%llu dst_level=%d dst_path=%s pid=%d comm=%s", >> __entry->dst_root, __entry->dst_id, __entry->dst_level, >> __get_str(dst_path), __entry->pid, __get_str(comm)) >> ); >> @@ -171,7 +171,7 @@ DECLARE_EVENT_CLASS(cgroup_event, >> >> TP_STRUCT__entry( >> __field( int, root ) >> - __field( int, id ) >> + __field( u64, id ) > > And here. > >> __field( int, level ) >> __string( path, path ) >> __field( int, val ) >> @@ -185,7 +185,7 @@ DECLARE_EVENT_CLASS(cgroup_event, >> __entry->val = val; >> ), >> >> - TP_printk("root=%d id=%d level=%d path=%s val=%d", >> + TP_printk("root=%d id=%llu level=%d path=%s val=%d", >> __entry->root, __entry->id, __entry->level, __get_str(path), >> __entry->val) >> ); > > > > -- Steve ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH V2] cgroup: Trace event cgroup id fields should be u64 2021-12-01 16:27 ` William Kucharski @ 2021-12-01 16:44 ` Steven Rostedt 0 siblings, 0 replies; 4+ messages in thread From: Steven Rostedt @ 2021-12-01 16:44 UTC (permalink / raw) To: William Kucharski; +Cc: Ingo Molnar, Tejun Heo, Greg Kroah-Hartman, LKML On Wed, 1 Dec 2021 16:27:54 +0000 William Kucharski <william.kucharski@oracle.com> wrote: > I had pondered that as a consequence for two of the three uses but wasn't sure > if it was worth reordering things; I can easily do so. > > What do you suggest for cgroup_migrate as that will have a hole either way > as it's: > > TP_STRUCT__entry( > __field( int, dst_root ) > __field( u64, dst_id ) > __field( int, dst_level ) > __field( int, pid ) > __string( dst_path, path ) > __string( comm, task->comm ) > ), > > if I put dst_level above dst_id, the int for pid field will leave a hole > anyway because the string pointer for dst_path will want to be 64-bit > aligned. > > Thanks in advance. > It's not actually a string pointer, both string() macros are really 4 byte words. The above would be best compressed as: TP_STRUCT__entry( __field( int, dst_root ) __field( int, dst_level ) __field( u64, dst_id ) __field( int, pid ) __string( dst_path, path ) __string( comm, task->comm ) ), What that would turn into is: struct __entry { int dst_root; int dst_level; u64 dst_id; int pid; int dst_path_offset_size; int comm_offset_size; char strings[]; } Remember, this is used for storing data onto the ring buffer. I'll make up two strings to show an example. dst_path = "/path/to/dst_path"; comm = "bash"; The string(dst_path) and string(comm) would be where the data to find the strings are. The strings will be stored in the strings[] field of the structure, and the dst_path and comm fields will be used to find those strings. dst_root = 1; dst_level = 2; dst_id = 3; pid = 4; dst_path = (28 << 16) | 18; comm = ((28 + 18) << 16 | 5; strings = "/path/to/dst_path\0bash\0"; The dst_path holds how to find the string it represents. It starts at offset 28[*] (sizeof(int) * 5 + sizeof(u64)) And has size of 18 (strlen("/path/to/dst_path") + 1) The comm string starts at offset 46 (28 + size of dst_path), and has he size of 5 (strlen("bash") + 1). So consider "string()" macros to be only 4 bytes in size. And leave the "holes" at the end, especially if there's strings, because the will start the actual strings nicely aligned. -- Steve [*] It really starts at a later offset, because there's common fields added before the first field (common_type, common_flags, common_preempt_count, common_pid) that is also included in that offset count. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-12-01 16:45 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-01 16:07 [PATCH V2] cgroup: Trace event cgroup id fields should be u64 William Kucharski 2021-12-01 16:14 ` Steven Rostedt 2021-12-01 16:27 ` William Kucharski 2021-12-01 16:44 ` Steven Rostedt
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).