linux-kernel - Re: [PATCH v11 19/20] timer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 21 Feb 2024 18:17:13 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Anna-Maria Behnsen <anna-maria@...utronix.de>
Cc: linux-kernel@...r.kernel.org, Peter Zijlstra <peterz@...radead.org>,
 John Stultz <jstultz@...gle.com>, Thomas Gleixner <tglx@...utronix.de>,
 Eric Dumazet <edumazet@...gle.com>, "Rafael J . Wysocki"
 <rafael.j.wysocki@...el.com>, Arjan van de Ven <arjan@...radead.org>, "Paul
 E . McKenney" <paulmck@...nel.org>, Frederic Weisbecker
 <frederic@...nel.org>, Rik van Riel <riel@...riel.com>, Sebastian Siewior
 <bigeasy@...utronix.de>, Giovanni Gherdovich <ggherdovich@...e.cz>, Lukasz
 Luba <lukasz.luba@....com>, "Gautham R . Shenoy" <gautham.shenoy@....com>,
 Srinivas Pandruvada <srinivas.pandruvada@...el.com>, K Prateek Nayak
 <kprateek.nayak@....com>, Christian Loehle <christian.loehle@....com>
Subject: Re: [PATCH v11 19/20] timer_migration: Add tracepoints

On Wed, 21 Feb 2024 10:05:47 +0100
Anna-Maria Behnsen <anna-maria@...utronix.de> wrote:

> diff --git a/include/trace/events/timer_migration.h b/include/trace/events/timer_migration.h
> new file mode 100644
> index 000000000000..3f6e9502c41e
> --- /dev/null
> +++ b/include/trace/events/timer_migration.h
> @@ -0,0 +1,297 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +

> +DECLARE_EVENT_CLASS(tmigr_group_and_cpu,
> +
> +	TP_PROTO(struct tmigr_group *group, union tmigr_state state, u32 childmask),
> +
> +	TP_ARGS(group, state, childmask),
> +
> +	TP_STRUCT__entry(
> +		__field( void *,	group		)
> +		__field( void *,	parent		)
> +		__field( unsigned int,	lvl		)
> +		__field( unsigned int,	numa_node	)
> +		__field( u8,		active		)
> +		__field( u8,		migrator	)
> +		__field( u32,		childmask	)

Could you move the two u8 after the u32? Although it probably doesn't
matter for the actual size, I prefer to keep holes from inside the
structure. The above will create a two byte hole between the two u8 and the
u32.

> +	),
> +
> +	TP_fast_assign(
> +		__entry->group		= group;
> +		__entry->parent		= group->parent;
> +		__entry->lvl		= group->level;
> +		__entry->numa_node	= group->numa_node;
> +		__entry->active		= state.active;
> +		__entry->migrator	= state.migrator;
> +		__entry->childmask	= childmask;
> +	),
> +
> +	TP_printk("group=%p lvl=%d numa=%d active=%0x migrator=%0x "
> +		  "parent=%p childmask=%0x",
> +		  __entry->group, __entry->lvl, __entry->numa_node,
> +		  __entry->active, __entry->migrator,
> +		  __entry->parent, __entry->childmask)
> +);
> +
> +DEFINE_EVENT(tmigr_group_and_cpu, tmigr_group_set_cpu_inactive,
> +
> +	TP_PROTO(struct tmigr_group *group, union tmigr_state state, u32 childmask),
> +
> +	TP_ARGS(group, state, childmask)
> +);
> +
> +DEFINE_EVENT(tmigr_group_and_cpu, tmigr_group_set_cpu_active,
> +
> +	TP_PROTO(struct tmigr_group *group, union tmigr_state state, u32 childmask),
> +
> +	TP_ARGS(group, state, childmask)
> +);
> +
> +/* CPU events*/
> +DECLARE_EVENT_CLASS(tmigr_cpugroup,
> +
> +	TP_PROTO(struct tmigr_cpu *tmc),
> +
> +	TP_ARGS(tmc),
> +
> +	TP_STRUCT__entry(
> +		__field( void *,	parent)
> +		__field( unsigned int,	cpu)
> +		__field( u64,		wakeup)

Please put the u64 first. That way on 32 bit machines, parent and cpu will
fit together and on 64 bit machines, there will not be a hole between cpu
and wakeup.

> +	),
> +
> +	TP_fast_assign(
> +		__entry->cpu		= tmc->cpuevt.cpu;
> +		__entry->parent		= tmc->tmgroup;
> +		__entry->wakeup		= tmc->wakeup;
> +	),
> +
> +	TP_printk("cpu=%d parent=%p wakeup=%llu", __entry->cpu, __entry->parent, __entry->wakeup)
> +);
> +
> +DEFINE_EVENT(tmigr_cpugroup, tmigr_cpu_new_timer,
> +
> +	TP_PROTO(struct tmigr_cpu *tmc),
> +
> +	TP_ARGS(tmc)
> +);
> +
> +DEFINE_EVENT(tmigr_cpugroup, tmigr_cpu_active,
> +
> +	TP_PROTO(struct tmigr_cpu *tmc),
> +
> +	TP_ARGS(tmc)
> +);
> +
> +DEFINE_EVENT(tmigr_cpugroup, tmigr_cpu_online,
> +
> +	TP_PROTO(struct tmigr_cpu *tmc),
> +
> +	TP_ARGS(tmc)
> +);
> +
> +DEFINE_EVENT(tmigr_cpugroup, tmigr_cpu_offline,
> +
> +	TP_PROTO(struct tmigr_cpu *tmc),
> +
> +	TP_ARGS(tmc)
> +);
> +
> +DEFINE_EVENT(tmigr_cpugroup, tmigr_handle_remote_cpu,
> +
> +	TP_PROTO(struct tmigr_cpu *tmc),
> +
> +	TP_ARGS(tmc)
> +);
> +
> +DECLARE_EVENT_CLASS(tmigr_idle,
> +
> +	TP_PROTO(struct tmigr_cpu *tmc, u64 nextevt),
> +
> +	TP_ARGS(tmc, nextevt),
> +
> +	TP_STRUCT__entry(
> +		__field( void *,	parent)
> +		__field( unsigned int,	cpu)
> +		__field( u64,		nextevt)
> +		__field( u64,		wakeup)

Please put parent and cpu after wakeup so that there will not be a hole
around cpu.

> +	),
> +
> +	TP_fast_assign(
> +		__entry->cpu		= tmc->cpuevt.cpu;
> +		__entry->parent		= tmc->tmgroup;
> +		__entry->nextevt	= nextevt;
> +		__entry->wakeup		= tmc->wakeup;
> +	),
> +
> +	TP_printk("cpu=%d parent=%p nextevt=%llu wakeup=%llu",
> +		  __entry->cpu, __entry->parent, __entry->nextevt, __entry->wakeup)
> +);
> +
> +DEFINE_EVENT(tmigr_idle, tmigr_cpu_idle,
> +
> +	TP_PROTO(struct tmigr_cpu *tmc, u64 nextevt),
> +
> +	TP_ARGS(tmc, nextevt)
> +);
> +
> +DEFINE_EVENT(tmigr_idle, tmigr_cpu_new_timer_idle,
> +
> +	TP_PROTO(struct tmigr_cpu *tmc, u64 nextevt),
> +
> +	TP_ARGS(tmc, nextevt)
> +);
> +
> +TRACE_EVENT(tmigr_update_events,
> +
> +	TP_PROTO(struct tmigr_group *child, struct tmigr_group *group,
> +		 union tmigr_state childstate,	union tmigr_state groupstate,
> +		 u64 nextevt),
> +
> +	TP_ARGS(child, group, childstate, groupstate, nextevt),
> +
> +	TP_STRUCT__entry(
> +		__field( void *,	child			)
> +		__field( void *,	group			)
> +		__field( u64,		nextevt			)
> +		__field( u64,		group_next_expiry	)
> +		__field( unsigned int,	group_lvl		)
> +		__field( u8,		child_active		)
> +		__field( u8,		group_active		)
> +		__field( unsigned int,	child_evtcpu		)
> +		__field( u64,		child_evt_expiry	)

Please put child_evt_expiry after group_next_expiry. Have group_lvl next to
child_evtcpu and the two u8 fields at the end.

Thanks!

-- Steve


> +	),
> +
> +	TP_fast_assign(
> +		__entry->child			= child;
> +		__entry->group			= group;
> +		__entry->nextevt		= nextevt;
> +		__entry->group_next_expiry	= group->next_expiry;
> +		__entry->group_lvl		= group->level;
> +		__entry->child_active		= childstate.active;
> +		__entry->group_active		= groupstate.active;
> +		__entry->child_evtcpu		= child ? child->groupevt.cpu : 0;
> +		__entry->child_evt_expiry	= child ? child->groupevt.nextevt.expires : 0;
> +	),
> +
> +	TP_printk("child=%p group=%p group_lvl=%d child_active=%0x group_active=%0x "
> +		  "nextevt=%llu next_expiry=%llu child_evt_expiry=%llu child_evtcpu=%d",
> +		  __entry->child, __entry->group, __entry->group_lvl, __entry->child_active,
> +		  __entry->group_active,
> +		  __entry->nextevt, __entry->group_next_expiry, __entry->child_evt_expiry,
> +		  __entry->child_evtcpu)
> +);
> +
> +TRACE_EVENT(tmigr_handle_remote,
> +
> +	TP_PROTO(struct tmigr_group *group),
> +
> +	TP_ARGS(group),
> +
> +	TP_STRUCT__entry(
> +		__field( void * ,	group	)
> +		__field( unsigned int ,	lvl	)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->group		= group;
> +		__entry->lvl		= group->level;
> +	),
> +
> +	TP_printk("group=%p lvl=%d",
> +		   __entry->group, __entry->lvl)
> +);
> +