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

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.


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

----
 kernel/trace/ftrace.c | 103 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 86 insertions(+), 17 deletions(-)

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.
+ *
+ * @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;

-- 
2.43.0

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

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

The functions ftrace_hash_rec_disable() and ftrace_hash_rec_enable()
always has 1 passed to its "ftrace_hash" parameter. Remove the parameter
and pass in true to __ftrace_hash_rec_update().

Also add some comments to both those functions explaining what they do.

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

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 93c7c5fd4249..de652201c86c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c

@@ -1895,16 +1895,24 @@ 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)
+/*
+ * 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, filter_hash, 0);
+	return __ftrace_hash_rec_update(ops, true, 0);
 }
 
-static bool ftrace_hash_rec_enable(struct ftrace_ops *ops,
-				   int filter_hash)
+/*
+ * 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, filter_hash, 1);
+	return __ftrace_hash_rec_update(ops, true, 1);
 }
 
 static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops,

@@ -3062,7 +3070,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);

@@ -3104,7 +3112,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

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

The parameters "filter_hash" and "inc" in the function
ftrace_hash_rec_update_modify() are boolean. Change them to be such.

Also add documentation to what the function does.

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

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index de652201c86c..021024164938 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c

@@ -1915,8 +1915,31 @@ static bool ftrace_hash_rec_enable(struct ftrace_ops *ops)
 	return __ftrace_hash_rec_update(ops, true, 1);
 }
 
+/*
+ * This function will update what functions @ops traces when its filter
+ * changes. @filter_hash is set to true when modifying the filter_hash
+ * and set to false when modifying the notrace_hash.
+ *
+ * For example, if the user does: echo schedule > set_ftrace_filter
+ * that would call: ftrace_hash_rec_update_modify(ops, true, true);
+ *
+ * For: echo schedule >> set_ftrace_notrace
+ * That would call: ftrace_hash_rec_enable(ops, false, true);
+ *
+ * The @inc states if the @ops callbacks are going to be added or removed.
+ * The dyn_ftrace records are update via:
+ *
+ * ftrace_hash_rec_disable_modify(ops, filter_hash);
+ * ops->hash = new_hash
+ * ftrace_hash_rec_enable_modify(ops, filter_hash);
+ *
+ * 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,
-					  int filter_hash, int inc)
+					  bool filter_hash, bool inc)
 {
 	struct ftrace_ops *op;
 

@@ -1939,15 +1962,15 @@ static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops,
 }
 
 static void ftrace_hash_rec_disable_modify(struct ftrace_ops *ops,
-					   int filter_hash)
+					   bool filter_hash)
 {
-	ftrace_hash_rec_update_modify(ops, filter_hash, 0);
+	ftrace_hash_rec_update_modify(ops, filter_hash, false);
 }
 
 static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,
-					  int filter_hash)
+					  bool filter_hash)
 {
-	ftrace_hash_rec_update_modify(ops, filter_hash, 1);
+	ftrace_hash_rec_update_modify(ops, filter_hash, true);
 }
 
 /*

-- 
2.43.0

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 021024164938..2d955f0c688f 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)

@@ -1431,6 +1432,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)
 {

@@ -1445,6 +1447,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

Hi Steven,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]
[also build test ERROR on linus/master v6.10-rc2 next-20240604]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Steven-Rostedt/ftrace-Rename-dup_hash-and-comment-it/20240605-053138
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20240604212855.046127611%40goodmis.org
patch subject: [PATCH 4/5] ftrace: Convert "filter_hash" and "inc" to bool in ftrace_hash_rec_update_modify()
config: s390-defconfig (https://download.01.org/0day-ci/archive/20240605/202406050838.7r32JzDI-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project d7d2d4f53fc79b4b58e8d8d08151b577c3699d4a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240605/202406050838.7r32JzDI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot [off-list ref]
| Closes: https://lore.kernel.org/oe-kbuild-all/202406050838.7r32JzDI-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from kernel/trace/ftrace.c:17:
   In file included from include/linux/stop_machine.h:5:
   In file included from include/linux/cpu.h:17:
   In file included from include/linux/node.h:18:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:173:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2245:
   include/linux/vmstat.h:484:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     484 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     485 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:491:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     491 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     492 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:498:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     498 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:503:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     503 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     504 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:512:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     512 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     513 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   In file included from kernel/trace/ftrace.c:18:
   In file included from include/linux/clocksource.h:22:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
         |                                                           ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
     102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
         |                                                      ^
   In file included from kernel/trace/ftrace.c:18:
   In file included from include/linux/clocksource.h:22:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
         |                                                           ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
     115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
         |                                                      ^
   In file included from kernel/trace/ftrace.c:18:
   In file included from include/linux/clocksource.h:22:
   In file included from arch/s390/include/asm/io.h:93:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     693 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     701 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     709 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     718 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     727 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     736 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^

quoted

kernel/trace/ftrace.c:1961:13: error: conflicting types for 'ftrace_hash_rec_disable_modify'

    1961 | static void ftrace_hash_rec_disable_modify(struct ftrace_ops *ops,
         |             ^
   kernel/trace/ftrace.c:1384:1: note: previous declaration is here
    1384 | ftrace_hash_rec_disable_modify(struct ftrace_ops *ops, int filter_hash);
         | ^

quoted

kernel/trace/ftrace.c:1967:13: error: conflicting types for 'ftrace_hash_rec_enable_modify'

    1967 | static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,
         |             ^
   kernel/trace/ftrace.c:1386:1: note: previous declaration is here
    1386 | ftrace_hash_rec_enable_modify(struct ftrace_ops *ops, int filter_hash);
         | ^
   17 warnings and 2 errors generated.


vim +/ftrace_hash_rec_disable_modify +1961 kernel/trace/ftrace.c

84261912ebee412 Steven Rostedt (Red Hat  2014-08-18  1960) 
84261912ebee412 Steven Rostedt (Red Hat  2014-08-18 @1961) static void ftrace_hash_rec_disable_modify(struct ftrace_ops *ops,
5177364f840058a Steven Rostedt (Google   2024-06-04  1962) 					   bool filter_hash)
84261912ebee412 Steven Rostedt (Red Hat  2014-08-18  1963) {
5177364f840058a Steven Rostedt (Google   2024-06-04  1964) 	ftrace_hash_rec_update_modify(ops, filter_hash, false);
84261912ebee412 Steven Rostedt (Red Hat  2014-08-18  1965) }
84261912ebee412 Steven Rostedt (Red Hat  2014-08-18  1966) 
84261912ebee412 Steven Rostedt (Red Hat  2014-08-18 @1967) static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,
5177364f840058a Steven Rostedt (Google   2024-06-04  1968) 					  bool filter_hash)
84261912ebee412 Steven Rostedt (Red Hat  2014-08-18  1969) {
5177364f840058a Steven Rostedt (Google   2024-06-04  1970) 	ftrace_hash_rec_update_modify(ops, filter_hash, true);
84261912ebee412 Steven Rostedt (Red Hat  2014-08-18  1971) }
84261912ebee412 Steven Rostedt (Red Hat  2014-08-18  1972) 

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On Wed, 5 Jun 2024 09:12:57 +0800
kernel test robot [off-list ref] wrote:

quoted

quoted

kernel/trace/ftrace.c:1961:13: error: conflicting types for 'ftrace_hash_rec_disable_modify'  

    1961 | static void ftrace_hash_rec_disable_modify(struct ftrace_ops *ops,
         |             ^
   kernel/trace/ftrace.c:1384:1: note: previous declaration is here
    1384 | ftrace_hash_rec_disable_modify(struct ftrace_ops *ops, int filter_hash);
         | ^

quoted

quoted

kernel/trace/ftrace.c:1967:13: error: conflicting types for 'ftrace_hash_rec_enable_modify'  

    1967 | static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,

Of course these static functions have prototypes to be used earlier. Bah!

-- Steve

Hi Steven,

kernel test robot noticed the following build errors:

[auto build test ERROR on akpm-mm/mm-everything]
[also build test ERROR on linus/master v6.10-rc2 next-20240604]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Steven-Rostedt/ftrace-Rename-dup_hash-and-comment-it/20240605-053138
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20240604212855.046127611%40goodmis.org
patch subject: [PATCH 4/5] ftrace: Convert "filter_hash" and "inc" to bool in ftrace_hash_rec_update_modify()
config: i386-buildonly-randconfig-004-20240605 (https://download.01.org/0day-ci/archive/20240605/202406051211.TA5OOyjM-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240605/202406051211.TA5OOyjM-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot [off-list ref]
| Closes: https://lore.kernel.org/oe-kbuild-all/202406051211.TA5OOyjM-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

quoted

kernel/trace/ftrace.c:1961:13: error: conflicting types for 'ftrace_hash_rec_disable_modify'; have 'void(struct ftrace_ops *, bool)' {aka 'void(struct ftrace_ops *, _Bool)'}

    1961 | static void ftrace_hash_rec_disable_modify(struct ftrace_ops *ops,
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   kernel/trace/ftrace.c:1384:1: note: previous declaration of 'ftrace_hash_rec_disable_modify' with type 'void(struct ftrace_ops *, int)'
    1384 | ftrace_hash_rec_disable_modify(struct ftrace_ops *ops, int filter_hash);
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

quoted

kernel/trace/ftrace.c:1967:13: error: conflicting types for 'ftrace_hash_rec_enable_modify'; have 'void(struct ftrace_ops *, bool)' {aka 'void(struct ftrace_ops *, _Bool)'}

    1967 | static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   kernel/trace/ftrace.c:1386:1: note: previous declaration of 'ftrace_hash_rec_enable_modify' with type 'void(struct ftrace_ops *, int)'
    1386 | ftrace_hash_rec_enable_modify(struct ftrace_ops *ops, int filter_hash);
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

quoted

kernel/trace/ftrace.c:1384:1: warning: 'ftrace_hash_rec_disable_modify' used but never defined

    1384 | ftrace_hash_rec_disable_modify(struct ftrace_ops *ops, int filter_hash);
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

quoted

kernel/trace/ftrace.c:1386:1: warning: 'ftrace_hash_rec_enable_modify' used but never defined

    1386 | ftrace_hash_rec_enable_modify(struct ftrace_ops *ops, int filter_hash);
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

quoted

kernel/trace/ftrace.c:1967:13: warning: 'ftrace_hash_rec_enable_modify' defined but not used [-Wunused-function]

    1967 | static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

quoted

kernel/trace/ftrace.c:1961:13: warning: 'ftrace_hash_rec_disable_modify' defined but not used [-Wunused-function]

    1961 | static void ftrace_hash_rec_disable_modify(struct ftrace_ops *ops,
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +1961 kernel/trace/ftrace.c

84261912ebee41 Steven Rostedt (Red Hat  2014-08-18  1960) 
84261912ebee41 Steven Rostedt (Red Hat  2014-08-18 @1961) static void ftrace_hash_rec_disable_modify(struct ftrace_ops *ops,
5177364f840058 Steven Rostedt (Google   2024-06-04  1962) 					   bool filter_hash)
84261912ebee41 Steven Rostedt (Red Hat  2014-08-18  1963) {
5177364f840058 Steven Rostedt (Google   2024-06-04  1964) 	ftrace_hash_rec_update_modify(ops, filter_hash, false);
84261912ebee41 Steven Rostedt (Red Hat  2014-08-18  1965) }
84261912ebee41 Steven Rostedt (Red Hat  2014-08-18  1966) 
84261912ebee41 Steven Rostedt (Red Hat  2014-08-18 @1967) static void ftrace_hash_rec_enable_modify(struct ftrace_ops *ops,
5177364f840058 Steven Rostedt (Google   2024-06-04  1968) 					  bool filter_hash)
84261912ebee41 Steven Rostedt (Red Hat  2014-08-18  1969) {
5177364f840058 Steven Rostedt (Google   2024-06-04  1970) 	ftrace_hash_rec_update_modify(ops, filter_hash, true);
84261912ebee41 Steven Rostedt (Red Hat  2014-08-18  1971) }
84261912ebee41 Steven Rostedt (Red Hat  2014-08-18  1972) 

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On Tue, Jun 04, 2024 at 05:28:19PM -0400, Steven Rostedt wrote:

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 Tue, Jun 04, 2024 at 05:28:20PM -0400, Steven Rostedt wrote:

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

The functions ftrace_hash_rec_disable() and ftrace_hash_rec_enable()
always has 1 passed to its "ftrace_hash" parameter. Remove the parameter
and pass in true to __ftrace_hash_rec_update().

Also add some comments to both those functions explaining what they do.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Looks good to me.

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

Mark.

On Tue, Jun 04, 2024 at 05:28:21PM -0400, Steven Rostedt wrote:

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

The parameters "filter_hash" and "inc" in the function
ftrace_hash_rec_update_modify() are boolean. Change them to be such.

Also add documentation to what the function does.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>

Aside from the issue with forward declarations that need to be updated,
this looks good to me, so with that fixed:

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

Mark.

On Wed, 5 Jun 2024 11:15:38 +0100
Mark Rutland [off-list ref] wrote:

quoted

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 ;-)

quoted

+ *
+ * @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.

quoted

+ * @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 [off-list ref] wrote:

quoted

quoted

+ *
+ * @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

On Wed, 5 Jun 2024 11:17:31 +0100
Mark Rutland [off-list ref] wrote:

On Tue, Jun 04, 2024 at 05:28:20PM -0400, Steven Rostedt wrote:

quoted

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

The functions ftrace_hash_rec_disable() and ftrace_hash_rec_enable()
always has 1 passed to its "ftrace_hash" parameter. Remove the parameter
and pass in true to __ftrace_hash_rec_update().

Also add some comments to both those functions explaining what they do.

Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>  

Looks good to me.

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

I removed your Ack from v2 as it changed enough that I believe it
requires a new Ack.

-- Steve

On Tue, 04 Jun 2024 17:28:18 -0400
Steven Rostedt [off-list ref] 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.

Good change.

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

Thank you,

-- 
Masami Hiramatsu (Google) [off-list ref]

On Tue, 04 Jun 2024 17:28:20 -0400
Steven Rostedt [off-list ref] wrote:

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

The functions ftrace_hash_rec_disable() and ftrace_hash_rec_enable()
always has 1 passed to its "ftrace_hash" parameter. Remove the parameter
and pass in true to __ftrace_hash_rec_update().

Also add some comments to both those functions explaining what they do.

Looks good to me.

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

Thank you,

-- 
Masami Hiramatsu (Google) [off-list ref]

On Thu, 6 Jun 2024 06:50:18 +0900
Masami Hiramatsu (Google) [off-list ref] wrote:

On Tue, 04 Jun 2024 17:28:20 -0400
Steven Rostedt [off-list ref] wrote:

quoted

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

The functions ftrace_hash_rec_disable() and ftrace_hash_rec_enable()
always has 1 passed to its "ftrace_hash" parameter. Remove the parameter
and pass in true to __ftrace_hash_rec_update().

Also add some comments to both those functions explaining what they do.
  

Looks good to me.

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

Thanks Masami, but I sent out a v2. Could you review those patches
instead?

https://lore.kernel.org/all/20240605180334.090848865@goodmis.org/

-- Steve