* [RFC PATCH v1 18/57] trace: Remove PAGE_SIZE compile-time constant assumption [not found] ` <20241014105912.3207374-1-ryan.roberts@arm.com> @ 2024-10-14 10:58 ` Ryan Roberts 2024-10-14 16:46 ` Steven Rostedt 0 siblings, 1 reply; 4+ messages in thread From: Ryan Roberts @ 2024-10-14 10:58 UTC (permalink / raw) To: Andrew Morton, Anshuman Khandual, Ard Biesheuvel, Catalin Marinas, David Hildenbrand, Greg Marsden, Ivan Ivanov, Kalesh Singh, Marc Zyngier, Mark Rutland, Masami Hiramatsu, Matthias Brugger, Miroslav Benes, Steven Rostedt, Will Deacon Cc: Ryan Roberts, linux-arm-kernel, linux-kernel, linux-mm, linux-trace-kernel To prepare for supporting boot-time page size selection, refactor code to remove assumptions about PAGE_SIZE being compile-time constant. Code intended to be equivalent when compile-time page size is active. Convert BUILD_BUG_ON() BUG_ON() since the argument depends on PAGE_SIZE and its not trivial to test against a page size limit. Redefine FTRACE_KSTACK_ENTRIES so that "struct ftrace_stacks" is always sized at 32K for 64-bit and 16K for 32-bit. It was previously defined in terms of PAGE_SIZE (and worked out at the quoted sizes for a 4K page size). But for 64K pages, the size expanded to 512K. Given the ftrace stacks should be invariant to page size, this seemed like a waste. As a side effect, it removes the PAGE_SIZE compile-time constant assumption from this code. Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> --- ***NOTE*** Any confused maintainers may want to read the cover note here for context: https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/ kernel/trace/fgraph.c | 2 +- kernel/trace/trace.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c index d7d4fb403f6f0..47aa5c8d8090e 100644 --- a/kernel/trace/fgraph.c +++ b/kernel/trace/fgraph.c @@ -534,7 +534,7 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, if (!current->ret_stack) return -EBUSY; - BUILD_BUG_ON(SHADOW_STACK_SIZE % sizeof(long)); + BUG_ON(SHADOW_STACK_SIZE % sizeof(long)); /* Set val to "reserved" with the delta to the new fgraph frame */ val = (FGRAPH_TYPE_RESERVED << FGRAPH_TYPE_SHIFT) | FGRAPH_FRAME_OFFSET; diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index c3b2c7dfadef1..0f2ec3d30579f 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -2887,7 +2887,7 @@ trace_function(struct trace_array *tr, unsigned long ip, unsigned long /* Allow 4 levels of nesting: normal, softirq, irq, NMI */ #define FTRACE_KSTACK_NESTING 4 -#define FTRACE_KSTACK_ENTRIES (PAGE_SIZE / FTRACE_KSTACK_NESTING) +#define FTRACE_KSTACK_ENTRIES (SZ_4K / FTRACE_KSTACK_NESTING) struct ftrace_stack { unsigned long calls[FTRACE_KSTACK_ENTRIES]; -- 2.43.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH v1 18/57] trace: Remove PAGE_SIZE compile-time constant assumption 2024-10-14 10:58 ` [RFC PATCH v1 18/57] trace: Remove PAGE_SIZE compile-time constant assumption Ryan Roberts @ 2024-10-14 16:46 ` Steven Rostedt 2024-10-15 11:09 ` Ryan Roberts 0 siblings, 1 reply; 4+ messages in thread From: Steven Rostedt @ 2024-10-14 16:46 UTC (permalink / raw) To: Ryan Roberts Cc: Andrew Morton, Anshuman Khandual, Ard Biesheuvel, Catalin Marinas, David Hildenbrand, Greg Marsden, Ivan Ivanov, Kalesh Singh, Marc Zyngier, Mark Rutland, Masami Hiramatsu, Matthias Brugger, Miroslav Benes, Will Deacon, linux-arm-kernel, linux-kernel, linux-mm, linux-trace-kernel On Mon, 14 Oct 2024 11:58:25 +0100 Ryan Roberts <ryan.roberts@arm.com> wrote: > To prepare for supporting boot-time page size selection, refactor code > to remove assumptions about PAGE_SIZE being compile-time constant. Code > intended to be equivalent when compile-time page size is active. > > Convert BUILD_BUG_ON() BUG_ON() since the argument depends on PAGE_SIZE > and its not trivial to test against a page size limit. > > Redefine FTRACE_KSTACK_ENTRIES so that "struct ftrace_stacks" is always > sized at 32K for 64-bit and 16K for 32-bit. It was previously defined in > terms of PAGE_SIZE (and worked out at the quoted sizes for a 4K page > size). But for 64K pages, the size expanded to 512K. Given the ftrace > stacks should be invariant to page size, this seemed like a waste. As a > side effect, it removes the PAGE_SIZE compile-time constant assumption > from this code. > > Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> > --- > > ***NOTE*** > Any confused maintainers may want to read the cover note here for context: > https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/ > > kernel/trace/fgraph.c | 2 +- > kernel/trace/trace.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c > index d7d4fb403f6f0..47aa5c8d8090e 100644 > --- a/kernel/trace/fgraph.c > +++ b/kernel/trace/fgraph.c > @@ -534,7 +534,7 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, > if (!current->ret_stack) > return -EBUSY; > > - BUILD_BUG_ON(SHADOW_STACK_SIZE % sizeof(long)); > + BUG_ON(SHADOW_STACK_SIZE % sizeof(long)); Absolutely not! BUG_ON() is in no way a substitution of any BUILD_BUG_ON(). BUILD_BUG_ON() is a non intrusive way to see if something isn't lined up correctly, and can fix it before you execute any code. BUG_ON() is the most intrusive way to say something is wrong and you crash the system. Not to mention, when function graph tracing is enabled, this gets triggered for *every* function call! So I do not want any runtime test done. Every nanosecond counts in this code path. If anything, this needs to be moved to initialization and checked once, if it fails, gives a WARN_ON() and disables function graph tracing. -- Steve > > /* Set val to "reserved" with the delta to the new fgraph frame */ > val = (FGRAPH_TYPE_RESERVED << FGRAPH_TYPE_SHIFT) | FGRAPH_FRAME_OFFSET; > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index c3b2c7dfadef1..0f2ec3d30579f 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -2887,7 +2887,7 @@ trace_function(struct trace_array *tr, unsigned long ip, unsigned long > /* Allow 4 levels of nesting: normal, softirq, irq, NMI */ > #define FTRACE_KSTACK_NESTING 4 > > -#define FTRACE_KSTACK_ENTRIES (PAGE_SIZE / FTRACE_KSTACK_NESTING) > +#define FTRACE_KSTACK_ENTRIES (SZ_4K / FTRACE_KSTACK_NESTING) > > struct ftrace_stack { > unsigned long calls[FTRACE_KSTACK_ENTRIES]; ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH v1 18/57] trace: Remove PAGE_SIZE compile-time constant assumption 2024-10-14 16:46 ` Steven Rostedt @ 2024-10-15 11:09 ` Ryan Roberts 2024-10-18 15:24 ` Steven Rostedt 0 siblings, 1 reply; 4+ messages in thread From: Ryan Roberts @ 2024-10-15 11:09 UTC (permalink / raw) To: Steven Rostedt Cc: Andrew Morton, Anshuman Khandual, Ard Biesheuvel, Catalin Marinas, David Hildenbrand, Greg Marsden, Ivan Ivanov, Kalesh Singh, Marc Zyngier, Mark Rutland, Masami Hiramatsu, Matthias Brugger, Miroslav Benes, Will Deacon, linux-arm-kernel, linux-kernel, linux-mm, linux-trace-kernel On 14/10/2024 17:46, Steven Rostedt wrote: > On Mon, 14 Oct 2024 11:58:25 +0100 > Ryan Roberts <ryan.roberts@arm.com> wrote: > >> To prepare for supporting boot-time page size selection, refactor code >> to remove assumptions about PAGE_SIZE being compile-time constant. Code >> intended to be equivalent when compile-time page size is active. >> >> Convert BUILD_BUG_ON() BUG_ON() since the argument depends on PAGE_SIZE >> and its not trivial to test against a page size limit. >> >> Redefine FTRACE_KSTACK_ENTRIES so that "struct ftrace_stacks" is always >> sized at 32K for 64-bit and 16K for 32-bit. It was previously defined in >> terms of PAGE_SIZE (and worked out at the quoted sizes for a 4K page >> size). But for 64K pages, the size expanded to 512K. Given the ftrace >> stacks should be invariant to page size, this seemed like a waste. As a >> side effect, it removes the PAGE_SIZE compile-time constant assumption >> from this code. >> >> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com> >> --- >> >> ***NOTE*** >> Any confused maintainers may want to read the cover note here for context: >> https://lore.kernel.org/all/20241014105514.3206191-1-ryan.roberts@arm.com/ >> >> kernel/trace/fgraph.c | 2 +- >> kernel/trace/trace.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c >> index d7d4fb403f6f0..47aa5c8d8090e 100644 >> --- a/kernel/trace/fgraph.c >> +++ b/kernel/trace/fgraph.c >> @@ -534,7 +534,7 @@ ftrace_push_return_trace(unsigned long ret, unsigned long func, >> if (!current->ret_stack) >> return -EBUSY; >> >> - BUILD_BUG_ON(SHADOW_STACK_SIZE % sizeof(long)); >> + BUG_ON(SHADOW_STACK_SIZE % sizeof(long)); > > Absolutely not! > > BUG_ON() is in no way a substitution of any BUILD_BUG_ON(). BUILD_BUG_ON() > is a non intrusive way to see if something isn't lined up correctly, and > can fix it before you execute any code. BUG_ON() is the most intrusive way > to say something is wrong and you crash the system. Yep, totally agree. I'm afraid this was me being lazy, and there are a couple of other instances where I have done this in other patches that I'll need to fix. Most of the time, I've been able to keep BUILD_BUG_ON() and simply compare against a page size limit. Looking at this again, perhaps the better solution is to define SHADOW_STACK_SIZE as PAGE_SIZE_MIN? Then it remains a compile-time constant. Is there any need for SHADOW_STACK_SIZE to increase with page size? > > Not to mention, when function graph tracing is enabled, this gets triggered > for *every* function call! So I do not want any runtime test done. Every > nanosecond counts in this code path. > > If anything, this needs to be moved to initialization and checked once, if > it fails, gives a WARN_ON() and disables function graph tracing. I'm hoping my suggestion above to decouple SHADOW_STACK_SIZE from PAGE_SIZE is acceptable and simpler? If not, happy to do as you suggest here. Thanks, Ryan > > -- Steve > > >> >> /* Set val to "reserved" with the delta to the new fgraph frame */ >> val = (FGRAPH_TYPE_RESERVED << FGRAPH_TYPE_SHIFT) | FGRAPH_FRAME_OFFSET; >> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c >> index c3b2c7dfadef1..0f2ec3d30579f 100644 >> --- a/kernel/trace/trace.c >> +++ b/kernel/trace/trace.c >> @@ -2887,7 +2887,7 @@ trace_function(struct trace_array *tr, unsigned long ip, unsigned long >> /* Allow 4 levels of nesting: normal, softirq, irq, NMI */ >> #define FTRACE_KSTACK_NESTING 4 >> >> -#define FTRACE_KSTACK_ENTRIES (PAGE_SIZE / FTRACE_KSTACK_NESTING) >> +#define FTRACE_KSTACK_ENTRIES (SZ_4K / FTRACE_KSTACK_NESTING) >> >> struct ftrace_stack { >> unsigned long calls[FTRACE_KSTACK_ENTRIES]; > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH v1 18/57] trace: Remove PAGE_SIZE compile-time constant assumption 2024-10-15 11:09 ` Ryan Roberts @ 2024-10-18 15:24 ` Steven Rostedt 0 siblings, 0 replies; 4+ messages in thread From: Steven Rostedt @ 2024-10-18 15:24 UTC (permalink / raw) To: Ryan Roberts Cc: Andrew Morton, Anshuman Khandual, Ard Biesheuvel, Catalin Marinas, David Hildenbrand, Greg Marsden, Ivan Ivanov, Kalesh Singh, Marc Zyngier, Mark Rutland, Masami Hiramatsu, Matthias Brugger, Miroslav Benes, Will Deacon, linux-arm-kernel, linux-kernel, linux-mm, linux-trace-kernel On Tue, 15 Oct 2024 12:09:38 +0100 Ryan Roberts <ryan.roberts@arm.com> wrote: > > > > Not to mention, when function graph tracing is enabled, this gets triggered > > for *every* function call! So I do not want any runtime test done. Every > > nanosecond counts in this code path. > > > > If anything, this needs to be moved to initialization and checked once, if > > it fails, gives a WARN_ON() and disables function graph tracing. > > I'm hoping my suggestion above to decouple SHADOW_STACK_SIZE from PAGE_SIZE is > acceptable and simpler? If not, happy to do as you suggest here. Yeah, I think we can do that. In fact, I'm thinking it should turn into a kmem_cache item that doesn't have to be a power of two (but must be evenly divisible by the size of long). I'll write up a patch. -- Steve ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-10-18 15:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20241014105514.3206191-1-ryan.roberts@arm.com>
[not found] ` <20241014105912.3207374-1-ryan.roberts@arm.com>
2024-10-14 10:58 ` [RFC PATCH v1 18/57] trace: Remove PAGE_SIZE compile-time constant assumption Ryan Roberts
2024-10-14 16:46 ` Steven Rostedt
2024-10-15 11:09 ` Ryan Roberts
2024-10-18 15:24 ` Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).