| Message ID | 20240604212854.725383717@goodmis.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ftrace: Clean up and comment code | expand |
On Tue, Jun 04, 2024 at 05:28:19PM -0400, Steven Rostedt wrote: > From: "Steven Rostedt (Google)" <rostedt@goodmis.org> > > The function __ftrace_hash_rec_update() parameter "filter_hash" is only > used for true or false (boolean), but is of type int. It already has an > "inc" parameter that is boolean. This is confusing, make "filter_hash" > boolean as well. > > While at it, add some documentation to that function especially since it > holds the guts of the filtering logic of ftrace. > > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org> > --- > kernel/trace/ftrace.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index 9dcdefe9d1aa..93c7c5fd4249 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -1701,8 +1701,20 @@ static bool skip_record(struct dyn_ftrace *rec) > !(rec->flags & FTRACE_FL_ENABLED); > } > > +/* > + * This is the main engine to the ftrace updates. > + * > + * It will iterate through all the available ftrace functions > + * (the ones that ftrace can have callbacks to) and set the flags > + * to the associated dyn_ftrace records. I beleive s/to/in/ here, to make this one of: set the flags in the associated dyn_ftrace records. ... rather than: set the flags to the associated dyn_ftrace records. > + * > + * @filter_hash: True if for the filter hash is udpated, false for the > + * notrace hash Typo: s/udpated/updated/ ... though I couldn't parse this regardless; maybe: @filter_hash: true to update the filter hash, false to update the notrace hash Mark. > + * @inc: True to add this hash, false to remove it (increment the > + * recorder counters or decrement them). > + */ > static bool __ftrace_hash_rec_update(struct ftrace_ops *ops, > - int filter_hash, > + bool filter_hash, > bool inc) > { > struct ftrace_hash *hash; > -- > 2.43.0 > >
On Wed, 5 Jun 2024 11:15:38 +0100 Mark Rutland <mark.rutland@arm.com> wrote: > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index 9dcdefe9d1aa..93c7c5fd4249 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -1701,8 +1701,20 @@ static bool skip_record(struct dyn_ftrace *rec) > > !(rec->flags & FTRACE_FL_ENABLED); > > } > > > > +/* > > + * This is the main engine to the ftrace updates. > > + * > > + * It will iterate through all the available ftrace functions > > + * (the ones that ftrace can have callbacks to) and set the flags > > + * to the associated dyn_ftrace records. > > I beleive s/to/in/ here, to make this one of: > > set the flags in the associated dyn_ftrace records. > > ... rather than: > > set the flags to the associated dyn_ftrace records. Thanks. It's good to get a "native English speaker" response ;-) > > > + * > > + * @filter_hash: True if for the filter hash is udpated, false for the > > + * notrace hash > > Typo: s/udpated/updated/ > > ... though I couldn't parse this regardless; maybe: > > @filter_hash: true to update the filter hash, false to update > the notrace hash Sure. -- Steve > > Mark. > > > + * @inc: True to add this hash, false to remove it (increment the > > + * recorder counters or decrement them). > > + */ > > static bool __ftrace_hash_rec_update(struct ftrace_ops *ops, > > - int filter_hash, > > + bool filter_hash, > > bool inc) > > { > > struct ftrace_hash *hash; > > -- > > 2.43.0 > > > >
On Wed, 5 Jun 2024 10:18:32 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > > > + * > > > + * @filter_hash: True if for the filter hash is udpated, false for the > > > + * notrace hash > > > > Typo: s/udpated/updated/ > > > > ... though I couldn't parse this regardless; maybe: > > > > @filter_hash: true to update the filter hash, false to update > > the notrace hash > > Sure. Actually, they are both wrong. Because I realized your's was not correct, I started describing it in more detail and realized it does basically the same thing (but differently) if filter_hash is set or not. I think it can be removed completely! I just tried it. I forced "filter_hash" to always be true, and all the tests worked just fine. This function is only to update the dyn_ftrace records when an ops is added or removed. It doesn't matter which filter is being used, as they are both necessary for the update. This will make that code a bit cleaner and simpler. Let me go and fix that first, and then add the documentation on top! This is what happens when you document your code :-p This code has been rewritten a few times. When it was first written, which filter was being changed may have been important. But now it is not. -- Steve
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 9dcdefe9d1aa..93c7c5fd4249 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -1701,8 +1701,20 @@ static bool skip_record(struct dyn_ftrace *rec) !(rec->flags & FTRACE_FL_ENABLED); } +/* + * This is the main engine to the ftrace updates. + * + * It will iterate through all the available ftrace functions + * (the ones that ftrace can have callbacks to) and set the flags + * to the associated dyn_ftrace records. + * + * @filter_hash: True if for the filter hash is udpated, false for the + * notrace hash + * @inc: True to add this hash, false to remove it (increment the + * recorder counters or decrement them). + */ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops, - int filter_hash, + bool filter_hash, bool inc) { struct ftrace_hash *hash;