linux-kernel - Re: [PATCH v3 20/20] thermal/traces: Replace the thermal zone structure parameter with the field value

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 27 Feb 2023 10:07:15 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     rafael@...nel.org, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Amit Kucheria <amitk@...nel.org>,
        Zhang Rui <rui.zhang@...el.com>,
        Lukasz Luba <lukasz.luba@....com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        linux-trace-kernel@...r.kernel.org (open list:TRACING)
Subject: Re: [PATCH v3 20/20] thermal/traces: Replace the thermal zone
 structure parameter with the field value

On Sun, 26 Feb 2023 23:54:06 +0100
Daniel Lezcano <daniel.lezcano@...aro.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@...aro.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).

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.

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.

-- Steve


>  
>  	return count;
>  }