[Skiboot] [PATCH 13/15] external/trace: Change dump_trace to use mmap
Oliver
oohall at gmail.com
Mon Mar 25 13:03:16 AEDT 2019
On Mon, Mar 25, 2019 at 11:19 AM Jordan Niethe <jniethe5 at 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 at 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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
More information about the Skiboot
mailing list