[PATCH v2 0/5] ftrace: Clean up and comment code

[PATCH v2 0/5] ftrace: Clean up and comment code

[Steven Rostedt]

While working on the function_graph multiple users code, I realized
that I was struggling with how the ftrace code worked. Being the
author of such code meant that it wasn't very intuitive. Namely, the
function names were not descriptive enough, or at least, they needed
comments.

This series moves to solve some of that via changing a couple function
names and parameters and adding comments to many of them.

There's more to do, but this at least moves it in the right direction.

Changes since v1: https://lore.kernel.org/all/20240604212817.384103202@goodmis.org/

- While working on v1 and responding to a comment from Mark Rutland about
  the usage of "ftrace_hash" in the __ftrace_hash_rec_update() code,
  I realized that the function does pretty much the same thing if
  it is set or not set (but slightly differently). It turns out that
  it isn't needed and that parameter can be removed, making the code
  simpler.

- Fixed some wording and typos suggested by Mark Rutland.

Steven Rostedt (Google) (5):
      ftrace: Rename dup_hash() and comment it
      ftrace: Remove "ftrace_hash" parameter from __ftrace_hash_rec_update()
      ftrace: Add comments to ftrace_hash_rec_disable/enable()
      ftrace: Convert "inc" parameter to bool in ftrace_hash_rec_update_modify()
      ftrace: Add comments to ftrace_hash_move() and friends

----
 kernel/trace/ftrace.c | 161 +++++++++++++++++++++++++++++---------------------
 1 file changed, 94 insertions(+), 67 deletions(-)

[PATCH v2 1/5] ftrace: Rename dup_hash() and comment it

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

The name "dup_hash()" is a misnomer as it does not duplicate the hash that
is passed in, but instead moves its entities from that hash to a newly
allocated one. Rename it to "__move_hash()" (using starting underscores as
it is an internal function), and add some comments about what it does.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index da7e6abf48b4..9dcdefe9d1aa 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1391,7 +1391,11 @@ ftrace_hash_rec_enable_modify(struct ftrace_ops *ops, int filter_hash);
 static int ftrace_hash_ipmodify_update(struct ftrace_ops *ops,
 				       struct ftrace_hash *new_hash);
 
-static struct ftrace_hash *dup_hash(struct ftrace_hash *src, int size)
+/*
+ * Allocate a new hash and remove entries from @src and move them to the new hash.
+ * On success, the @src hash will be empty and should be freed.
+ */
+static struct ftrace_hash *__move_hash(struct ftrace_hash *src, int size)
 {
 	struct ftrace_func_entry *entry;
 	struct ftrace_hash *new_hash;
@@ -1438,7 +1442,7 @@ __ftrace_hash_move(struct ftrace_hash *src)
 	if (ftrace_hash_empty(src))
 		return EMPTY_HASH;
 
-	return dup_hash(src, size);
+	return __move_hash(src, size);
 }
 
 static int
-- 
2.43.0

Re: [PATCH v2 1/5] ftrace: Rename dup_hash() and comment it

[Masami Hiramatsu]

On Wed, 05 Jun 2024 14:03:35 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The name "dup_hash()" is a misnomer as it does not duplicate the hash that
> is passed in, but instead moves its entities from that hash to a newly
> allocated one. Rename it to "__move_hash()" (using starting underscores as
> it is an internal function), and add some comments about what it does.
> 

Looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  kernel/trace/ftrace.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index da7e6abf48b4..9dcdefe9d1aa 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1391,7 +1391,11 @@ ftrace_hash_rec_enable_modify(struct ftrace_ops *ops, int filter_hash);
>  static int ftrace_hash_ipmodify_update(struct ftrace_ops *ops,
>  				       struct ftrace_hash *new_hash);
>  
> -static struct ftrace_hash *dup_hash(struct ftrace_hash *src, int size)
> +/*
> + * Allocate a new hash and remove entries from @src and move them to the new hash.
> + * On success, the @src hash will be empty and should be freed.
> + */
> +static struct ftrace_hash *__move_hash(struct ftrace_hash *src, int size)
>  {
>  	struct ftrace_func_entry *entry;
>  	struct ftrace_hash *new_hash;
> @@ -1438,7 +1442,7 @@ __ftrace_hash_move(struct ftrace_hash *src)
>  	if (ftrace_hash_empty(src))
>  		return EMPTY_HASH;
>  
> -	return dup_hash(src, size);
> +	return __move_hash(src, size);
>  }
>  
>  static int
> -- 
> 2.43.0
> 
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

Re: [PATCH v2 1/5] ftrace: Rename dup_hash() and comment it

[Mark Rutland]

On Wed, Jun 05, 2024 at 02:03:35PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The name "dup_hash()" is a misnomer as it does not duplicate the hash that
> is passed in, but instead moves its entities from that hash to a newly
> allocated one. Rename it to "__move_hash()" (using starting underscores as
> it is an internal function), and add some comments about what it does.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  kernel/trace/ftrace.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index da7e6abf48b4..9dcdefe9d1aa 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1391,7 +1391,11 @@ ftrace_hash_rec_enable_modify(struct ftrace_ops *ops, int filter_hash);
>  static int ftrace_hash_ipmodify_update(struct ftrace_ops *ops,
>  				       struct ftrace_hash *new_hash);
>  
> -static struct ftrace_hash *dup_hash(struct ftrace_hash *src, int size)
> +/*
> + * Allocate a new hash and remove entries from @src and move them to the new hash.
> + * On success, the @src hash will be empty and should be freed.
> + */
> +static struct ftrace_hash *__move_hash(struct ftrace_hash *src, int size)
>  {
>  	struct ftrace_func_entry *entry;
>  	struct ftrace_hash *new_hash;
> @@ -1438,7 +1442,7 @@ __ftrace_hash_move(struct ftrace_hash *src)
>  	if (ftrace_hash_empty(src))
>  		return EMPTY_HASH;
>  
> -	return dup_hash(src, size);
> +	return __move_hash(src, size);
>  }
>  
>  static int
> -- 
> 2.43.0
> 
> 

[PATCH v2 2/5] ftrace: Remove "ftrace_hash" parameter from __ftrace_hash_rec_update()

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

While adding comments to the function __ftrace_hash_rec_update() and
trying to describe in detail what the parameter for "ftrace_hash" does, I
realized that it basically does exactly the same thing (but differently)
if it is set or not!

If it is set, the idea was the ops->filter_hash was being updated, and the
code should focus on the functions that are in the ops->filter_hash and
add them. But it still had to pay attention to the functions in the
ops->notrace_hash, to ignore them.

If it was cleared, it focused on the ops->notrace_hash, and would add
functions that were not in the ops->notrace_hash but would still keep
functions in the "ops->filter_hash". Basically doing the same thing.

In reality, the __ftrace_hash_rec_update() only needs to either remove the
functions associated to the give ops (if "inc" is set) or remove them (if
"inc" is cleared). It has to pay attention to both the filter_hash and
notrace_hash regardless.

Remove the "filter_hash" parameter from __filter_hash_rec_update() and
comment the function for what it really is doing.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 102 ++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 64 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 9dcdefe9d1aa..f8d8de7adade 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1383,10 +1383,8 @@ alloc_and_copy_ftrace_hash(int size_bits, struct ftrace_hash *hash)
 	return NULL;
 }
 
-static void
-ftrace_hash_rec_disable_modify(struct ftrace_ops *ops, int filter_hash);
-static void
-ftrace_hash_rec_enable_modify(struct ftrace_ops *ops, int filter_hash);
+static void ftrace_hash_rec_disable_modify(struct ftrace_ops *ops);
+static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops);
 
 static int ftrace_hash_ipmodify_update(struct ftrace_ops *ops,
 				       struct ftrace_hash *new_hash);
@@ -1474,11 +1472,11 @@ ftrace_hash_move(struct ftrace_ops *ops, int enable,
 	 * Remove the current set, update the hash and add
 	 * them back.
 	 */
-	ftrace_hash_rec_disable_modify(ops, enable);
+	ftrace_hash_rec_disable_modify(ops);
 
 	rcu_assign_pointer(*dst, new_hash);
 
-	ftrace_hash_rec_enable_modify(ops, enable);
+	ftrace_hash_rec_enable_modify(ops);
 
 	return 0;
 }
@@ -1701,12 +1699,21 @@ static bool skip_record(struct dyn_ftrace *rec)
 		!(rec->flags & FTRACE_FL_ENABLED);
 }
 
+/*
+ * This is the main engine to the ftrace updates to the dyn_ftrace records.
+ *
+ * It will iterate through all the available ftrace functions
+ * (the ones that ftrace can have callbacks to) and set the flags
+ * in the associated dyn_ftrace records.
+ *
+ * @inc: If true, the functions associated to @ops are added to
+ *       the dyn_ftrace records, otherwise they are removed.
+ */
 static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
-				     int filter_hash,
 				     bool inc)
 {
 	struct ftrace_hash *hash;
-	struct ftrace_hash *other_hash;
+	struct ftrace_hash *notrace_hash;
 	struct ftrace_page *pg;
 	struct dyn_ftrace *rec;
 	bool update = false;
@@ -1718,35 +1725,16 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
 		return false;
 
 	/*
-	 * In the filter_hash case:
 	 *   If the count is zero, we update all records.
 	 *   Otherwise we just update the items in the hash.
-	 *
-	 * In the notrace_hash case:
-	 *   We enable the update in the hash.
-	 *   As disabling notrace means enabling the tracing,
-	 *   and enabling notrace means disabling, the inc variable
-	 *   gets inversed.
 	 */
-	if (filter_hash) {
-		hash = ops->func_hash->filter_hash;
-		other_hash = ops->func_hash->notrace_hash;
-		if (ftrace_hash_empty(hash))
-			all = true;
-	} else {
-		inc = !inc;
-		hash = ops->func_hash->notrace_hash;
-		other_hash = ops->func_hash->filter_hash;
-		/*
-		 * If the notrace hash has no items,
-		 * then there's nothing to do.
-		 */
-		if (ftrace_hash_empty(hash))
-			return false;
-	}
+	hash = ops->func_hash->filter_hash;
+	notrace_hash = ops->func_hash->notrace_hash;
+	if (ftrace_hash_empty(hash))
+		all = true;
 
 	do_for_each_ftrace_rec(pg, rec) {
-		int in_other_hash = 0;
+		int in_notrace_hash = 0;
 		int in_hash = 0;
 		int match = 0;
 
@@ -1758,26 +1746,17 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
 			 * Only the filter_hash affects all records.
 			 * Update if the record is not in the notrace hash.
 			 */
-			if (!other_hash || !ftrace_lookup_ip(other_hash, rec->ip))
+			if (!notrace_hash || !ftrace_lookup_ip(notrace_hash, rec->ip))
 				match = 1;
 		} else {
 			in_hash = !!ftrace_lookup_ip(hash, rec->ip);
-			in_other_hash = !!ftrace_lookup_ip(other_hash, rec->ip);
+			in_notrace_hash = !!ftrace_lookup_ip(notrace_hash, rec->ip);
 
 			/*
-			 * If filter_hash is set, we want to match all functions
-			 * that are in the hash but not in the other hash.
-			 *
-			 * If filter_hash is not set, then we are decrementing.
-			 * That means we match anything that is in the hash
-			 * and also in the other_hash. That is, we need to turn
-			 * off functions in the other hash because they are disabled
-			 * by this hash.
+			 * We want to match all functions that are in the hash but
+			 * not in the other hash.
 			 */
-			if (filter_hash && in_hash && !in_other_hash)
-				match = 1;
-			else if (!filter_hash && in_hash &&
-				 (in_other_hash || ftrace_hash_empty(other_hash)))
+			if (in_hash && !in_notrace_hash)
 				match = 1;
 		}
 		if (!match)
@@ -1883,24 +1862,21 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
 	return update;
 }
 
-static bool ftrace_hash_rec_disable(struct ftrace_ops *ops,
-				    int filter_hash)
+static bool ftrace_hash_rec_disable(struct ftrace_ops *ops)
 {
-	return __ftrace_hash_rec_update(ops, filter_hash, 0);
+	return __ftrace_hash_rec_update(ops, 0);
 }
 
-static bool ftrace_hash_rec_enable(struct ftrace_ops *ops,
-				   int filter_hash)
+static bool ftrace_hash_rec_enable(struct ftrace_ops *ops)
 {
-	return __ftrace_hash_rec_update(ops, filter_hash, 1);
+	return __ftrace_hash_rec_update(ops, 1);
 }
 
-static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops,
-					  int filter_hash, int inc)
+static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops, int inc)
 {
 	struct ftrace_ops *op;
 
-	__ftrace_hash_rec_update(ops, filter_hash, inc);
+	__ftrace_hash_rec_update(ops, inc);
 
 	if (ops->func_hash != &global_ops.local_hash)
 		return;
@@ -1914,20 +1890,18 @@ static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops,
 		if (op == ops)
 			continue;
 		if (op->func_hash == &global_ops.local_hash)
-			__ftrace_hash_rec_update(op, filter_hash, inc);
+			__ftrace_hash_rec_update(op, inc);
 	} while_for_each_ftrace_op(op);
 }
 
-static void ftrace_hash_rec_disable_modify(struct ftrace_ops *ops,
-					   int filter_hash)
+static void ftrace_hash_rec_disable_modify(struct ftrace_ops *ops)
 {
-	ftrace_hash_rec_update_modify(ops, filter_hash, 0);
+	ftrace_hash_rec_update_modify(ops, 0);
 }
 
-static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,
-					  int filter_hash)
+static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops)
 {
-	ftrace_hash_rec_update_modify(ops, filter_hash, 1);
+	ftrace_hash_rec_update_modify(ops, 1);
 }
 
 /*
@@ -3050,7 +3024,7 @@ int ftrace_startup(struct ftrace_ops *ops, int command)
 		return ret;
 	}
 
-	if (ftrace_hash_rec_enable(ops, 1))
+	if (ftrace_hash_rec_enable(ops))
 		command |= FTRACE_UPDATE_CALLS;
 
 	ftrace_startup_enable(command);
@@ -3092,7 +3066,7 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command)
 	/* Disabling ipmodify never fails */
 	ftrace_hash_ipmodify_disable(ops);
 
-	if (ftrace_hash_rec_disable(ops, 1))
+	if (ftrace_hash_rec_disable(ops))
 		command |= FTRACE_UPDATE_CALLS;
 
 	ops->flags &= ~FTRACE_OPS_FL_ENABLED;
-- 
2.43.0

Re: [PATCH v2 2/5] ftrace: Remove "ftrace_hash" parameter from __ftrace_hash_rec_update()

[Mark Rutland]

On Wed, Jun 05, 2024 at 02:03:36PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> While adding comments to the function __ftrace_hash_rec_update() and
> trying to describe in detail what the parameter for "ftrace_hash" does, I
> realized that it basically does exactly the same thing (but differently)
> if it is set or not!

Typo: "ftrace_hash" should be "filter_hash", and likewise in the commit
title.

> If it is set, the idea was the ops->filter_hash was being updated, and the
> code should focus on the functions that are in the ops->filter_hash and
> add them. But it still had to pay attention to the functions in the
> ops->notrace_hash, to ignore them.
> 
> If it was cleared, it focused on the ops->notrace_hash, and would add
> functions that were not in the ops->notrace_hash but would still keep
> functions in the "ops->filter_hash". Basically doing the same thing.
> 
> In reality, the __ftrace_hash_rec_update() only needs to either remove the
> functions associated to the give ops (if "inc" is set) or remove them (if
> "inc" is cleared). It has to pay attention to both the filter_hash and
> notrace_hash regardless.

AFAICT, we actually remove a latent bug here, but that bug wasn't
reachable because we never call __ftrace_hash_rec_update() with
"filter_hash" clear.

Before this patch, if we did call __ftrace_hash_rec_update() with
"filter_hash" clear, we'd setup:

	all = false;
	...
	if (filter_hash) {
		...
	} else {
		hash = ops->func_hash->notrace_hash;
		other_hash = ops->func_hash->filter_hash;
	}

... and then at the tail of the loop where we do:

	count++;

	[...] 

	/* Shortcut, if we handled all records, we are done. */
	if (!all && count == hash->count) {
		pr_info("HARK: stopping after %d recs\n", count);
		return update;
	}

... we'd be checking whether we've updated notrace_hash->count entries,
when that could be smaller than the number of entries we actually need
to update (e.g. if the notrace hash contains a single entry, but the
filter_hash contained more).

As above, we never actually hit that because we never call with
"filter_hash" clear, so it might be good to explicitly say that before
this patch we never actually call __ftrace_hash_rec_update() with
"filter_hash" clear, and this is removing dead (and potentially broken)
code.

> Remove the "filter_hash" parameter from __filter_hash_rec_update() and
> comment the function for what it really is doing.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

FWIW, this looks good to me, and works in testing, so:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

I have one comment below, but the above stands regardless.

[...]

> +/*
> + * This is the main engine to the ftrace updates to the dyn_ftrace records.
> + *
> + * It will iterate through all the available ftrace functions
> + * (the ones that ftrace can have callbacks to) and set the flags
> + * in the associated dyn_ftrace records.
> + *
> + * @inc: If true, the functions associated to @ops are added to
> + *       the dyn_ftrace records, otherwise they are removed.
> + */
>  static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
> -				     int filter_hash,
>  				     bool inc)
>  {
>  	struct ftrace_hash *hash;
> -	struct ftrace_hash *other_hash;
> +	struct ftrace_hash *notrace_hash;
>  	struct ftrace_page *pg;
>  	struct dyn_ftrace *rec;
>  	bool update = false;
> @@ -1718,35 +1725,16 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
>  		return false;
>  
>  	/*
> -	 * In the filter_hash case:
>  	 *   If the count is zero, we update all records.
>  	 *   Otherwise we just update the items in the hash.
> -	 *
> -	 * In the notrace_hash case:
> -	 *   We enable the update in the hash.
> -	 *   As disabling notrace means enabling the tracing,
> -	 *   and enabling notrace means disabling, the inc variable
> -	 *   gets inversed.
>  	 */
> -	if (filter_hash) {
> -		hash = ops->func_hash->filter_hash;
> -		other_hash = ops->func_hash->notrace_hash;
> -		if (ftrace_hash_empty(hash))
> -			all = true;
> -	} else {
> -		inc = !inc;
> -		hash = ops->func_hash->notrace_hash;
> -		other_hash = ops->func_hash->filter_hash;
> -		/*
> -		 * If the notrace hash has no items,
> -		 * then there's nothing to do.
> -		 */
> -		if (ftrace_hash_empty(hash))
> -			return false;
> -	}
> +	hash = ops->func_hash->filter_hash;
> +	notrace_hash = ops->func_hash->notrace_hash;
> +	if (ftrace_hash_empty(hash))
> +		all = true;
>  
>  	do_for_each_ftrace_rec(pg, rec) {
> -		int in_other_hash = 0;
> +		int in_notrace_hash = 0;
>  		int in_hash = 0;
>  		int match = 0;
>  
> @@ -1758,26 +1746,17 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
>  			 * Only the filter_hash affects all records.
>  			 * Update if the record is not in the notrace hash.
>  			 */
> -			if (!other_hash || !ftrace_lookup_ip(other_hash, rec->ip))
> +			if (!notrace_hash || !ftrace_lookup_ip(notrace_hash, rec->ip))
>  				match = 1;
>  		} else {
>  			in_hash = !!ftrace_lookup_ip(hash, rec->ip);
> -			in_other_hash = !!ftrace_lookup_ip(other_hash, rec->ip);
> +			in_notrace_hash = !!ftrace_lookup_ip(notrace_hash, rec->ip);
>  
>  			/*
> -			 * If filter_hash is set, we want to match all functions
> -			 * that are in the hash but not in the other hash.
> -			 *
> -			 * If filter_hash is not set, then we are decrementing.
> -			 * That means we match anything that is in the hash
> -			 * and also in the other_hash. That is, we need to turn
> -			 * off functions in the other hash because they are disabled
> -			 * by this hash.
> +			 * We want to match all functions that are in the hash but
> +			 * not in the other hash.
>  			 */
> -			if (filter_hash && in_hash && !in_other_hash)
> -				match = 1;
> -			else if (!filter_hash && in_hash &&
> -				 (in_other_hash || ftrace_hash_empty(other_hash)))
> +			if (in_hash && !in_notrace_hash)
>  				match = 1;
>  		}
>  		if (!match)

I wonder how much the (subsequent) shortcut for count == hash->count
matters in practice, because if we were happy to get rid of that, we
could get rid of 'all', 'count', 'in_hash', 'in_notrace_hash', and
simplify the above down to:

	do_for_each_ftrace_rec(pg, rec) {
		if (skip_record(rec))
			continue;

		/*
		 * When the hash is empty we update all records.
		 * Otherwise we just update the items in the hash.
		 */
		if (!ftrace_hash_empty(hash) &&
		    !ftrace_lookup_ip(hash, rec->ip))
			continue;

		if (!ftrace_lookup_ip(notrace_hash, rec->ip))
			continue;

		[...]
			/* do the actual updates here */
		[...]

	} while_for_each_ftrace_rec();

... which'd be easier to follow.

FWIW, diff atop this patch below. It passes the selftests in my local
testing, but I understand if we'd rather keep the shortcut.

Mark.

---->8----
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f1aab82fa465..165e8dd4f894 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1714,49 +1714,27 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
 	struct ftrace_page *pg;
 	struct dyn_ftrace *rec;
 	bool update = false;
-	int count = 0;
-	int all = false;
 
 	/* Only update if the ops has been registered */
 	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
 		return false;
 
-	/*
-	 *   If the count is zero, we update all records.
-	 *   Otherwise we just update the items in the hash.
-	 */
 	hash = ops->func_hash->filter_hash;
 	notrace_hash = ops->func_hash->notrace_hash;
-	if (ftrace_hash_empty(hash))
-		all = true;
 
 	do_for_each_ftrace_rec(pg, rec) {
-		int in_notrace_hash = 0;
-		int in_hash = 0;
-		int match = 0;
-
 		if (skip_record(rec))
 			continue;
 
-		if (all) {
-			/*
-			 * Only the filter_hash affects all records.
-			 * Update if the record is not in the notrace hash.
-			 */
-			if (!notrace_hash || !ftrace_lookup_ip(notrace_hash, rec->ip))
-				match = 1;
-		} else {
-			in_hash = !!ftrace_lookup_ip(hash, rec->ip);
-			in_notrace_hash = !!ftrace_lookup_ip(notrace_hash, rec->ip);
+		/*
+		 * If the hash is empty, we update all records.
+		 * Otherwise we just update the items in the hash.
+		 */
+		if (!ftrace_hash_empty(hash) &&
+		    !ftrace_lookup_ip(hash, rec->ip))
+			continue;
 
-			/*
-			 * We want to match all functions that are in the hash but
-			 * not in the other hash.
-			 */
-			if (in_hash && !in_notrace_hash)
-				match = 1;
-		}
-		if (!match)
+		if (ftrace_lookup_ip(notrace_hash, rec->ip))
 			continue;
 
 		if (inc) {
@@ -1846,14 +1824,9 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
 		else
 			rec->flags &= ~FTRACE_FL_CALL_OPS;
 
-		count++;
-
 		/* Must match FTRACE_UPDATE_CALLS in ftrace_modify_all_code() */
 		update |= ftrace_test_record(rec, true) != FTRACE_UPDATE_IGNORE;
 
-		/* Shortcut, if we handled all records, we are done. */
-		if (!all && count == hash->count)
-			return update;
 	} while_for_each_ftrace_rec();
 
 	return update;

Re: [PATCH v2 2/5] ftrace: Remove "ftrace_hash" parameter from __ftrace_hash_rec_update()

[Steven Rostedt]

On Thu, 6 Jun 2024 18:53:12 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Wed, Jun 05, 2024 at 02:03:36PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> > 
> > While adding comments to the function __ftrace_hash_rec_update() and
> > trying to describe in detail what the parameter for "ftrace_hash" does, I
> > realized that it basically does exactly the same thing (but differently)
> > if it is set or not!  
> 
> Typo: "ftrace_hash" should be "filter_hash", and likewise in the commit
> title.

Let me go fix up linux-next :-p

> 
> > If it is set, the idea was the ops->filter_hash was being updated, and the
> > code should focus on the functions that are in the ops->filter_hash and
> > add them. But it still had to pay attention to the functions in the
> > ops->notrace_hash, to ignore them.
> > 
> > If it was cleared, it focused on the ops->notrace_hash, and would add
> > functions that were not in the ops->notrace_hash but would still keep
> > functions in the "ops->filter_hash". Basically doing the same thing.
> > 
> > In reality, the __ftrace_hash_rec_update() only needs to either remove the
> > functions associated to the give ops (if "inc" is set) or remove them (if
> > "inc" is cleared). It has to pay attention to both the filter_hash and
> > notrace_hash regardless.  
> 
> AFAICT, we actually remove a latent bug here, but that bug wasn't
> reachable because we never call __ftrace_hash_rec_update() with
> "filter_hash" clear.
> 
> Before this patch, if we did call __ftrace_hash_rec_update() with
> "filter_hash" clear, we'd setup:
> 
> 	all = false;
> 	...
> 	if (filter_hash) {
> 		...
> 	} else {
> 		hash = ops->func_hash->notrace_hash;
> 		other_hash = ops->func_hash->filter_hash;
> 	}
> 
> ... and then at the tail of the loop where we do:
> 
> 	count++;
> 
> 	[...] 
> 
> 	/* Shortcut, if we handled all records, we are done. */
> 	if (!all && count == hash->count) {
> 		pr_info("HARK: stopping after %d recs\n", count);
> 		return update;
> 	}
> 
> ... we'd be checking whether we've updated notrace_hash->count entries,
> when that could be smaller than the number of entries we actually need
> to update (e.g. if the notrace hash contains a single entry, but the
> filter_hash contained more).

I noticed this too as well as:

	if (filter_hash) {
		hash = ops->func_hash->filter_hash;
		other_hash = ops->func_hash->notrace_hash;
		if (ftrace_hash_empty(hash))
			all = true;
	} else {
		inc = !inc;
		hash = ops->func_hash->notrace_hash;
		other_hash = ops->func_hash->filter_hash;
		/*
		 * If the notrace hash has no items,
		 * then there's nothing to do.
		 */
		if (ftrace_hash_empty(hash))
			return false;
	}

That "return false" is actually a mistake as well. But I tried to hit
it, but found that I could not. I think I updated the code due to bugs
where I prevent that from happening, but the real fix would have been
this patch. :-p

> 
> As above, we never actually hit that because we never call with
> "filter_hash" clear, so it might be good to explicitly say that before
> this patch we never actually call __ftrace_hash_rec_update() with
> "filter_hash" clear, and this is removing dead (and potentially broken)
> code.

Right.

> 
> > Remove the "filter_hash" parameter from __filter_hash_rec_update() and
> > comment the function for what it really is doing.
> > 
> > Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>  
> 
> FWIW, this looks good to me, and works in testing, so:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> 
> I have one comment below, but the above stands regardless.
> 
> [...]
> 
> > +/*
> > + * This is the main engine to the ftrace updates to the dyn_ftrace records.
> > + *
> > + * It will iterate through all the available ftrace functions
> > + * (the ones that ftrace can have callbacks to) and set the flags
> > + * in the associated dyn_ftrace records.
> > + *
> > + * @inc: If true, the functions associated to @ops are added to
> > + *       the dyn_ftrace records, otherwise they are removed.
> > + */
> >  static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
> > -				     int filter_hash,
> >  				     bool inc)
> >  {
> >  	struct ftrace_hash *hash;
> > -	struct ftrace_hash *other_hash;
> > +	struct ftrace_hash *notrace_hash;
> >  	struct ftrace_page *pg;
> >  	struct dyn_ftrace *rec;
> >  	bool update = false;
> > @@ -1718,35 +1725,16 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
> >  		return false;
> >  
> >  	/*
> > -	 * In the filter_hash case:
> >  	 *   If the count is zero, we update all records.
> >  	 *   Otherwise we just update the items in the hash.
> > -	 *
> > -	 * In the notrace_hash case:
> > -	 *   We enable the update in the hash.
> > -	 *   As disabling notrace means enabling the tracing,
> > -	 *   and enabling notrace means disabling, the inc variable
> > -	 *   gets inversed.
> >  	 */
> > -	if (filter_hash) {
> > -		hash = ops->func_hash->filter_hash;
> > -		other_hash = ops->func_hash->notrace_hash;
> > -		if (ftrace_hash_empty(hash))
> > -			all = true;
> > -	} else {
> > -		inc = !inc;
> > -		hash = ops->func_hash->notrace_hash;
> > -		other_hash = ops->func_hash->filter_hash;
> > -		/*
> > -		 * If the notrace hash has no items,
> > -		 * then there's nothing to do.
> > -		 */
> > -		if (ftrace_hash_empty(hash))
> > -			return false;
> > -	}
> > +	hash = ops->func_hash->filter_hash;
> > +	notrace_hash = ops->func_hash->notrace_hash;
> > +	if (ftrace_hash_empty(hash))
> > +		all = true;
> >  
> >  	do_for_each_ftrace_rec(pg, rec) {
> > -		int in_other_hash = 0;
> > +		int in_notrace_hash = 0;
> >  		int in_hash = 0;
> >  		int match = 0;
> >  
> > @@ -1758,26 +1746,17 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
> >  			 * Only the filter_hash affects all records.
> >  			 * Update if the record is not in the notrace hash.
> >  			 */
> > -			if (!other_hash || !ftrace_lookup_ip(other_hash, rec->ip))
> > +			if (!notrace_hash || !ftrace_lookup_ip(notrace_hash, rec->ip))
> >  				match = 1;
> >  		} else {
> >  			in_hash = !!ftrace_lookup_ip(hash, rec->ip);
> > -			in_other_hash = !!ftrace_lookup_ip(other_hash, rec->ip);
> > +			in_notrace_hash = !!ftrace_lookup_ip(notrace_hash, rec->ip);
> >  
> >  			/*
> > -			 * If filter_hash is set, we want to match all functions
> > -			 * that are in the hash but not in the other hash.
> > -			 *
> > -			 * If filter_hash is not set, then we are decrementing.
> > -			 * That means we match anything that is in the hash
> > -			 * and also in the other_hash. That is, we need to turn
> > -			 * off functions in the other hash because they are disabled
> > -			 * by this hash.
> > +			 * We want to match all functions that are in the hash but
> > +			 * not in the other hash.
> >  			 */
> > -			if (filter_hash && in_hash && !in_other_hash)
> > -				match = 1;
> > -			else if (!filter_hash && in_hash &&
> > -				 (in_other_hash || ftrace_hash_empty(other_hash)))
> > +			if (in_hash && !in_notrace_hash)
> >  				match = 1;
> >  		}
> >  		if (!match)  
> 
> I wonder how much the (subsequent) shortcut for count == hash->count
> matters in practice, because if we were happy to get rid of that, we
> could get rid of 'all', 'count', 'in_hash', 'in_notrace_hash', and
> simplify the above down to:
> 
> 	do_for_each_ftrace_rec(pg, rec) {
> 		if (skip_record(rec))
> 			continue;
> 
> 		/*
> 		 * When the hash is empty we update all records.
> 		 * Otherwise we just update the items in the hash.
> 		 */
> 		if (!ftrace_hash_empty(hash) &&
> 		    !ftrace_lookup_ip(hash, rec->ip))
> 			continue;
> 
> 		if (!ftrace_lookup_ip(notrace_hash, rec->ip))
> 			continue;
> 
> 		[...]
> 			/* do the actual updates here */
> 		[...]
> 
> 	} while_for_each_ftrace_rec();
> 
> ... which'd be easier to follow.
> 
> FWIW, diff atop this patch below. It passes the selftests in my local
> testing, but I understand if we'd rather keep the shortcut.

I'll have to do benchmarks. This loop is what takes up a lot of time
when you enable function tracing. It loops over 40,000 records (just do
a wc -l available_filter_functions to get the true count).

But if you want to send a formal patch, I could test it.

Thanks,

-- Steve


> 
> Mark.
> 
> ---->8----  
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index f1aab82fa465..165e8dd4f894 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1714,49 +1714,27 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
>  	struct ftrace_page *pg;
>  	struct dyn_ftrace *rec;
>  	bool update = false;
> -	int count = 0;
> -	int all = false;
>  
>  	/* Only update if the ops has been registered */
>  	if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
>  		return false;
>  
> -	/*
> -	 *   If the count is zero, we update all records.
> -	 *   Otherwise we just update the items in the hash.
> -	 */
>  	hash = ops->func_hash->filter_hash;
>  	notrace_hash = ops->func_hash->notrace_hash;
> -	if (ftrace_hash_empty(hash))
> -		all = true;
>  
>  	do_for_each_ftrace_rec(pg, rec) {
> -		int in_notrace_hash = 0;
> -		int in_hash = 0;
> -		int match = 0;
> -
>  		if (skip_record(rec))
>  			continue;
>  
> -		if (all) {
> -			/*
> -			 * Only the filter_hash affects all records.
> -			 * Update if the record is not in the notrace hash.
> -			 */
> -			if (!notrace_hash || !ftrace_lookup_ip(notrace_hash, rec->ip))
> -				match = 1;
> -		} else {
> -			in_hash = !!ftrace_lookup_ip(hash, rec->ip);
> -			in_notrace_hash = !!ftrace_lookup_ip(notrace_hash, rec->ip);
> +		/*
> +		 * If the hash is empty, we update all records.
> +		 * Otherwise we just update the items in the hash.
> +		 */
> +		if (!ftrace_hash_empty(hash) &&
> +		    !ftrace_lookup_ip(hash, rec->ip))
> +			continue;
>  
> -			/*
> -			 * We want to match all functions that are in the hash but
> -			 * not in the other hash.
> -			 */
> -			if (in_hash && !in_notrace_hash)
> -				match = 1;
> -		}
> -		if (!match)
> +		if (ftrace_lookup_ip(notrace_hash, rec->ip))
>  			continue;
>  
>  		if (inc) {
> @@ -1846,14 +1824,9 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
>  		else
>  			rec->flags &= ~FTRACE_FL_CALL_OPS;
>  
> -		count++;
> -
>  		/* Must match FTRACE_UPDATE_CALLS in ftrace_modify_all_code() */
>  		update |= ftrace_test_record(rec, true) != FTRACE_UPDATE_IGNORE;
>  
> -		/* Shortcut, if we handled all records, we are done. */
> -		if (!all && count == hash->count)
> -			return update;
>  	} while_for_each_ftrace_rec();
>  
>  	return update;
> 

[PATCH v2 3/5] ftrace: Add comments to ftrace_hash_rec_disable/enable()

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Add comments to describe what the functions ftrace_hash_rec_disable() and
ftrace_hash_rec_enable() do. Also change the passing of the "inc" variable
to __ftrace_hash_rec_update() to a boolean value as that is what it is
supposed to take.

Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f8d8de7adade..1a2444199740 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1862,14 +1862,24 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
 	return update;
 }
 
+/*
+ * This is called when an ops is removed from tracing. It will decrement
+ * the counters of the dyn_ftrace records for all the functions that
+ * the @ops attached to.
+ */
 static bool ftrace_hash_rec_disable(struct ftrace_ops *ops)
 {
-	return __ftrace_hash_rec_update(ops, 0);
+	return __ftrace_hash_rec_update(ops, false);
 }
 
+/*
+ * This is called when an ops is added to tracing. It will increment
+ * the counters of the dyn_ftrace records for all the functions that
+ * the @ops attached to.
+ */
 static bool ftrace_hash_rec_enable(struct ftrace_ops *ops)
 {
-	return __ftrace_hash_rec_update(ops, 1);
+	return __ftrace_hash_rec_update(ops, true);
 }
 
 static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops, int inc)
-- 
2.43.0

[PATCH v2 4/5] ftrace: Convert "inc" parameter to bool in ftrace_hash_rec_update_modify()

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

The parameter "inc" in the function ftrace_hash_rec_update_modify() is
boolean. Change it to be such.

Also add documentation to what the function does.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 1a2444199740..83f23f8fc26d 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1882,7 +1882,24 @@ static bool ftrace_hash_rec_enable(struct ftrace_ops *ops)
 	return __ftrace_hash_rec_update(ops, true);
 }
 
-static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops, int inc)
+/*
+ * This function will update what functions @ops traces when its filter
+ * changes.
+ *
+ * The @inc states if the @ops callbacks are going to be added or removed.
+ * When one of the @ops hashes are updated to a "new_hash" the dyn_ftrace
+ * records are update via:
+ *
+ * ftrace_hash_rec_disable_modify(ops);
+ * ops->hash = new_hash
+ * ftrace_hash_rec_enable_modify(ops);
+ *
+ * Where the @ops is removed from all the records it is tracing using
+ * its old hash. The @ops hash is updated to the new hash, and then
+ * the @ops is added back to the records so that it is tracing all
+ * the new functions.
+ */
+static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops, bool inc)
 {
 	struct ftrace_ops *op;
 
@@ -1906,12 +1923,12 @@ static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops, int inc)
 
 static void ftrace_hash_rec_disable_modify(struct ftrace_ops *ops)
 {
-	ftrace_hash_rec_update_modify(ops, 0);
+	ftrace_hash_rec_update_modify(ops, false);
 }
 
 static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops)
 {
-	ftrace_hash_rec_update_modify(ops, 1);
+	ftrace_hash_rec_update_modify(ops, true);
 }
 
 /*
-- 
2.43.0

Re: [PATCH v2 4/5] ftrace: Convert "inc" parameter to bool in ftrace_hash_rec_update_modify()

[Mark Rutland]

On Wed, Jun 05, 2024 at 02:03:38PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> The parameter "inc" in the function ftrace_hash_rec_update_modify() is
> boolean. Change it to be such.
> 
> Also add documentation to what the function does.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  kernel/trace/ftrace.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 1a2444199740..83f23f8fc26d 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1882,7 +1882,24 @@ static bool ftrace_hash_rec_enable(struct ftrace_ops *ops)
>  	return __ftrace_hash_rec_update(ops, true);
>  }
>  
> -static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops, int inc)
> +/*
> + * This function will update what functions @ops traces when its filter
> + * changes.
> + *
> + * The @inc states if the @ops callbacks are going to be added or removed.
> + * When one of the @ops hashes are updated to a "new_hash" the dyn_ftrace
> + * records are update via:
> + *
> + * ftrace_hash_rec_disable_modify(ops);
> + * ops->hash = new_hash
> + * ftrace_hash_rec_enable_modify(ops);
> + *
> + * Where the @ops is removed from all the records it is tracing using
> + * its old hash. The @ops hash is updated to the new hash, and then
> + * the @ops is added back to the records so that it is tracing all
> + * the new functions.
> + */
> +static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops, bool inc)
>  {
>  	struct ftrace_ops *op;
>  
> @@ -1906,12 +1923,12 @@ static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops, int inc)
>  
>  static void ftrace_hash_rec_disable_modify(struct ftrace_ops *ops)
>  {
> -	ftrace_hash_rec_update_modify(ops, 0);
> +	ftrace_hash_rec_update_modify(ops, false);
>  }
>  
>  static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops)
>  {
> -	ftrace_hash_rec_update_modify(ops, 1);
> +	ftrace_hash_rec_update_modify(ops, true);
>  }
>  
>  /*
> -- 
> 2.43.0
> 
> 

[PATCH v2 5/5] ftrace: Add comments to ftrace_hash_move() and friends

[Steven Rostedt]

From: "Steven Rostedt (Google)" <rostedt@goodmis.org>

Describe what ftrace_hash_move() does and add some more comments to some
other functions to make it easier to understand.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/trace/ftrace.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 83f23f8fc26d..ae1603e771c5 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -169,6 +169,7 @@ static inline void ftrace_ops_init(struct ftrace_ops *ops)
 #endif
 }
 
+/* Call this function for when a callback filters on set_ftrace_pid */
 static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip,
 			    struct ftrace_ops *op, struct ftrace_regs *fregs)
 {
@@ -1317,7 +1318,7 @@ static struct ftrace_hash *alloc_ftrace_hash(int size_bits)
 	return hash;
 }
 
-
+/* Used to save filters on functions for modules not loaded yet */
 static int ftrace_add_mod(struct trace_array *tr,
 			  const char *func, const char *module,
 			  int enable)
@@ -1429,6 +1430,7 @@ static struct ftrace_hash *__move_hash(struct ftrace_hash *src, int size)
 	return new_hash;
 }
 
+/* Move the @src entries to a newly allocated hash */
 static struct ftrace_hash *
 __ftrace_hash_move(struct ftrace_hash *src)
 {
@@ -1443,6 +1445,26 @@ __ftrace_hash_move(struct ftrace_hash *src)
 	return __move_hash(src, size);
 }
 
+/**
+ * ftrace_hash_move - move a new hash to a filter and do updates
+ * @ops: The ops with the hash that @dst points to
+ * @enable: True if for the filter hash, false for the notrace hash
+ * @dst: Points to the @ops hash that should be updated
+ * @src: The hash to update @dst with
+ *
+ * This is called when an ftrace_ops hash is being updated and the
+ * the kernel needs to reflect this. Note, this only updates the kernel
+ * function callbacks if the @ops is enabled (not to be confused with
+ * @enable above). If the @ops is enabled, its hash determines what
+ * callbacks get called. This function gets called when the @ops hash
+ * is updated and it requires new callbacks.
+ *
+ * On success the elements of @src is moved to @dst, and @dst is updated
+ * properly, as well as the functions determined by the @ops hashes
+ * are now calling the @ops callback function.
+ *
+ * Regardless of return type, @src should be freed with free_ftrace_hash().
+ */
 static int
 ftrace_hash_move(struct ftrace_ops *ops, int enable,
 		 struct ftrace_hash **dst, struct ftrace_hash *src)
-- 
2.43.0

Re: [PATCH v2 5/5] ftrace: Add comments to ftrace_hash_move() and friends

[Mark Rutland]

On Wed, Jun 05, 2024 at 02:03:39PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
> 
> Describe what ftrace_hash_move() does and add some more comments to some
> other functions to make it easier to understand.
> 
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  kernel/trace/ftrace.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 83f23f8fc26d..ae1603e771c5 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -169,6 +169,7 @@ static inline void ftrace_ops_init(struct ftrace_ops *ops)
>  #endif
>  }
>  
> +/* Call this function for when a callback filters on set_ftrace_pid */
>  static void ftrace_pid_func(unsigned long ip, unsigned long parent_ip,
>  			    struct ftrace_ops *op, struct ftrace_regs *fregs)
>  {
> @@ -1317,7 +1318,7 @@ static struct ftrace_hash *alloc_ftrace_hash(int size_bits)
>  	return hash;
>  }
>  
> -
> +/* Used to save filters on functions for modules not loaded yet */
>  static int ftrace_add_mod(struct trace_array *tr,
>  			  const char *func, const char *module,
>  			  int enable)
> @@ -1429,6 +1430,7 @@ static struct ftrace_hash *__move_hash(struct ftrace_hash *src, int size)
>  	return new_hash;
>  }
>  
> +/* Move the @src entries to a newly allocated hash */
>  static struct ftrace_hash *
>  __ftrace_hash_move(struct ftrace_hash *src)
>  {
> @@ -1443,6 +1445,26 @@ __ftrace_hash_move(struct ftrace_hash *src)
>  	return __move_hash(src, size);
>  }
>  
> +/**
> + * ftrace_hash_move - move a new hash to a filter and do updates
> + * @ops: The ops with the hash that @dst points to
> + * @enable: True if for the filter hash, false for the notrace hash
> + * @dst: Points to the @ops hash that should be updated
> + * @src: The hash to update @dst with
> + *
> + * This is called when an ftrace_ops hash is being updated and the
> + * the kernel needs to reflect this. Note, this only updates the kernel
> + * function callbacks if the @ops is enabled (not to be confused with
> + * @enable above). If the @ops is enabled, its hash determines what
> + * callbacks get called. This function gets called when the @ops hash
> + * is updated and it requires new callbacks.
> + *
> + * On success the elements of @src is moved to @dst, and @dst is updated
> + * properly, as well as the functions determined by the @ops hashes
> + * are now calling the @ops callback function.
> + *
> + * Regardless of return type, @src should be freed with free_ftrace_hash().
> + */
>  static int
>  ftrace_hash_move(struct ftrace_ops *ops, int enable,
>  		 struct ftrace_hash **dst, struct ftrace_hash *src)
> -- 
> 2.43.0
> 
> 

Re: [PATCH v2 0/5] ftrace: Clean up and comment code

[Masami Hiramatsu]

On Wed, 05 Jun 2024 14:03:34 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> While working on the function_graph multiple users code, I realized
> that I was struggling with how the ftrace code worked. Being the
> author of such code meant that it wasn't very intuitive. Namely, the
> function names were not descriptive enough, or at least, they needed
> comments.
> 
> This series moves to solve some of that via changing a couple function
> names and parameters and adding comments to many of them.
> 

This series looks good to me.

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

for this series.

Thanks!

> There's more to do, but this at least moves it in the right direction.
> 
> Changes since v1: https://lore.kernel.org/all/20240604212817.384103202@goodmis.org/
> 
> - While working on v1 and responding to a comment from Mark Rutland about
>   the usage of "ftrace_hash" in the __ftrace_hash_rec_update() code,
>   I realized that the function does pretty much the same thing if
>   it is set or not set (but slightly differently). It turns out that
>   it isn't needed and that parameter can be removed, making the code
>   simpler.
> 
> - Fixed some wording and typos suggested by Mark Rutland.
> 
> Steven Rostedt (Google) (5):
>       ftrace: Rename dup_hash() and comment it
>       ftrace: Remove "ftrace_hash" parameter from __ftrace_hash_rec_update()
>       ftrace: Add comments to ftrace_hash_rec_disable/enable()
>       ftrace: Convert "inc" parameter to bool in ftrace_hash_rec_update_modify()
>       ftrace: Add comments to ftrace_hash_move() and friends
> 
> ----
>  kernel/trace/ftrace.c | 161 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 94 insertions(+), 67 deletions(-)


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>