If the perf buffer isn't large enough, provide a hint about how large it
needs to be for whatever is running.
Signed-off-by: Robin H. Johnson <[email protected]>
---
kernel/trace/trace_event_perf.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 03be4435d103..26eed4b89100 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -400,7 +400,8 @@ void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp)
BUILD_BUG_ON(PERF_MAX_TRACE_SIZE % sizeof(unsigned long));
if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE,
- "perf buffer not large enough"))
+ "perf buffer not large enough, wanted %d, have %d",
+ size, PERF_MAX_TRACE_SIZE))
return NULL;
*rctxp = rctx = perf_swevent_get_recursion_context();
--
2.33.0
I'll need Acks for these patches from the Perf maintainers.
-- Steve
On Mon, 30 Aug 2021 21:37:22 -0700
"Robin H. Johnson" <[email protected]> wrote:
> If the perf buffer isn't large enough, provide a hint about how large it
> needs to be for whatever is running.
>
> Signed-off-by: Robin H. Johnson <[email protected]>
> ---
> kernel/trace/trace_event_perf.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index 03be4435d103..26eed4b89100 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -400,7 +400,8 @@ void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp)
> BUILD_BUG_ON(PERF_MAX_TRACE_SIZE % sizeof(unsigned long));
>
> if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE,
> - "perf buffer not large enough"))
> + "perf buffer not large enough, wanted %d, have %d",
> + size, PERF_MAX_TRACE_SIZE))
> return NULL;
>
> *rctxp = rctx = perf_swevent_get_recursion_context();
On Mon, 30 Aug 2021 21:37:22 -0700
"Robin H. Johnson" <[email protected]> wrote:
Sorry for the late reply, I was on holiday when this was sent, and I'm just
getting to looking at this email now (as my OoO email should have suggested ;-)
Anyway, this needs to be reviewed by the Perf maintainers (Cc'd)
(Lore link for patch 2:
https://lore.kernel.org/all/[email protected]/ )
-- Steve
> If the perf buffer isn't large enough, provide a hint about how large it
> needs to be for whatever is running.
>
> Signed-off-by: Robin H. Johnson <[email protected]>
> ---
> kernel/trace/trace_event_perf.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> index 03be4435d103..26eed4b89100 100644
> --- a/kernel/trace/trace_event_perf.c
> +++ b/kernel/trace/trace_event_perf.c
> @@ -400,7 +400,8 @@ void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp)
> BUILD_BUG_ON(PERF_MAX_TRACE_SIZE % sizeof(unsigned long));
>
> if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE,
> - "perf buffer not large enough"))
> + "perf buffer not large enough, wanted %d, have %d",
> + size, PERF_MAX_TRACE_SIZE))
> return NULL;
>
> *rctxp = rctx = perf_swevent_get_recursion_context();
On Wed, Oct 06, 2021 at 06:26:52PM -0400, Steven Rostedt wrote:
> On Mon, 30 Aug 2021 21:37:22 -0700
> "Robin H. Johnson" <[email protected]> wrote:
>
> Sorry for the late reply, I was on holiday when this was sent, and I'm just
> getting to looking at this email now (as my OoO email should have suggested ;-)
>
> Anyway, this needs to be reviewed by the Perf maintainers (Cc'd)
>
> (Lore link for patch 2:
> https://lore.kernel.org/all/[email protected]/ )
You already CC'd them on Sept 7th, no response yet.
Does MAINTAINERS need an update for kernel/trace/trace_event_perf.c?
It points to Ingo & yourself for that directory, and not to the Perf
maintainers.
--
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail : [email protected]
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136
On Wed, 6 Oct 2021 22:48:35 +0000
"Robin H. Johnson" <[email protected]> wrote:
> On Wed, Oct 06, 2021 at 06:26:52PM -0400, Steven Rostedt wrote:
> > On Mon, 30 Aug 2021 21:37:22 -0700
> > "Robin H. Johnson" <[email protected]> wrote:
> >
> > Sorry for the late reply, I was on holiday when this was sent, and I'm just
> > getting to looking at this email now (as my OoO email should have suggested ;-)
> >
> > Anyway, this needs to be reviewed by the Perf maintainers (Cc'd)
> >
> > (Lore link for patch 2:
> > https://lore.kernel.org/all/[email protected]/ )
>
> You already CC'd them on Sept 7th, no response yet.
Oh good, that means I'm not the one that dropped the ball on this ;-)
>
> Does MAINTAINERS need an update for kernel/trace/trace_event_perf.c?
> It points to Ingo & yourself for that directory, and not to the Perf
> maintainers.
Well, it is maintained by both of us. I usually just ask them to review
before taking it, but if there's no response from them by the end of
the week, I'll add it to my "for-next" queue.
-- Steve
On Tue, Sep 07, 2021 at 09:24:26PM -0400, Steven Rostedt wrote:
>
> I'll need Acks for these patches from the Perf maintainers.
>
> -- Steve
>
>
> On Mon, 30 Aug 2021 21:37:22 -0700
> "Robin H. Johnson" <[email protected]> wrote:
>
> > If the perf buffer isn't large enough, provide a hint about how large it
> > needs to be for whatever is running.
Is that an actual reason?
> > Signed-off-by: Robin H. Johnson <[email protected]>
> > ---
> > kernel/trace/trace_event_perf.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
> > index 03be4435d103..26eed4b89100 100644
> > --- a/kernel/trace/trace_event_perf.c
> > +++ b/kernel/trace/trace_event_perf.c
> > @@ -400,7 +400,8 @@ void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp)
> > BUILD_BUG_ON(PERF_MAX_TRACE_SIZE % sizeof(unsigned long));
> >
> > if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE,
> > - "perf buffer not large enough"))
> > + "perf buffer not large enough, wanted %d, have %d",
> > + size, PERF_MAX_TRACE_SIZE))
Priting a constant seems daft.. why is any of this important in any way?
On Thu, 7 Oct 2021 09:11:51 +0200
Peter Zijlstra <[email protected]> wrote:
> > > +++ b/kernel/trace/trace_event_perf.c
> > > @@ -400,7 +400,8 @@ void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp)
> > > BUILD_BUG_ON(PERF_MAX_TRACE_SIZE % sizeof(unsigned long));
> > >
> > > if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE,
> > > - "perf buffer not large enough"))
> > > + "perf buffer not large enough, wanted %d, have %d",
> > > + size, PERF_MAX_TRACE_SIZE))
>
> Priting a constant seems daft.. why is any of this important in any way?
I see your point, but it can be useful if you changed it, and want to know
if you are running the kernel with the change or not.
I've done daft things were I changed a const and was running a kernel
without the change and couldn't understand why it wasn't working ;-)
-- Steve
On Thu, Oct 07, 2021 at 09:23:58AM -0400, Steven Rostedt wrote:
> On Thu, 7 Oct 2021 09:11:51 +0200
> Peter Zijlstra <[email protected]> wrote:
>
> > > > +++ b/kernel/trace/trace_event_perf.c
> > > > @@ -400,7 +400,8 @@ void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp)
> > > > BUILD_BUG_ON(PERF_MAX_TRACE_SIZE % sizeof(unsigned long));
> > > >
> > > > if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE,
> > > > - "perf buffer not large enough"))
> > > > + "perf buffer not large enough, wanted %d, have %d",
> > > > + size, PERF_MAX_TRACE_SIZE))
> >
> > Priting a constant seems daft.. why is any of this important in any way?
>
> I see your point, but it can be useful if you changed it, and want to know
> if you are running the kernel with the change or not.
>
> I've done daft things were I changed a const and was running a kernel
> without the change and couldn't understand why it wasn't working ;-)
Yes, my initial internal versions of this series only bumped the
constant, and then I was running a few different kernels, and being able
to compare which value was in use became a problem.
I was trying to think further what would make sense for the constant.
- What are the negative impacts of a too-large value?
- Is there demand for more reconfigurability?
- Should PERF_MAX_TRACE_SIZE be a knob in Kconfig?
--
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail : [email protected]
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136
On Thu, 7 Oct 2021 17:26:04 +0000
"Robin H. Johnson" <[email protected]> wrote:
> I was trying to think further what would make sense for the constant.
> - What are the negative impacts of a too-large value?
> - Is there demand for more reconfigurability?
> - Should PERF_MAX_TRACE_SIZE be a knob in Kconfig?
One thing you haven't discussed was, have you hit this warning, and if so,
what were you doing?
-- Steve
On Fri, Oct 08, 2021 at 06:13:48PM -0400, Steven Rostedt wrote:
> On Thu, 7 Oct 2021 17:26:04 +0000
> "Robin H. Johnson" <[email protected]> wrote:
>
> > I was trying to think further what would make sense for the constant.
> > - What are the negative impacts of a too-large value?
> > - Is there demand for more reconfigurability?
> > - Should PERF_MAX_TRACE_SIZE be a knob in Kconfig?
>
> One thing you haven't discussed was, have you hit this warning, and if so,
> what were you doing?
Ah, I covered that in patch 2/2 in the original series, which discussed
why I was raising the limit, and how I came to the 8K value for
PERF_MAX_TRACE_SIZE.
https://lore.kernel.org/all/[email protected]/
To summarize here:
$work requires that all employees run endpoint security software from
SentinalOne [1]. I can see that it uses trace/perf stuff to dig deeply
into what's taking place on the system.
Something in my personal setup/configuration, leads to SentinelOne
getting large perf backlogs during heavy workloads, primarily when I'm
doing deep things with containers (esp with tun devices inside
containers). I haven't been able to narrow it down much more than that,
and I don't have the source to SentinelOne at all.
It does feel like SentinelOne either has a leak of some sort, or gets a
backlog of work that takes a noticible amount of time to clean up.
[1] https://www.sentinelone.com/
--
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail : [email protected]
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136
On Thu, 7 Oct 2021 09:23:58 -0400
Steven Rostedt <[email protected]> wrote:
> On Thu, 7 Oct 2021 09:11:51 +0200
> Peter Zijlstra <[email protected]> wrote:
>
> > > > +++ b/kernel/trace/trace_event_perf.c
> > > > @@ -400,7 +400,8 @@ void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp)
> > > > BUILD_BUG_ON(PERF_MAX_TRACE_SIZE % sizeof(unsigned long));
> > > >
> > > > if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE,
> > > > - "perf buffer not large enough"))
> > > > + "perf buffer not large enough, wanted %d, have %d",
> > > > + size, PERF_MAX_TRACE_SIZE))
> >
> > Priting a constant seems daft.. why is any of this important in any way?
>
> I see your point, but it can be useful if you changed it, and want to know
> if you are running the kernel with the change or not.
>
> I've done daft things were I changed a const and was running a kernel
> without the change and couldn't understand why it wasn't working ;-)
Peter,
Do you have any real issue if I just take this patch set through my tree?
-- Steve
On Tue, Oct 26, 2021 at 04:43:43PM -0400, Steven Rostedt wrote:
> On Thu, 7 Oct 2021 09:23:58 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > On Thu, 7 Oct 2021 09:11:51 +0200
> > Peter Zijlstra <[email protected]> wrote:
> >
> > > > > +++ b/kernel/trace/trace_event_perf.c
> > > > > @@ -400,7 +400,8 @@ void *perf_trace_buf_alloc(int size, struct pt_regs **regs, int *rctxp)
> > > > > BUILD_BUG_ON(PERF_MAX_TRACE_SIZE % sizeof(unsigned long));
> > > > >
> > > > > if (WARN_ONCE(size > PERF_MAX_TRACE_SIZE,
> > > > > - "perf buffer not large enough"))
> > > > > + "perf buffer not large enough, wanted %d, have %d",
> > > > > + size, PERF_MAX_TRACE_SIZE))
> > >
> > > Priting a constant seems daft.. why is any of this important in any way?
> >
> > I see your point, but it can be useful if you changed it, and want to know
> > if you are running the kernel with the change or not.
> >
> > I've done daft things were I changed a const and was running a kernel
> > without the change and couldn't understand why it wasn't working ;-)
>
> Peter,
>
> Do you have any real issue if I just take this patch set through my tree?
No real objections; just weary, huge events like that are fairly sucky
for performance.
On Tue, 26 Oct 2021 23:23:32 +0200
Peter Zijlstra <[email protected]> wrote:
> > Do you have any real issue if I just take this patch set through my tree?
>
> No real objections; just weary, huge events like that are fairly sucky
> for performance.
OK. Then I'll just take this in, and let people have sucky performance if
they want huge events ;-)
-- Steve