| Message ID | 20190325001425.29483-7-jniethe5@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve usability of skiboot traces | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (b392d785eb49630b9f00fef8d17944ed82b2c1fe) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | fail | Signed-off-by missing |
On Mon, Mar 25, 2019 at 11:17 AM Jordan Niethe <jniethe5@gmail.com> wrote: > > For mmaping it makes sense for the trace buffer to fit on a page. This > is slightly complicated by the space taken up by the header at the > beginning of the trace and by the different sized elements that need to > be accommodated in the buffer. This is taken into account so that it > will fit on a 64K page. Not sure what you mean by "and by the different sized elements that need to be accomodated in the buffer". I also, can you be a bit more explicit about who is mmap()ing what and why? Repeating yourself a bit between commit messages is fine. Generally when I'm reading a commit messages it's because I've been trawling through git-blame trying to work out why a change happened so the commit messages appears outside the context of the rest of the series. Making a commit message stand-alone makes that sort of git archaeology much easier. > --- > core/test/run-trace.c | 2 +- > core/trace.c | 2 -- > include/trace.h | 4 +++- > include/trace_types.h | 2 ++ > 4 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/core/test/run-trace.c b/core/test/run-trace.c > index e53ab30ddf69..1455394da0e6 100644 > --- a/core/test/run-trace.c > +++ b/core/test/run-trace.c > @@ -133,7 +133,7 @@ static struct cpu_thread *this_cpu(void) > } > > #include <sys/mman.h> > -#define PER_CHILD_TRACES ((RUNNING_ON_VALGRIND) ? (1024*16) : (1024*1024)) > +#define PER_CHILD_TRACES TBUF_SZ > > static void write_trace_entries(int id) > { > diff --git a/core/trace.c b/core/trace.c > index 8e231b32eaa4..5580eb13a8bd 100644 > --- a/core/trace.c > +++ b/core/trace.c > @@ -29,8 +29,6 @@ > > #define DEBUG_TRACES > > -#define MAX_SIZE (sizeof(union trace) + 7) > - > /* Smaller trace buffer for early booting */ > static struct { > struct trace_info trace_info; > diff --git a/include/trace.h b/include/trace.h > index 2b65e9075591..68afa28355d8 100644 > --- a/include/trace.h > +++ b/include/trace.h > @@ -16,12 +16,12 @@ > > #ifndef __TRACE_H > #define __TRACE_H > +#include <skiboot.h> > #include <ccan/short_types/short_types.h> > #include <stddef.h> > #include <lock.h> > #include <trace_types.h> > > -#define TBUF_SZ (1024 * 1024) > > struct cpu_thread; > > @@ -35,6 +35,8 @@ struct trace_info { > struct tracebuf tb; > }; > > +#define TBUF_SZ ALIGN_DOWN((0x10000 - sizeof(struct trace_info) - MAX_SIZE), 0x10) Why is it aligned to a 16 byte boundary? This change also reduces the buffer to around 64K without any justification. I'm not opposed to changing the buffer size since 1MB per-core is probably overkill, but you need to spell out that you're doing it and why. Renaming MAX_SIZE to something a little more descriptive/specific might be a good idea too since trace_types.h is included (via trace.h) in any code that writes trace entries. Do that in a seperate patch though. > + > /* Allocate trace buffers once we know memory topology */ > void init_trace_buffers(void); > > diff --git a/include/trace_types.h b/include/trace_types.h > index d14c78d2eed6..9364186aedbe 100644 > --- a/include/trace_types.h > +++ b/include/trace_types.h > @@ -129,4 +129,6 @@ union trace { > struct trace_uart uart; > }; > > +#define MAX_SIZE (sizeof(union trace) + 7) > + > #endif /* __TRACE_TYPES_H */ > -- > 2.20.1 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
diff --git a/core/test/run-trace.c b/core/test/run-trace.c index e53ab30ddf69..1455394da0e6 100644 --- a/core/test/run-trace.c +++ b/core/test/run-trace.c @@ -133,7 +133,7 @@ static struct cpu_thread *this_cpu(void) } #include <sys/mman.h> -#define PER_CHILD_TRACES ((RUNNING_ON_VALGRIND) ? (1024*16) : (1024*1024)) +#define PER_CHILD_TRACES TBUF_SZ static void write_trace_entries(int id) { diff --git a/core/trace.c b/core/trace.c index 8e231b32eaa4..5580eb13a8bd 100644 --- a/core/trace.c +++ b/core/trace.c @@ -29,8 +29,6 @@ #define DEBUG_TRACES -#define MAX_SIZE (sizeof(union trace) + 7) - /* Smaller trace buffer for early booting */ static struct { struct trace_info trace_info; diff --git a/include/trace.h b/include/trace.h index 2b65e9075591..68afa28355d8 100644 --- a/include/trace.h +++ b/include/trace.h @@ -16,12 +16,12 @@ #ifndef __TRACE_H #define __TRACE_H +#include <skiboot.h> #include <ccan/short_types/short_types.h> #include <stddef.h> #include <lock.h> #include <trace_types.h> -#define TBUF_SZ (1024 * 1024) struct cpu_thread; @@ -35,6 +35,8 @@ struct trace_info { struct tracebuf tb; }; +#define TBUF_SZ ALIGN_DOWN((0x10000 - sizeof(struct trace_info) - MAX_SIZE), 0x10) + /* Allocate trace buffers once we know memory topology */ void init_trace_buffers(void); diff --git a/include/trace_types.h b/include/trace_types.h index d14c78d2eed6..9364186aedbe 100644 --- a/include/trace_types.h +++ b/include/trace_types.h @@ -129,4 +129,6 @@ union trace { struct trace_uart uart; }; +#define MAX_SIZE (sizeof(union trace) + 7) + #endif /* __TRACE_TYPES_H */