[Skiboot] [PATCH 06/15] core/trace: Change trace buffer size

[Oliver]

On Mon, Mar 25, 2019 at 11:17 AM Jordan Niethe <jniethe5 at 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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot