Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Mar 2022 22:41:24 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Joel Fernandes <joel@joelfernandes.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Tom Zanussi <zanussi@kernel.org>
Subject: Re: [PATCH 2/2] tracing: Add sample code for custom trace events
Message-ID: <20220302224124.433ba754@rorschach.local.home>
In-Reply-To: <CAEXW_YQ4id4-GOgYzP_U2BLx8VVKy=RRLQgmV-npHETG5SqL2g@mail.gmail.com>
References: <20220302032414.503960863@goodmis.org>
        <20220302032820.877781830@goodmis.org>
        <CAEXW_YQ4id4-GOgYzP_U2BLx8VVKy=RRLQgmV-npHETG5SqL2g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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