Messages in this thread | | | From | William Kucharski <> | Subject | Re: [PATCH V2] cgroup: Trace event cgroup id fields should be u64 | Date | Wed, 1 Dec 2021 16:27:54 +0000 |
| |
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
| |