* [PATCH] cgroup: Trace event cgroup id fields should be u64
@ 2021-12-01 14:40 William Kucharski
2021-12-01 14:54 ` Greg Kroah-Hartman
2021-12-01 15:45 ` Steven Rostedt
0 siblings, 2 replies; 6+ messages in thread
From: William Kucharski @ 2021-12-01 14:40 UTC (permalink / raw)
To: Steven Rostedt, Ingo Molnar, Tejun Heo, Greg Kroah-Hartman, linux-kernel
Cc: william.kucharski
Various trace event fields that store cgroup IDs were declared
as ints, but cgroup_id(() returns a u64 and the conversion was
not intended.
Also remove extraneous spaces in fields that are no longer proper C style.
Fixes: 743210386c03 ("cgroup: use cgrp->kn->id as the cgroup ID")
Signed-off-by: William Kucharski <william.kucharski@oracle.com>
---
include/trace/events/cgroup.h | 42 +++++++++++++++++------------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/include/trace/events/cgroup.h b/include/trace/events/cgroup.h
index 7f42a3de59e6..a85b01aa1fff 100644
--- a/include/trace/events/cgroup.h
+++ b/include/trace/events/cgroup.h
@@ -15,9 +15,9 @@ DECLARE_EVENT_CLASS(cgroup_root,
TP_ARGS(root),
TP_STRUCT__entry(
- __field( int, root )
- __field( u16, ss_mask )
- __string( name, root->name )
+ __field(int, root)
+ __field(u16, ss_mask)
+ __string(name, root->name)
),
TP_fast_assign(
@@ -58,10 +58,10 @@ DECLARE_EVENT_CLASS(cgroup,
TP_ARGS(cgrp, path),
TP_STRUCT__entry(
- __field( int, root )
- __field( int, id )
- __field( int, level )
- __string( path, path )
+ __field(int, root)
+ __field(u64, id)
+ __field(int, level)
+ __string(path, path)
),
TP_fast_assign(
@@ -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))
);
@@ -125,12 +125,12 @@ DECLARE_EVENT_CLASS(cgroup_migrate,
TP_ARGS(dst_cgrp, path, task, threadgroup),
TP_STRUCT__entry(
- __field( int, dst_root )
- __field( int, dst_id )
- __field( int, dst_level )
- __field( int, pid )
- __string( dst_path, path )
- __string( comm, task->comm )
+ __field(int, dst_root)
+ __field(u64, dst_id)
+ __field(int, dst_level)
+ __field(int, pid)
+ __string(dst_path, path)
+ __string(comm, task->comm)
),
TP_fast_assign(
@@ -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))
);
@@ -170,11 +170,11 @@ DECLARE_EVENT_CLASS(cgroup_event,
TP_ARGS(cgrp, path, val),
TP_STRUCT__entry(
- __field( int, root )
- __field( int, id )
- __field( int, level )
- __string( path, path )
- __field( int, val )
+ __field(int, root)
+ __field(u64, id)
+ __field(int, level)
+ __string(path, path)
+ __field(int, val)
),
TP_fast_assign(
@@ -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] 6+ messages in thread* Re: [PATCH] cgroup: Trace event cgroup id fields should be u64
2021-12-01 14:40 [PATCH] cgroup: Trace event cgroup id fields should be u64 William Kucharski
@ 2021-12-01 14:54 ` Greg Kroah-Hartman
2021-12-01 15:16 ` William Kucharski
2021-12-01 15:45 ` Steven Rostedt
1 sibling, 1 reply; 6+ messages in thread
From: Greg Kroah-Hartman @ 2021-12-01 14:54 UTC (permalink / raw)
To: William Kucharski; +Cc: Steven Rostedt, Ingo Molnar, Tejun Heo, linux-kernel
On Wed, Dec 01, 2021 at 07:40:43AM -0700, William Kucharski wrote:
> Various trace event fields that store cgroup IDs were declared
> as ints, but cgroup_id(() returns a u64 and the conversion was
> not intended.
>
> Also remove extraneous spaces in fields that are no longer proper C style.
Shouldn't this be 2 different patches? When writing "also" that's a
huge hint that the patch should be split up.
So one for the bugfix, and one for the coding style change?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cgroup: Trace event cgroup id fields should be u64
2021-12-01 14:54 ` Greg Kroah-Hartman
@ 2021-12-01 15:16 ` William Kucharski
0 siblings, 0 replies; 6+ messages in thread
From: William Kucharski @ 2021-12-01 15:16 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Steven Rostedt, Ingo Molnar, Tejun Heo, linux-kernel
I'll break it up into two patches if you prefer, one that makes the stylistic change and then
another that makes the u64 change.
> On Dec 1, 2021, at 7:54 AM, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Dec 01, 2021 at 07:40:43AM -0700, William Kucharski wrote:
>> Various trace event fields that store cgroup IDs were declared
>> as ints, but cgroup_id(() returns a u64 and the conversion was
>> not intended.
>>
>> Also remove extraneous spaces in fields that are no longer proper C style.
>
> Shouldn't this be 2 different patches? When writing "also" that's a
> huge hint that the patch should be split up.
>
> So one for the bugfix, and one for the coding style change?
>
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cgroup: Trace event cgroup id fields should be u64
2021-12-01 14:40 [PATCH] cgroup: Trace event cgroup id fields should be u64 William Kucharski
2021-12-01 14:54 ` Greg Kroah-Hartman
@ 2021-12-01 15:45 ` Steven Rostedt
2021-12-01 15:47 ` William Kucharski
2021-12-01 15:59 ` William Kucharski
1 sibling, 2 replies; 6+ messages in thread
From: Steven Rostedt @ 2021-12-01 15:45 UTC (permalink / raw)
To: William Kucharski
Cc: Ingo Molnar, Tejun Heo, Greg Kroah-Hartman, linux-kernel
On Wed, 1 Dec 2021 07:40:43 -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 conversion was
> not intended.
>
> Also remove extraneous spaces in fields that are no longer proper C style.
Um, actually, please do not do this!
>
> Fixes: 743210386c03 ("cgroup: use cgrp->kn->id as the cgroup ID")
> Signed-off-by: William Kucharski <william.kucharski@oracle.com>
> ---
> include/trace/events/cgroup.h | 42 +++++++++++++++++------------------
> 1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/include/trace/events/cgroup.h b/include/trace/events/cgroup.h
> index 7f42a3de59e6..a85b01aa1fff 100644
> --- a/include/trace/events/cgroup.h
> +++ b/include/trace/events/cgroup.h
> @@ -15,9 +15,9 @@ DECLARE_EVENT_CLASS(cgroup_root,
> TP_ARGS(root),
>
> TP_STRUCT__entry(
> - __field( int, root )
> - __field( u16, ss_mask )
> - __string( name, root->name )
> + __field(int, root)
> + __field(u16, ss_mask)
> + __string(name, root->name)
The spaces are there for a reason. These are special macros and not C code.
It's defining a structure, and the spacing is to imitate just that!
struct __entry {
int root;
u16 ss_mask;
name root->name;
};
See the difference. You just made it less readable than the original.
So leave it alone.
NACK on the style changes.
-- Steve
> ),
>
> TP_fast_assign(
> @@ -58,10 +58,10 @@ DECLARE_EVENT_CLASS(cgroup,
> TP_ARGS(cgrp, path),
>
> TP_STRUCT__entry(
> - __field( int, root )
> - __field( int, id )
> - __field( int, level )
> - __string( path, path )
> + __field(int, root)
> + __field(u64, id)
> + __field(int, level)
> + __string(path, path)
> ),
>
> TP_fast_assign(
> @@ -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))
> );
>
> @@ -125,12 +125,12 @@ DECLARE_EVENT_CLASS(cgroup_migrate,
> TP_ARGS(dst_cgrp, path, task, threadgroup),
>
> TP_STRUCT__entry(
> - __field( int, dst_root )
> - __field( int, dst_id )
> - __field( int, dst_level )
> - __field( int, pid )
> - __string( dst_path, path )
> - __string( comm, task->comm )
> + __field(int, dst_root)
> + __field(u64, dst_id)
> + __field(int, dst_level)
> + __field(int, pid)
> + __string(dst_path, path)
> + __string(comm, task->comm)
> ),
>
> TP_fast_assign(
> @@ -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))
> );
> @@ -170,11 +170,11 @@ DECLARE_EVENT_CLASS(cgroup_event,
> TP_ARGS(cgrp, path, val),
>
> TP_STRUCT__entry(
> - __field( int, root )
> - __field( int, id )
> - __field( int, level )
> - __string( path, path )
> - __field( int, val )
> + __field(int, root)
> + __field(u64, id)
> + __field(int, level)
> + __string(path, path)
> + __field(int, val)
> ),
>
> TP_fast_assign(
> @@ -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)
> );
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cgroup: Trace event cgroup id fields should be u64
2021-12-01 15:45 ` Steven Rostedt
@ 2021-12-01 15:47 ` William Kucharski
2021-12-01 15:59 ` William Kucharski
1 sibling, 0 replies; 6+ messages in thread
From: William Kucharski @ 2021-12-01 15:47 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Ingo Molnar, Tejun Heo, Greg Kroah-Hartman, linux-kernel
No problem, it makes my life easier for V2. :-)
Thanks!!
> On Dec 1, 2021, at 8:45 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 1 Dec 2021 07:40:43 -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 conversion was
>> not intended.
>>
>> Also remove extraneous spaces in fields that are no longer proper C style.
>
> Um, actually, please do not do this!
>
>>
>> Fixes: 743210386c03 ("cgroup: use cgrp->kn->id as the cgroup ID")
>> Signed-off-by: William Kucharski <william.kucharski@oracle.com>
>> ---
>> include/trace/events/cgroup.h | 42 +++++++++++++++++------------------
>> 1 file changed, 21 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/trace/events/cgroup.h b/include/trace/events/cgroup.h
>> index 7f42a3de59e6..a85b01aa1fff 100644
>> --- a/include/trace/events/cgroup.h
>> +++ b/include/trace/events/cgroup.h
>> @@ -15,9 +15,9 @@ DECLARE_EVENT_CLASS(cgroup_root,
>> TP_ARGS(root),
>>
>> TP_STRUCT__entry(
>> - __field( int, root )
>> - __field( u16, ss_mask )
>> - __string( name, root->name )
>> + __field(int, root)
>> + __field(u16, ss_mask)
>> + __string(name, root->name)
>
> The spaces are there for a reason. These are special macros and not C code.
> It's defining a structure, and the spacing is to imitate just that!
>
> struct __entry {
> int root;
> u16 ss_mask;
> name root->name;
> };
>
>
> See the difference. You just made it less readable than the original.
>
> So leave it alone.
>
> NACK on the style changes.
>
> -- Steve
>
>
>> ),
>>
>> TP_fast_assign(
>> @@ -58,10 +58,10 @@ DECLARE_EVENT_CLASS(cgroup,
>> TP_ARGS(cgrp, path),
>>
>> TP_STRUCT__entry(
>> - __field( int, root )
>> - __field( int, id )
>> - __field( int, level )
>> - __string( path, path )
>> + __field(int, root)
>> + __field(u64, id)
>> + __field(int, level)
>> + __string(path, path)
>> ),
>>
>> TP_fast_assign(
>> @@ -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))
>> );
>>
>> @@ -125,12 +125,12 @@ DECLARE_EVENT_CLASS(cgroup_migrate,
>> TP_ARGS(dst_cgrp, path, task, threadgroup),
>>
>> TP_STRUCT__entry(
>> - __field( int, dst_root )
>> - __field( int, dst_id )
>> - __field( int, dst_level )
>> - __field( int, pid )
>> - __string( dst_path, path )
>> - __string( comm, task->comm )
>> + __field(int, dst_root)
>> + __field(u64, dst_id)
>> + __field(int, dst_level)
>> + __field(int, pid)
>> + __string(dst_path, path)
>> + __string(comm, task->comm)
>> ),
>>
>> TP_fast_assign(
>> @@ -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))
>> );
>> @@ -170,11 +170,11 @@ DECLARE_EVENT_CLASS(cgroup_event,
>> TP_ARGS(cgrp, path, val),
>>
>> TP_STRUCT__entry(
>> - __field( int, root )
>> - __field( int, id )
>> - __field( int, level )
>> - __string( path, path )
>> - __field( int, val )
>> + __field(int, root)
>> + __field(u64, id)
>> + __field(int, level)
>> + __string(path, path)
>> + __field(int, val)
>> ),
>>
>> TP_fast_assign(
>> @@ -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)
>> );
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] cgroup: Trace event cgroup id fields should be u64
2021-12-01 15:45 ` Steven Rostedt
2021-12-01 15:47 ` William Kucharski
@ 2021-12-01 15:59 ` William Kucharski
1 sibling, 0 replies; 6+ messages in thread
From: William Kucharski @ 2021-12-01 15:59 UTC (permalink / raw)
To: Steven Rostedt; +Cc: Ingo Molnar, Tejun Heo, Greg Kroah-Hartman, LKML
Thanks, V2 will be out shortly.
> On Dec 1, 2021, at 8:45 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 1 Dec 2021 07:40:43 -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 conversion was
>> not intended.
>>
>> Also remove extraneous spaces in fields that are no longer proper C style.
>
> Um, actually, please do not do this!
>
>>
>> Fixes: 743210386c03 ("cgroup: use cgrp->kn->id as the cgroup ID")
>> Signed-off-by: William Kucharski <william.kucharski@oracle.com>
>> ---
>> include/trace/events/cgroup.h | 42 +++++++++++++++++------------------
>> 1 file changed, 21 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/trace/events/cgroup.h b/include/trace/events/cgroup.h
>> index 7f42a3de59e6..a85b01aa1fff 100644
>> --- a/include/trace/events/cgroup.h
>> +++ b/include/trace/events/cgroup.h
>> @@ -15,9 +15,9 @@ DECLARE_EVENT_CLASS(cgroup_root,
>> TP_ARGS(root),
>>
>> TP_STRUCT__entry(
>> - __field( int, root )
>> - __field( u16, ss_mask )
>> - __string( name, root->name )
>> + __field(int, root)
>> + __field(u16, ss_mask)
>> + __string(name, root->name)
>
> The spaces are there for a reason. These are special macros and not C code.
> It's defining a structure, and the spacing is to imitate just that!
>
> struct __entry {
> int root;
> u16 ss_mask;
> name root->name;
> };
>
>
> See the difference. You just made it less readable than the original.
>
> So leave it alone.
>
> NACK on the style changes.
>
> -- Steve
>
>
>> ),
>>
>> TP_fast_assign(
>> @@ -58,10 +58,10 @@ DECLARE_EVENT_CLASS(cgroup,
>> TP_ARGS(cgrp, path),
>>
>> TP_STRUCT__entry(
>> - __field( int, root )
>> - __field( int, id )
>> - __field( int, level )
>> - __string( path, path )
>> + __field(int, root)
>> + __field(u64, id)
>> + __field(int, level)
>> + __string(path, path)
>> ),
>>
>> TP_fast_assign(
>> @@ -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))
>> );
>>
>> @@ -125,12 +125,12 @@ DECLARE_EVENT_CLASS(cgroup_migrate,
>> TP_ARGS(dst_cgrp, path, task, threadgroup),
>>
>> TP_STRUCT__entry(
>> - __field( int, dst_root )
>> - __field( int, dst_id )
>> - __field( int, dst_level )
>> - __field( int, pid )
>> - __string( dst_path, path )
>> - __string( comm, task->comm )
>> + __field(int, dst_root)
>> + __field(u64, dst_id)
>> + __field(int, dst_level)
>> + __field(int, pid)
>> + __string(dst_path, path)
>> + __string(comm, task->comm)
>> ),
>>
>> TP_fast_assign(
>> @@ -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))
>> );
>> @@ -170,11 +170,11 @@ DECLARE_EVENT_CLASS(cgroup_event,
>> TP_ARGS(cgrp, path, val),
>>
>> TP_STRUCT__entry(
>> - __field( int, root )
>> - __field( int, id )
>> - __field( int, level )
>> - __string( path, path )
>> - __field( int, val )
>> + __field(int, root)
>> + __field(u64, id)
>> + __field(int, level)
>> + __string(path, path)
>> + __field(int, val)
>> ),
>>
>> TP_fast_assign(
>> @@ -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)
>> );
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-01 15:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-01 14:40 [PATCH] cgroup: Trace event cgroup id fields should be u64 William Kucharski
2021-12-01 14:54 ` Greg Kroah-Hartman
2021-12-01 15:16 ` William Kucharski
2021-12-01 15:45 ` Steven Rostedt
2021-12-01 15:47 ` William Kucharski
2021-12-01 15:59 ` William Kucharski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.