Re: [PATCH v2 4/5] tracing: Unify the logic for function tracing options

[Yordan Karadzhov (VMware)]

Hi Steven,

Hi Steven,
On 6.04.21 г. 1:15, Steven Rostedt wrote:
  
@@ -235,30 +248,31 @@ static struct tracer function_trace;
  static int
  func_set_flag(struct trace_array *tr, u32 old_flags, u32 bit, int set)
  {
-       switch (bit) {
-       case TRACE_FUNC_OPT_STACK:
-               /* do nothing if already set */
-               if (!!set == !!(func_flags.val & TRACE_FUNC_OPT_STACK))
-                       break;
+       ftrace_func_t func;
+       u32 new_flags_val;
Nit, but the variable should just be "new_flags", which is consistent with
old_flags. In the kernel we don't need to the variable names to be so
verbose.

  
-		/* We can change this flag when not running. */
-               if (tr->current_trace != &function_trace)
-                       break;
+       /* Do nothing if already set. */
+       if (!!set == !!(func_flags.val & bit))
+               return 0;
  
-		unregister_ftrace_function(tr->ops);
+       /* We can change this flag only when not running. */
+       if (tr->current_trace != &function_trace)
+               return 0;
  
-		if (set) {
-                       tr->ops->func = function_stack_trace_call;
-                       register_ftrace_function(tr->ops);
-               } else {
-                       tr->ops->func = function_trace_call;
-                       register_ftrace_function(tr->ops);
-               }
+       new_flags_val = (func_flags.val & ~(1UL << (bit - 1)));
+       new_flags_val |= (set << (bit - 1));
bit is already the mask, no need to shift it, nor there's no reason for the
extra set of parenthesis. And the above can be done in one line.

        new_flags = (func_flags.val & ~bit) | (set ? bit : 0);


OK, I totally misinterpreted the meaning of the "bit" argument of the 
function. I did not realized it is a mask. I was thinking the argument 
gives only the number of the bit that changes (like 5 for the 5-th bit 
inside the mask).

Thanks!
Yordan