Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 4/5] tracing: Unify the logic for function tracing
 options
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-kernel@vger.kernel.org, tglx@linutronix.de,
        peterz@infradead.org
References: <20210329130533.199507-1-y.karadz@gmail.com>
 <20210329130533.199507-5-y.karadz@gmail.com>
 <20210405181525.0d4a724e@gandalf.local.home>
From:   "Yordan Karadzhov (VMware)" <y.karadz@gmail.com>
Message-ID: <3eb21018-d10d-6f05-cd91-e514d260f924@gmail.com>
Date:   Wed, 7 Apr 2021 16:34:11 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <20210405181525.0d4a724e@gandalf.local.home>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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