Message-ID: <b7b99ec6-5c6f-34a1-05be-a109b4328eab@linaro.org>
Date:   Mon, 27 Feb 2023 17:01:39 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v3 20/20] thermal/traces: Replace the thermal zone
 structure parameter with the field value
Content-Language: en-US
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     rafael@kernel.org, linux-pm@vger.kernel.org,
        linux-kernel@vger.kernel.org, Amit Kucheria <amitk@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>,
        Lukasz Luba <lukasz.luba@arm.com>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        "open list:TRACING" <linux-trace-kernel@vger.kernel.org>
References: <20230226225406.979703-1-daniel.lezcano@linaro.org>
 <20230226225406.979703-21-daniel.lezcano@linaro.org>
 <20230227100715.7d896836@gandalf.local.home>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <20230227100715.7d896836@gandalf.local.home>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


Hi Steven,


On 27/02/2023 16:07, Steven Rostedt wrote:
> On Sun, 26 Feb 2023 23:54:06 +0100
> Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> 
>> In the work of the thermal zone device self-encapsulation, let's pass
>> the field value instead of dereferencing them in the traces which
>> force us to export publicly the thermal_zone_device structure.
>>
>> No fonctionnal change intended.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>   drivers/thermal/gov_fair_share.c              |  4 +++-
>>   drivers/thermal/gov_power_allocator.c         |  6 +++--
>>   drivers/thermal/gov_step_wise.c               |  4 +++-
>>   drivers/thermal/thermal_core.c                |  8 +++++--
>>   include/trace/events/thermal.h                | 24 +++++++++----------
>>   .../trace/events/thermal_power_allocator.h    | 12 +++++-----
>>   6 files changed, 34 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/thermal/gov_fair_share.c b/drivers/thermal/gov_fair_share.c
>> index aad7d5fe3a14..cdddd593021d 100644
>> --- a/drivers/thermal/gov_fair_share.c
>> +++ b/drivers/thermal/gov_fair_share.c
>> @@ -35,7 +35,9 @@ static int get_trip_level(struct thermal_zone_device *tz)
>>   	 * point, in which case, trip_point = count - 1
>>   	 */
>>   	if (count > 0)
>> -		trace_thermal_zone_trip(tz, count - 1, trip.type);
>> +		trace_thermal_zone_trip(thermal_zone_device_type(tz),
>> +					thermal_zone_device_id(tz),
>> +					count - 1, trip.type);
> 
> The problem with this approach is that you are moving all the work to
> dereference the pointers into the hot paths (the code execution), instead
> of doing it in the slow path (where the tracepoint work is done).

Good point, that is something I did not realize, thanks for pointing it out.

> If you are concerned with exporting a structure then move the trace file
> from:
>
>    include/trace/events/thermal.h to drivers/thermal/trace.h
> 
> like drivers/vfio/pci/trace.h and many other drivers do.

Good idea, I'll do that

> Read
>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/trace_events/Makefile
> to see how to use a trace header outside the include/trace/events directory.
> 
> also, by removing the pointer, you lose out on BPF and kprobe traces that
> could dereference the pointer if you needed to trace something that was not
> exported by the trace point itself.

As the trace will be in the drivers/thermal/trace.h, it will be able to 
use the thermal_core.h private file and no longer 
include/linux/thermal.h, so preventing to unexport the thermal zone 
structure from thermal.h. Consequently, we no longer have to change the 
prototype of the traces and the pointer will stay in place.

Thanks for your suggestions


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog