[PATCH] sched/core: Export pelt_thermal_tp

[PATCH] sched/core: Export pelt_thermal_tp

[Qais Yousef]

We can't use this tracepoint in modules without having the symbol
exported first, fix that.

Fixes: 765047932f15 ("sched/pelt: Add support to track thermal pressure")
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
---
 kernel/sched/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index f2611b9cf503..2f7a1d8f5f17 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -36,6 +36,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_rt_tp);
 EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_dl_tp);
 EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_irq_tp);
 EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_se_tp);
+EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_thermal_tp);
 EXPORT_TRACEPOINT_SYMBOL_GPL(sched_cpu_capacity_tp);
 EXPORT_TRACEPOINT_SYMBOL_GPL(sched_overutilized_tp);
 EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_cfs_tp);
-- 
2.25.1

Re: [PATCH] sched/core: Export pelt_thermal_tp

[Christoph Hellwig]

On Thu, Oct 28, 2021 at 12:50:05PM +0100, Qais Yousef wrote:
> We can't use this tracepoint in modules without having the symbol
> exported first, fix that.

Which modules is using this?  In linux-next there does not seems to be
any user outside of kernel/sched/pelt.c.

> @@ -36,6 +36,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_rt_tp);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_dl_tp);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_irq_tp);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_se_tp);
> +EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_thermal_tp);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_cpu_capacity_tp);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_overutilized_tp);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_cfs_tp);

... and while we're at it, all these exports are unused and should
be deleted as well.

Re: [PATCH] sched/core: Export pelt_thermal_tp

[Peter Zijlstra]

On Thu, Oct 28, 2021 at 09:00:56AM -0700, Christoph Hellwig wrote:
> On Thu, Oct 28, 2021 at 12:50:05PM +0100, Qais Yousef wrote:
> > We can't use this tracepoint in modules without having the symbol
> > exported first, fix that.
> 
> Which modules is using this?  In linux-next there does not seems to be
> any user outside of kernel/sched/pelt.c.
> 
> > @@ -36,6 +36,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_rt_tp);
> >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_dl_tp);
> >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_irq_tp);
> >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_se_tp);
> > +EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_thermal_tp);
> >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_cpu_capacity_tp);
> >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_overutilized_tp);
> >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_cfs_tp);
> 
> ... and while we're at it, all these exports are unused and should
> be deleted as well.

This is my concession wrt tracepoints. Actual tracepoints are ABI,
exports are in-kernel interfaces and are explicitly not ABI.

This way people can use an external module to get at the tracepoint data
without having in-tree tracepoints.

Re: [PATCH] sched/core: Export pelt_thermal_tp

[Christoph Hellwig]

On Thu, Oct 28, 2021 at 06:18:55PM +0200, Peter Zijlstra wrote:
> > > @@ -36,6 +36,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_rt_tp);
> > >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_dl_tp);
> > >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_irq_tp);
> > >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_se_tp);
> > > +EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_thermal_tp);
> > >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_cpu_capacity_tp);
> > >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_overutilized_tp);
> > >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_cfs_tp);
> > 
> > ... and while we're at it, all these exports are unused and should
> > be deleted as well.
> 
> This is my concession wrt tracepoints. Actual tracepoints are ABI,
> exports are in-kernel interfaces and are explicitly not ABI.
> 
> This way people can use an external module to get at the tracepoint data
> without having in-tree tracepoints.

All of this makes no sense at all.  These are entirely dead exports.
If you remove them nothing else changes.  Note taht the tracepoints
do have in-kernel callers, so if people thing of them as an ABI you've
got your ABI already with or without the exports.

Re: [PATCH] sched/core: Export pelt_thermal_tp

[Phil Auld]

On Thu, Oct 28, 2021 at 09:22:05AM -0700 Christoph Hellwig wrote:
> On Thu, Oct 28, 2021 at 06:18:55PM +0200, Peter Zijlstra wrote:
> > > > @@ -36,6 +36,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_rt_tp);
> > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_dl_tp);
> > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_irq_tp);
> > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_se_tp);
> > > > +EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_thermal_tp);
> > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_cpu_capacity_tp);
> > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_overutilized_tp);
> > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_cfs_tp);
> > > 
> > > ... and while we're at it, all these exports are unused and should
> > > be deleted as well.
> > 
> > This is my concession wrt tracepoints. Actual tracepoints are ABI,
> > exports are in-kernel interfaces and are explicitly not ABI.
> > 
> > This way people can use an external module to get at the tracepoint data
> > without having in-tree tracepoints.
> 
> All of this makes no sense at all.  These are entirely dead exports.
> If you remove them nothing else changes.  Note taht the tracepoints
> do have in-kernel callers, so if people thing of them as an ABI you've
> got your ABI already with or without the exports.
> 

Full blown trace _events_ create an ABI. These trace points are not ABI.
But by exporting them they are accesible to little helper modules which
can turn them into trace events which can then by used by trace-cmd and
ftrace etc.  That way we can have the tracepoints at the interesting spots
in the code but still have control over them with respect to changes. 

See https://github.com/auldp/tracepoints-helpers for an example. 

Cheers,
Phil

-- 

Re: [PATCH] sched/core: Export pelt_thermal_tp

[Peter Zijlstra]

On Thu, Oct 28, 2021 at 09:22:05AM -0700, Christoph Hellwig wrote:
> On Thu, Oct 28, 2021 at 06:18:55PM +0200, Peter Zijlstra wrote:
> > > > @@ -36,6 +36,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_rt_tp);
> > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_dl_tp);
> > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_irq_tp);
> > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_se_tp);
> > > > +EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_thermal_tp);
> > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_cpu_capacity_tp);
> > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_overutilized_tp);
> > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_cfs_tp);
> > > 
> > > ... and while we're at it, all these exports are unused and should
> > > be deleted as well.
> > 
> > This is my concession wrt tracepoints. Actual tracepoints are ABI,
> > exports are in-kernel interfaces and are explicitly not ABI.
> > 
> > This way people can use an external module to get at the tracepoint data
> > without having in-tree tracepoints.
> 
> All of this makes no sense at all.  These are entirely dead exports.
> If you remove them nothing else changes.  Note taht the tracepoints
> do have in-kernel callers, so if people thing of them as an ABI you've
> got your ABI already with or without the exports.

These are not normal traceevents, these are tracepoints, the distinction
is that these things do not show up in tracefs and there is no userspace
visible representation of them. No userspace gives no ABI.

All they provide is the in-code hook and in-kernel registration
interface. These EXPORTS export that registration interface, such that
an out-of-tree module can make use of them.

And yes, unused exports are iffy, out-of-tree modules are iffy, but in
this case I made an exception since ABI contraints are worse. We very
clearly state there is no such thing is kabi, so breaking any user of
these exports if fair game.

Breaking users of userspace trace-events gets kernel patches reverted
(been there, done that, never want to ever be there again).

People want to trace this stuff, but I *REALLY* do not want to commit to
ABI, this is the middle-ground that sucks least :/

Re: [PATCH] sched/core: Export pelt_thermal_tp

[Qais Yousef]

Hi Peter

On 10/28/21 18:50, Peter Zijlstra wrote:
> On Thu, Oct 28, 2021 at 09:22:05AM -0700, Christoph Hellwig wrote:
> > On Thu, Oct 28, 2021 at 06:18:55PM +0200, Peter Zijlstra wrote:
> > > > > @@ -36,6 +36,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_rt_tp);
> > > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_dl_tp);
> > > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_irq_tp);
> > > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_se_tp);
> > > > > +EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_thermal_tp);
> > > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_cpu_capacity_tp);
> > > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_overutilized_tp);
> > > > >  EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_cfs_tp);
> > > > 
> > > > ... and while we're at it, all these exports are unused and should
> > > > be deleted as well.
> > > 
> > > This is my concession wrt tracepoints. Actual tracepoints are ABI,
> > > exports are in-kernel interfaces and are explicitly not ABI.
> > > 
> > > This way people can use an external module to get at the tracepoint data
> > > without having in-tree tracepoints.
> > 
> > All of this makes no sense at all.  These are entirely dead exports.
> > If you remove them nothing else changes.  Note taht the tracepoints
> > do have in-kernel callers, so if people thing of them as an ABI you've
> > got your ABI already with or without the exports.
> 
> These are not normal traceevents, these are tracepoints, the distinction
> is that these things do not show up in tracefs and there is no userspace
> visible representation of them. No userspace gives no ABI.
> 
> All they provide is the in-code hook and in-kernel registration
> interface. These EXPORTS export that registration interface, such that
> an out-of-tree module can make use of them.
> 
> And yes, unused exports are iffy, out-of-tree modules are iffy, but in
> this case I made an exception since ABI contraints are worse. We very
> clearly state there is no such thing is kabi, so breaking any user of
> these exports if fair game.
> 
> Breaking users of userspace trace-events gets kernel patches reverted
> (been there, done that, never want to ever be there again).
> 
> People want to trace this stuff, but I *REALLY* do not want to commit to
> ABI, this is the middle-ground that sucks least :/

AFAICS this wasn't picked up. Should I tweak the commit message to make things
clearer?

Thanks!

--
Qais Yousef

[tip: sched/core] sched/core: Export pelt_thermal_tp

[tip-bot2 for Qais Yousef]

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     77cf151b7bbdfa3577b3c3f3a5e267a6c60a263b
Gitweb:        https://git.kernel.org/tip/77cf151b7bbdfa3577b3c3f3a5e267a6c60a263b
Author:        Qais Yousef <qais.yousef@arm.com>
AuthorDate:    Thu, 28 Oct 2021 12:50:05 +01:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 27 Jan 2022 12:57:18 +01:00

sched/core: Export pelt_thermal_tp

We can't use this tracepoint in modules without having the symbol
exported first, fix that.

Fixes: 765047932f15 ("sched/pelt: Add support to track thermal pressure")
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20211028115005.873539-1-qais.yousef@arm.com
---
 kernel/sched/core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2e4ae00..1d863d7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -36,6 +36,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_rt_tp);
 EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_dl_tp);
 EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_irq_tp);
 EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_se_tp);
+EXPORT_TRACEPOINT_SYMBOL_GPL(pelt_thermal_tp);
 EXPORT_TRACEPOINT_SYMBOL_GPL(sched_cpu_capacity_tp);
 EXPORT_TRACEPOINT_SYMBOL_GPL(sched_overutilized_tp);
 EXPORT_TRACEPOINT_SYMBOL_GPL(sched_util_est_cfs_tp);