From:   William Kucharski <william.kucharski@oracle.com>
To:     Steven Rostedt <rostedt@goodmis.org>
CC:     Ingo Molnar <mingo@redhat.com>, Tejun Heo <tj@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] cgroup: Trace event cgroup id fields should be u64
Thread-Topic: [PATCH] cgroup: Trace event cgroup id fields should be u64
Thread-Index: AQHX5sFwaB6Hoy4YakmqLMRZzO8P2Kwdx44AgAAAZgA=
Date:   Wed, 1 Dec 2021 15:47:24 +0000
Message-ID: <087E34E0-A259-4BC2-82DF-8FD7665CF22C@oracle.com>
References: <20211201144043.557470-1-william.kucharski@oracle.com>
 <20211201104558.230196a7@gandalf.local.home>
In-Reply-To: <20211201104558.230196a7@gandalf.local.home>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?xMhzR45qBZ2KpJCylPica8o6TqFdvcicavEd/ErJ8bEj4Ih+KhIUgVuNpQlR?=
 =?us-ascii?Q?tN2g7FdHpz7TazRURo9l7GT3P+wLT32WKJmL7cXmZItmjSWXV1JhK9/45tzw?=
 =?us-ascii?Q?Uj6rxRBMllIpMkZW01ZlhgtrInXJlGWhI124BfemhPjR3CEyM42IOPqzJJCr?=
 =?us-ascii?Q?ifNiyWRxsv4PDlRxUUDsEI9j3ps0DHBZ4M6TZ2ct/VPsxqvNPkQRsw2A78I/?=
 =?us-ascii?Q?uW6T8MK/oIJ/FUZ+okA+IRomBt2GczRgsb/pZXsYvfvNgGEzqGkaobPD/Lgp?=
 =?us-ascii?Q?EQauRyRjWjzN2+myeY0XGiBZBWlhp/zyNtKrtgkct7EljxsQeRA2hq7AnxCL?=
 =?us-ascii?Q?QOLPR14VZZmquBAf7Sshnit964QP/U4YDcxN24sMZ8SjJnE9Z+5RBbt1r0l8?=
 =?us-ascii?Q?/KpyDtVCUPv8xkdHIEcD4DpRWT52v/D3/8bBQVwxUPLI090GXXewlYoeN0iK?=
 =?us-ascii?Q?uJ/OUkSnQbmdVZRKiYCe6qv2WrHLybQYVhnSTKzMqHZ2BPiVuWWYNR7PGaDX?=
 =?us-ascii?Q?h9RUImB0MeoHUtE1aUWuAId0yHtcT5vP9SMeTHMInvdahFtISQ5WRY4MBRPU?=
 =?us-ascii?Q?mhHRAvUXChOacpCmTFLnfcEAdo+Xbu7oYEx4WHJze/R1z2M0Oheq2Ay4umS9?=
 =?us-ascii?Q?H6gWgn5L+9o88DGr2aBnsrIuBS82mY6+WGyxfWke2cbtIpFsb2XVncVHvvKo?=
 =?us-ascii?Q?34oNe/StjP/+Q9PQBjIceL9yA4feILju7Y6yhO1lDJqqMiF0xjbkNAnJ+HCn?=
 =?us-ascii?Q?KBfAKyjoagwwsB7AlbYU7/nziSj37l1FNncIsEvIQtIQ0pRJ6iYhsSwK5QvB?=
 =?us-ascii?Q?usMpou1KknvJKTfTyrnPZ1VyaUvlttEq8qXWNro7kbYDWYqLi5B0KoCK+tTL?=
 =?us-ascii?Q?nVIgkOdmUCvViCRv/oPSS6/wfuYgEOTh98xFpL5c6NxcZEhAmK4deWlHfuLR?=
 =?us-ascii?Q?TojPgiuvGz8PSUWqKlJN5CgVMuqBHkICz6e1PFMVcn0Z6GWBwLLzIAU1FfWR?=
 =?us-ascii?Q?d+W8ffQB7bSc5OBa98ztRAgDu50aD5e7sGAdbjD989b406XxjvpiPxntSccK?=
 =?us-ascii?Q?JtSmmmnRbFEfBnL/PPwzxTdvdqfuJ7kDDLdxRYiDPndTSY/Z0jzNC3Vf5B0k?=
 =?us-ascii?Q?6lUcwbbtpW6eiulIDLThIZltD7SK1QmUL8YVzBr6sCud1/6xYIfHg9qxxYlA?=
 =?us-ascii?Q?O38Uh9+p3ryxsKoLWfy/kSYHOcdq5Pv81aWTd/fRleD7C1OPBDJu23rmRG6P?=
 =?us-ascii?Q?1oQ8GjvSAsz5VmH+vPdVciAdcxmqD/B1UbH2s9/2LjVTWpTr+pniopjI5VbH?=
 =?us-ascii?Q?yLgpcpte+2UdS4jhiDgmAfOtDsiKRPVqT4dIlCOPucHQybZoPRGFeiEyM4LP?=
 =?us-ascii?Q?JoU+GIbJtubxlu1eYQ5JfQoEnyAmCamxmosP4f800xwevn5WWZ/cuE3215hw?=
 =?us-ascii?Q?MFahRWWmrJ7kTQTSIPNZKWptYM/WV5/04nrWg0xQV1dTwZRRRyxxgr2YHfMy?=
 =?us-ascii?Q?JrvwIfSfAdYLdjgOC71utBGk6u8rtK4Ce/Xh+XX8b+O8LVWy/qCm41k8H3TY?=
 =?us-ascii?Q?Hkq8jRu0NE3lR30LFO+1dx0yhzkdCTMqPM/o163r3D6wDm4Tqkte0jtsUpud?=
 =?us-ascii?Q?ME3+VAxzkOw8DkQuFiQqZm/GxpRxYyTeexEP6U6dXVr7/l9uFlU/SzaTKP+L?=
 =?us-ascii?Q?1aFXGS577TtszFozeR85BxQfTAeDJexQFoCyMIDkQRunIEw3OO3JQeG123Tb?=
 =?us-ascii?Q?ilwWJuR2XH3kMLIL0Z0kNEouwBbUVfQ=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <9DFC66E19DA31C4480653EDFDBCF29C6@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN4PR10MB5559.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 579da10a-cadc-4092-c1cc-08d9b4e1defc
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Dec 2021 15:47:24.9860
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: CXHGzk9Xa89jgIMMacFwqWUntfYi+avzhFPm+GdNDh8Cw58qnGDflaQW64IHXsQhomHKSbG11bk5xCInUxV+CfKjvhVfhwPB5ezcGTgac14=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR10MB2685
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10185 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0 adultscore=0
 bulkscore=0 malwarescore=0 mlxlogscore=999 mlxscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112010087
X-Proofpoint-ORIG-GUID: p7WgAVovFdBa-AAq6ZP9g757f07LJ2nV
X-Proofpoint-GUID: p7WgAVovFdBa-AAq6ZP9g757f07LJ2nV
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

No problem, it makes my life easier for V2. :-)

Thanks!!

> On Dec 1, 2021, at 8:45 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>=20
> On Wed,  1 Dec 2021 07:40:43 -0700
> William Kucharski <william.kucharski@oracle.com> wrote:
>=20
>> Various trace event fields that store cgroup IDs were declared
>> as ints, but cgroup_id(() returns a u64 and the conversion was
>> not intended.
>>=20
>> Also remove extraneous spaces in fields that are no longer proper C styl=
e.
>=20
> Um, actually, please do not do this!
>=20
>>=20
>> 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(-)
>>=20
>> 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),
>>=20
>> 	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)
>=20
> The spaces are there for a reason. These are special macros and not C cod=
e.
> It's defining a structure, and the spacing is to imitate just that!
>=20
> struct __entry {
> 	int		root;
> 	u16		ss_mask;
> 	name		root->name;
> };
>=20
>=20
> See the difference. You just made it less readable than the original.
>=20
> So leave it alone.
>=20
> NACK on the style changes.
>=20
> -- Steve
>=20
>=20
>> 	),
>>=20
>> 	TP_fast_assign(
>> @@ -58,10 +58,10 @@ DECLARE_EVENT_CLASS(cgroup,
>> 	TP_ARGS(cgrp, path),
>>=20
>> 	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)
>> 	),
>>=20
>> 	TP_fast_assign(
>> @@ -71,7 +71,7 @@ DECLARE_EVENT_CLASS(cgroup,
>> 		__assign_str(path, path);
>> 	),
>>=20
>> -	TP_printk("root=3D%d id=3D%d level=3D%d path=3D%s",
>> +	TP_printk("root=3D%d id=3D%llu level=3D%d path=3D%s",
>> 		  __entry->root, __entry->id, __entry->level, __get_str(path))
>> );
>>=20
>> @@ -125,12 +125,12 @@ DECLARE_EVENT_CLASS(cgroup_migrate,
>> 	TP_ARGS(dst_cgrp, path, task, threadgroup),
>>=20
>> 	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)
>> 	),
>>=20
>> 	TP_fast_assign(
>> @@ -142,7 +142,7 @@ DECLARE_EVENT_CLASS(cgroup_migrate,
>> 		__assign_str(comm, task->comm);
>> 	),
>>=20
>> -	TP_printk("dst_root=3D%d dst_id=3D%d dst_level=3D%d dst_path=3D%s pid=
=3D%d comm=3D%s",
>> +	TP_printk("dst_root=3D%d dst_id=3D%llu dst_level=3D%d dst_path=3D%s pi=
d=3D%d comm=3D%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),
>>=20
>> 	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)
>> 	),
>>=20
>> 	TP_fast_assign(
>> @@ -185,7 +185,7 @@ DECLARE_EVENT_CLASS(cgroup_event,
>> 		__entry->val =3D val;
>> 	),
>>=20
>> -	TP_printk("root=3D%d id=3D%d level=3D%d path=3D%s val=3D%d",
>> +	TP_printk("root=3D%d id=3D%llu level=3D%d path=3D%s val=3D%d",
>> 		  __entry->root, __entry->id, __entry->level, __get_str(path),
>> 		  __entry->val)
>> );
>=20