Received-SPF: pass (google.com: domain of linux-kernel+bounces-75638-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 21 Feb 2024 18:17:13 -0500
From: Steven Rostedt <rostedt@goodmis.org>
To: Anna-Maria Behnsen <anna-maria@linutronix.de>
Cc: linux-kernel@vger.kernel.org, Peter Zijlstra <peterz@infradead.org>,
 John Stultz <jstultz@google.com>, Thomas Gleixner <tglx@linutronix.de>,
 Eric Dumazet <edumazet@google.com>, "Rafael J . Wysocki"
 <rafael.j.wysocki@intel.com>, Arjan van de Ven <arjan@infradead.org>, "Paul
 E . McKenney" <paulmck@kernel.org>, Frederic Weisbecker
 <frederic@kernel.org>, Rik van Riel <riel@surriel.com>, Sebastian Siewior
 <bigeasy@linutronix.de>, Giovanni Gherdovich <ggherdovich@suse.cz>, Lukasz
 Luba <lukasz.luba@arm.com>, "Gautham R . Shenoy" <gautham.shenoy@amd.com>,
 Srinivas Pandruvada <srinivas.pandruvada@intel.com>, K Prateek Nayak
 <kprateek.nayak@amd.com>, Christian Loehle <christian.loehle@arm.com>
Subject: Re: [PATCH v11 19/20] timer_migration: Add tracepoints
Message-ID: <20240221181713.12f530cb@gandalf.local.home>
In-Reply-To: <20240221090548.36600-20-anna-maria@linutronix.de>
References: <20240221090548.36600-1-anna-maria@linutronix.de>
	<20240221090548.36600-20-anna-maria@linutronix.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 21 Feb 2024 10:05:47 +0100
Anna-Maria Behnsen <anna-maria@linutronix.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)
> +);
> +