Re: [PATCH 2/2] tracing: Add sample code for custom trace events

[Steven Rostedt <rostedt@goodmis.org>]

On Wed, 2 Mar 2022 22:22:30 -0500
Joel Fernandes <joel@joelfernandes.org> wrote:

> > +#include <linux/trace_events.h>
> > +#include <linux/version.h>
> > +#include <linux/module.h>
> > +#include <linux/sched.h>
> > +#include <trace/events/sched.h>
> > +
> > +#define THIS_SYSTEM "custom_sched"
> > +
> > +#define SCHED_PRINT_FMT                                                        \
> > +       C("prev_prio=%d next_pid=%d next_prio=%d", REC->prev_prio, REC->next_pid, \  
> 
> Probably prev_pid should be included so we know what the previous task was?
> 
> Or are you expecting that a prior sched_switch would have that
> information? If so, then prev_prio is also not needed as the previous
> sched_switch's next_prio would have the prio. That would save even
> more space too..

No, I left it out because it's already recorded in the event:

kworker/u16:2-19213   [005] d..2. 24689.792052: sched_switch: prev_comm=kworker/u16:2 prev_pid=19213 prev_prio=120 prev_state=I ==> next_comm=swapper/5 next_pid=0 next_prio=120

The above is the normal sched_switch event. The prev pid is 19213 which
is equal to common_pid (the first 19213 in that string). As prios can
change, I would not want to drop that anyway.

> 
> > +         REC->next_prio)
> > +
> > +#define SCHED_WAKING_FMT                               \
> > +       C("pid=%d prio=%d", REC->pid, REC->prio)
> > +  
> 
> I think including the target_cpu of a wake up is also really important
> to show where the task is going to be awakened, and maybe we can drop
> prio since a subsequent sched_switch will have the priority in
> next_prio.

True, we can do that.

> 
> [..]
> > +static void __exit trace_sched_exit(void)
> > +{
> > +       trace_set_clr_event(THIS_SYSTEM, "sched_switch", 0);
> > +       trace_set_clr_event(THIS_SYSTEM, "sched_waking", 0);
> > +
> > +       trace_remove_event_call(&sched_switch_call);
> > +       trace_remove_event_call(&sched_waking_call);
> > +}
> > +
> > +module_init(trace_sched_init);
> > +module_exit(trace_sched_exit);
> > +
> > +MODULE_AUTHOR("Steven Rostedt");
> > +MODULE_DESCRIPTION("Custom scheduling events");
> > +MODULE_LICENSE("GPL");
> > +  
> 
> Remove extra lines from the end of the file?
>

Ah, yeah.

Thanks for reviewing.

-- Steve