[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240221181713.12f530cb@gandalf.local.home>
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)
> +);
> +
Powered by blists - more mailing lists