[13/15] external/trace: Change dump_trace to use mmap

The current lseek/read approach used in dump_trace makes no attempt to
handle the ring buffer aspect of the trace buffers. It does not move
back to the beginning of the trace buffer file as the buffer wraps
around. It also does not handle the overflow case of the writer
overwriting when the reader is up to. Mmap the trace buffer file so that
the existing reading functions in extra/trace.c can be used.  This
reduces code duplication. However this requires a kernel where the trace
buffer sysfs nodes are able to be mmaped (see
https://patchwork.ozlabs.org/patch/1056786/)
---
 core/test/run-trace.c       |  2 +-
 core/trace.c                |  4 +--
 external/trace/Makefile     |  2 +-
 external/trace/dump_trace.c | 54 +++++++++++++++----------------------
 include/trace_types.h       |  7 +----
 5 files changed, 27 insertions(+), 42 deletions(-)

On Mon, Mar 25, 2019 at 11:19 AM Jordan Niethe <jniethe5@gmail.com> wrote:
>
> The current lseek/read approach used in dump_trace makes no attempt to
> handle the ring buffer aspect of the trace buffers. It does not move
> back to the beginning of the trace buffer file as the buffer wraps
> around. It also does not handle the overflow case of the writer
> overwriting when the reader is up to. Mmap the trace buffer file so that
> the existing reading functions in extra/trace.c can be used.  This
> reduces code duplication. However this requires a kernel where the trace
> buffer sysfs nodes are able to be mmaped (see
> https://patchwork.ozlabs.org/patch/1056786/)
> ---
>  core/test/run-trace.c       |  2 +-
>  core/trace.c                |  4 +--
>  external/trace/Makefile     |  2 +-
>  external/trace/dump_trace.c | 54 +++++++++++++++----------------------
>  include/trace_types.h       |  7 +----
>  5 files changed, 27 insertions(+), 42 deletions(-)
>
> diff --git a/core/test/run-trace.c b/core/test/run-trace.c
> index 19bd2e67dbdd..1f47293e406a 100644
> --- a/core/test/run-trace.c
> +++ b/core/test/run-trace.c
> @@ -182,7 +182,7 @@ static void test_parallel(void)
>
>         for (i = 0; i < CPUS; i++) {
>                 fake_cpus[i].trace = p + i * len;
> -               fake_cpus[i].trace->tb.max_size = cpu_to_be32(sizeof(union trace));
> +               fake_cpus[i].trace->tb.max_size = cpu_to_be64(sizeof(union trace));
>                 fake_cpus[i].is_secondary = false;
>                 memset(&trace_readers[i], 0, sizeof(struct trace_reader));
>                 trace_readers[i].tb = &fake_cpus[i].trace->tb;
> diff --git a/core/trace.c b/core/trace.c
> index dfb5cfad045c..72c8b34b8b81 100644
> --- a/core/trace.c
> +++ b/core/trace.c
> @@ -39,7 +39,7 @@ static struct {
>  void init_boot_tracebuf(struct cpu_thread *boot_cpu)
>  {
>         init_lock(&boot_tracebuf.trace_info.lock);
> -       boot_tracebuf.trace_info.tb.max_size = cpu_to_be32(MAX_SIZE);
> +       boot_tracebuf.trace_info.tb.max_size = cpu_to_be64(MAX_SIZE);
>
>         boot_cpu->trace = &boot_tracebuf.trace_info;
>  }
> @@ -227,7 +227,7 @@ void init_trace_buffers(void)
>                         any = t->trace;
>                         memset(t->trace, 0, size);
>                         init_lock(&t->trace->lock);
> -                       t->trace->tb.max_size = cpu_to_be32(MAX_SIZE);
> +                       t->trace->tb.max_size = cpu_to_be64(MAX_SIZE);
>                         trace_add_desc(any, size, t->pir);
>                 } else
>                         prerror("TRACE: cpu 0x%x allocation failed\n", t->pir);
> diff --git a/external/trace/Makefile b/external/trace/Makefile
> index 3828fea534ea..bff52f3a6ab2 100644
> --- a/external/trace/Makefile
> +++ b/external/trace/Makefile
> @@ -1,7 +1,7 @@
>  HOSTEND=$(shell uname -m | sed -e 's/^i.*86$$/LITTLE/' -e 's/^x86.*/LITTLE/' -e 's/^ppc64le/LITTLE/' -e 's/^ppc.*/BIG/')
>  CFLAGS=-g -Wall -DHAVE_$(HOSTEND)_ENDIAN -I../../include -I../../
>
> -dump_trace: dump_trace.c
> +dump_trace: dump_trace.c trace.c
>
>  clean:
>         rm -f dump_trace *.o
> diff --git a/external/trace/dump_trace.c b/external/trace/dump_trace.c
> index 4779dc42ec6d..f38b38187691 100644
> --- a/external/trace/dump_trace.c
> +++ b/external/trace/dump_trace.c
> @@ -14,42 +14,24 @@
>   * limitations under the License.
>   */
>
> +#include <trace.h>
>  #include <err.h>
>  #include <stdio.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
> +#include <sys/mman.h>
>  #include <fcntl.h>
>  #include <string.h>
>  #include <inttypes.h>
>  #include <stdbool.h>
>  #include <stddef.h>
>  #include <unistd.h>
> +#include <stdlib.h>
>
>  #include "../../ccan/endian/endian.h"
>  #include "../../ccan/short_types/short_types.h"
> -#include <trace_types.h>
> +#include "trace.h"
>
> -/* Handles trace from debugfs (one record at a time) or file */
> -static bool get_trace(int fd, union trace *t, int *len)
> -{
> -       void *dest = t;
> -       int r;
> -
> -       /* Move down any extra we read last time. */
> -       if (*len >= sizeof(t->hdr) && *len >= t->hdr.len_div_8 * 8) {
> -               u8 rlen = t->hdr.len_div_8 * 8;
> -               memmove(dest, dest + rlen, *len - rlen);
> -               *len -= rlen;
> -       }
> -
> -       r = read(fd, dest + *len, sizeof(*t) - *len);
> -       if (r < 0)
> -               return false;
> -
> -       *len += r;
> -       /* We should have a complete record. */
> -       return *len >= sizeof(t->hdr) && *len >= t->hdr.len_div_8 * 8;
> -}
>
>  static void display_header(const struct trace_hdr *h)
>  {
> @@ -152,20 +134,28 @@ static void dump_uart(struct trace_uart *t)
>
>  int main(int argc, char *argv[])
>  {
> -       int fd, len = 0;
> +       int fd;
> +       struct trace_reader tr;
> +       struct trace_info *ti;
>         union trace t;
> -       const char *in = "/sys/kernel/debug/powerpc/opal-trace";
>
> -       if (argc > 2)
> -               errx(1, "Usage: dump_trace [file]");
> +       if (argc != 2)
> +               errx(1, "Usage: dump_trace file");
> +
> +       fd = open(argv[1], O_RDONLY);
> +       if(fd < 0)
> +               err(1, "Opening %s", argv[1]);
> +

> +       ti = mmap(NULL, sizeof(struct trace_info) + TBUF_SZ + MAX_SIZE,
> +                       PROT_READ, MAP_PRIVATE, fd, 0);

[12:59 oliveroh@localhost .../opal/exports]$ ls -l
total 0
-r--------. 1 root root 8388608 Feb 21 15:15 hdat_map
-r--------. 1 root root  219533 Feb 21 15:15 symbol_map

Use stat on the file to work out the file size and mmap() that. What
you're doing there seems like a really wierd hack.

> +       if (ti == MAP_FAILED)
> +               err(1, "Mmaping %s", argv[1]);
> +

> +       memset(&tr, 0, sizeof(struct trace_reader));
> +       tr.tb = &ti->tb;

I'd use zalloc() or calloc() or whatever to pre-zero the memory.

>
> -       if (argv[1])
> -               in = argv[1];
> -       fd = open(in, O_RDONLY);
> -       if (fd < 0)
> -               err(1, "Opening %s", in);
>
> -       while (get_trace(fd, &t, &len)) {
> +       while (trace_get(&t, &tr)) {
>                 display_header(&t.hdr);
>                 switch (t.hdr.type) {
>                 case TRACE_REPEAT:
> diff --git a/include/trace_types.h b/include/trace_types.h
> index 9364186aedbe..e9fb8f353e62 100644
> --- a/include/trace_types.h
> +++ b/include/trace_types.h
> @@ -34,13 +34,8 @@ struct tracebuf {
>         __be64 end;
>         /* This is where the writer wrote to previously. */
>         __be64 last;
> -       /* This is where the reader is up to. */
> -       __be64 rpos;
> -       /* If the last one we read was a repeat, this shows how many. */
> -       __be32 last_repeat;
>         /* Maximum possible size of a record. */
> -       __be32 max_size;
> -
> +       __be64 max_size;

Why's this now 64 bit?

>         char buf[/* TBUF_SZ + max_size */];
>  };
>
> --
> 2.20.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot