On 13-11-22 08:21 AM, Mathieu Desnoyers wrote:
> ----- Original Message -----
>> From: "Julien Desfossez" <[email protected]>
>> To: "mathieu desnoyers" <[email protected]>
>> Cc: [email protected], "Julien Desfossez" <[email protected]>
>> Sent: Thursday, November 21, 2013 11:05:20 AM
>> Subject: [BABELTRACE PATCH] Parse LTTng indexes
>>
>> If a trace file has an associated index (same filename and .idx suffix in the
>> "index" folder), we open it and use it instead of generating the index at
>> open.
>
> Please cut changelog at 72 or 76 lines. This is slightly too large for
> git show.
>
>>
>> Signed-off-by: Julien Desfossez <[email protected]>
>> ---
>> formats/ctf/ctf.c | 216
>> ++++++++++++++++++++++++++++++++++++-
>> include/Makefile.am | 1 +
>> include/babeltrace/ctf/ctf-index.h | 58 ++++++++++
>> include/babeltrace/ctf/types.h | 1 +
>> 4 files changed, 271 insertions(+), 5 deletions(-)
>> create mode 100644 include/babeltrace/ctf/ctf-index.h
>>
>> diff --git a/formats/ctf/ctf.c b/formats/ctf/ctf.c
>> index 60d9c9f..7a0a383 100644
>> --- a/formats/ctf/ctf.c
>> +++ b/formats/ctf/ctf.c
>> @@ -35,6 +35,7 @@
>> #include <babeltrace/context-internal.h>
>> #include <babeltrace/compat/uuid.h>
>> #include <babeltrace/endian.h>
>> +#include <babeltrace/ctf/ctf-index.h>
>> #include <inttypes.h>
>> #include <stdio.h>
>> #include <sys/mman.h>
>> @@ -71,6 +72,8 @@
>>
>> #define NSEC_PER_SEC 1000000000ULL
>>
>> +#define INDEX_PATH "./index/%s.idx"
>> +
>> int opt_clock_cycles,
>> opt_clock_seconds,
>> opt_clock_date,
>> @@ -1666,6 +1669,153 @@ error:
>> return ret;
>> }
>>
>> +static
>> +int import_stream_packet_index(struct ctf_trace *td,
>> + struct ctf_file_stream *file_stream)
>> +{
>> + struct ctf_stream_declaration *stream;
>> + struct ctf_stream_pos *pos;
>> + struct ctf_packet_index ctf_index;
>> + struct ctf_packet_index_file_hdr index_hdr;
>> + uint64_t packet_map_len = DEFAULT_HEADER_LEN;
>> + struct packet_index index;
>> + int index_read;
>> + int ret = 0;
>> + int first_packet = 1;
>> + size_t filesize;
>> + struct stat filestats;
>> + size_t len;
>> +
>> + pos = &file_stream->pos;
>> +
>> + len = fread(&index_hdr, sizeof(index_hdr), 1, pos->index_fp);
>> + if (len != 1) {
>> + perror("read index file header");
>> + goto error;
>> + }
>> +
>> + /* Check the index header */
>> + if (be32toh(index_hdr.magic) != CTF_INDEX_MAGIC) {
>> + fprintf(stderr, "[error] wrong index magic\n");
>> + ret = -1;
>> + goto error;
>> + }
>> + if (be32toh(index_hdr.index_major) != CTF_INDEX_MAJOR ||
>> + be32toh(index_hdr.index_minor) != CTF_INDEX_MINOR) {
>> + fprintf(stderr, "[error] Incompatible index file %" PRIu64
>> + ".%" PRIu64 ", supported %d.%d\n",
>> + be64toh(index_hdr.index_major),
>> + be64toh(index_hdr.index_minor), CTF_INDEX_MAJOR,
>> + CTF_INDEX_MINOR);
>> + ret = -1;
>> + goto error;
>> + }
>> +
>> + while((index_read = fread(&ctf_index, sizeof(ctf_index), 1,
>> pos->index_fp))
>> == 1) {
>
> missing space after "while", should be cut into 2 lines.
>
>> + uint64_t stream_id;
>> + int len_index;
>> +
>> + index.offset = be64toh(ctf_index.offset);
>> + index.packet_size = be64toh(ctf_index.packet_size);
>> + index.content_size = be64toh(ctf_index.content_size);
>> + index.timestamp_begin = be64toh(ctf_index.timestamp_begin);
>> + index.timestamp_end = be64toh(ctf_index.timestamp_end);
>> + index.events_discarded = be64toh(ctf_index.events_discarded);
>> + stream_id = be64toh(ctf_index.stream_id);
>> +
>> + if (!first_packet) {
>> + /* add index to packet array */
>> +
>> g_array_append_val(file_stream->pos.packet_cycles_index, index);
>> + continue;
>> + }
>> +
[...]
>> + /* map new base. Need mapping length from header. */
>> + pos->base_mma = mmap_align(packet_map_len >> LOG2_CHAR_BIT,
>> PROT_READ,
>> + MAP_PRIVATE, pos->fd, pos->mmap_offset);
>> + assert(pos->base_mma != MAP_FAILED);
>
> It makes no sense to map the trace packets when we should just be pulling the
> index
> from the index file. It makes the performance gain of having the index
> useless.
We need to compute the data_offset because we don't have it in the
index. But we only do this operation for the first packet of each
stream. Is that really a problem ?
Thanks,
Julien
>
>> + /*
>> + * Use current mapping size as temporary content and packet
>> + * size.
>> + */
>> + pos->content_size = packet_map_len;
>> + pos->packet_size = packet_map_len;
>> + pos->offset = 0; /* Position of the packet header */
>> +
>> + /* update trace_packet_header and stream_packet_context */
>> + if (pos->prot != PROT_WRITE &&
>> file_stream->parent.trace_packet_header) {
>> + /* Read packet header */
>> + ret = generic_rw(&pos->parent,
>> &file_stream->parent.trace_packet_header->p);
>> + assert(!ret);
>> + }
>> + if (pos->prot != PROT_WRITE &&
>> file_stream->parent.stream_packet_context)
>> {
>> + /* Read packet context */
>> + ret = generic_rw(&pos->parent,
>> &file_stream->parent.stream_packet_context->p);
>> + assert(!ret);
>> + }
>> + index.data_offset = pos->offset;
>> +
>> + /* read events discarded len from header */
>> + len_index = bt_struct_declaration_lookup_field_index(
>> +
>> file_stream->parent.stream_packet_context->declaration,
>> + g_quark_from_static_string("events_discarded"));
>
> The packet lost count in the index should already be extended to a 64-bit
> counter
> by the consumer daemon. We should not need to get the len_index from the
> trace.
>
> Is there any other reason why you map/unmap the trace ?
>
>> + if (len_index >= 0) {
>> + struct bt_definition *field;
>> +
>> + field = bt_struct_definition_get_field_from_index(
>> +
>> file_stream->parent.stream_packet_context,
>> + len_index);
>> + index.events_discarded_len = bt_get_int_len(field);
>> + }
>> + first_packet = 0;
>> + /* add index to packet array */
>> + g_array_append_val(file_stream->pos.packet_cycles_index, index);
>> + }
>> +
>> + ret = 0;
>> +
>> +error:
>> + return ret;
>> +}
>> +
>> /*
>> * Note: many file streams can inherit from the same stream class
>> * description (metadata).
>> @@ -1678,6 +1828,7 @@ int ctf_open_file_stream_read(struct ctf_trace *td,
>> const char *path, int flags,
>> int ret, fd, closeret;
>> struct ctf_file_stream *file_stream;
>> struct stat statbuf;
>> + char *index_name;
>>
>> fd = openat(td->dirfd, path, flags);
>> if (fd < 0) {
>> @@ -1693,13 +1844,18 @@ int ctf_open_file_stream_read(struct ctf_trace *td,
>> const char *path, int flags,
>> goto fstat_error;
>> }
>> if (S_ISDIR(statbuf.st_mode)) {
>> - fprintf(stderr, "[warning] Skipping directory '%s' found in
>> trace\n",
>> path);
>> + if (strncmp(path, "index", 5) != 0) {
>> + fprintf(stderr, "[warning] Skipping directory '%s' "
>> + "found in trace\n", path);
>> + }
>> ret = 0;
>> goto fd_is_dir_ok;
>> }
>>
>> file_stream = g_new0(struct ctf_file_stream, 1);
>> file_stream->pos.last_offset = LAST_OFFSET_POISON;
>> + file_stream->pos.fd = -1;
>> + file_stream->pos.index_fp = NULL;
>>
>> strncpy(file_stream->parent.path, path, PATH_MAX);
>> file_stream->parent.path[PATH_MAX - 1] = '\0';
>> @@ -1722,19 +1878,61 @@ int ctf_open_file_stream_read(struct ctf_trace *td,
>> const char *path, int flags,
>> * For now, only a single clock per trace is supported.
>> */
>> file_stream->parent.current_clock = td->parent.single_clock;
>> - ret = create_stream_packet_index(td, file_stream);
>> - if (ret) {
>> - fprintf(stderr, "[error] Stream index creation error.\n");
>> - goto error_index;
>> +
>> + /*
>> + * Allocate the index name for this stream and try to open it.
>> + */
>> + index_name = malloc((strlen(path) + sizeof(INDEX_PATH)) * sizeof(char));
>> + if (!index_name) {
>> + fprintf(stderr, "[error] Cannot allocate index filename\n");
>> + goto error_def;
>> + }
>> + snprintf(index_name, strlen(path) + sizeof(INDEX_PATH),
>> + INDEX_PATH, path);
>> +
>> + if (faccessat(td->dirfd, index_name, O_RDONLY, flags) == 0) {
>
> please exchange the ok and failure branches, and change the condition.
> Everywhere else, the first branch is the " < 0" case (error).
>
>> + ret = openat(td->dirfd, index_name, flags);
>> + if (ret < 0) {
>> + perror("Index file openat()");
>> + ret = -1;
>> + goto error_free;
>> + }
>> + file_stream->pos.index_fp = fdopen(ret, "r");
>> + ret = import_stream_packet_index(td, file_stream);
>> + if (ret) {
>> + ret = -1;
>> + goto error_index;
>> + }
>> + ret = fclose(file_stream->pos.index_fp);
>> + if (ret < 0) {
>> + perror("close index");
>> + goto error_free;
>> + }
>> + } else {
>> + ret = create_stream_packet_index(td, file_stream);
>> + if (ret) {
>> + fprintf(stderr, "[error] Stream index creation
>> error.\n");
>> + goto error_index;
>> + }
>> }
>> + free(index_name);
>> +
>> /* Add stream file to stream class */
>> g_ptr_array_add(file_stream->parent.stream_class->streams,
>> &file_stream->parent);
>> return 0;
>>
>> error_index:
>> + if (file_stream->pos.index_fp) {
>> + ret = fclose(file_stream->pos.index_fp);
>> + if (ret < 0) {
>> + perror("close index");
>> + }
>> + }
>> if (file_stream->parent.trace_packet_header)
>>
>> bt_definition_unref(&file_stream->parent.trace_packet_header->p);
>> +error_free:
>> + free(index_name);
>> error_def:
>> closeret = ctf_fini_pos(&file_stream->pos);
>> if (closeret) {
>> @@ -1761,6 +1959,7 @@ int ctf_open_trace_read(struct ctf_trace *td,
>> struct dirent *dirent;
>> struct dirent *diriter;
>> size_t dirent_len;
>> + char *ext;
>>
>> td->flags = flags;
>>
>> @@ -1816,6 +2015,13 @@ int ctf_open_trace_read(struct ctf_trace *td,
>> || !strcmp(diriter->d_name, "..")
>> || !strcmp(diriter->d_name, "metadata"))
>> continue;
>> +
>> + /* Ignore index files : *.idx */
>> + ext = strrchr(diriter->d_name, '.');
>> + if (ext && (!strcmp(ext, ".idx"))) {
>> + continue;
>> + }
>> +
>> ret = ctf_open_file_stream_read(td, diriter->d_name,
>> flags, packet_seek);
>> if (ret) {
>> diff --git a/include/Makefile.am b/include/Makefile.am
>> index ec927b9..d9996a7 100644
>> --- a/include/Makefile.am
>> +++ b/include/Makefile.am
>> @@ -38,6 +38,7 @@ noinst_HEADERS = \
>> babeltrace/ctf-text/types.h \
>> babeltrace/ctf/types.h \
>> babeltrace/ctf/callbacks-internal.h \
>> + babeltrace/ctf/ctf-index.h \
>> babeltrace/ctf-writer/ref-internal.h \
>> babeltrace/ctf-writer/writer-internal.h \
>> babeltrace/ctf-writer/event-types-internal.h \
>> diff --git a/include/babeltrace/ctf/ctf-index.h
>> b/include/babeltrace/ctf/ctf-index.h
>> new file mode 100644
>> index 0000000..ee7ea26
>> --- /dev/null
>> +++ b/include/babeltrace/ctf/ctf-index.h
>> @@ -0,0 +1,58 @@
>> +/*
>> + * Copyright (C) 2013 - Julien Desfossez <[email protected]>
>> + * Mathieu Desnoyers <[email protected]>
>> + * David Goulet <[email protected]>
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
>> copy
>> + * of this software and associated documentation files (the "Software"), to
>> deal
>> + * in the Software without restriction, including without limitation the
>> rights
>> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
>> + * copies of the Software, and to permit persons to whom the Software is
>> + * furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included
>> in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>> OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> THE
>> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> FROM,
>> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>> THE
>> + * SOFTWARE.
>> + */
>> +
>> +#ifndef LTTNG_INDEX_H
>> +#define LTTNG_INDEX_H
>> +
>> +#include <limits.h>
>> +
>> +#define CTF_INDEX_MAGIC 0xC11D81C1
>> +#define CTF_INDEX_MAJOR 1
>> +#define CTF_INDEX_MINOR 0
>> +
>> +/*
>> + * Header at the beginning of each index file.
>> + * All integer fields are stored in big endian.
>> + */
>> +struct ctf_packet_index_file_hdr {
>> + uint32_t magic;
>
> You should submit the change to lttng-tools first.
>
> Thanks,
>
> Mathieu
>
>> + uint32_t index_major;
>> + uint32_t index_minor;
>> +} __attribute__((__packed__));
>> +
>> +/*
>> + * Packet index generated for each trace packet store in a trace file.
>> + * All integer fields are stored in big endian.
>> + */
>> +struct ctf_packet_index {
>> + uint64_t offset; /* offset of the packet in the file, in
>> bytes */
>> + uint64_t packet_size; /* packet size, in bits */
>> + uint64_t content_size; /* content size, in bits */
>> + uint64_t timestamp_begin;
>> + uint64_t timestamp_end;
>> + uint64_t events_discarded;
>> + uint64_t stream_id;
>> +} __attribute__((__packed__));
>> +
>> +#endif /* LTTNG_INDEX_H */
>> diff --git a/include/babeltrace/ctf/types.h b/include/babeltrace/ctf/types.h
>> index 96c5083..eeb0c57 100644
>> --- a/include/babeltrace/ctf/types.h
>> +++ b/include/babeltrace/ctf/types.h
>> @@ -61,6 +61,7 @@ struct packet_index {
>> struct ctf_stream_pos {
>> struct bt_stream_pos parent;
>> int fd; /* backing file fd. -1 if unset. */
>> + FILE *index_fp; /* backing index file fp. NULL if unset. */
>> GArray *packet_cycles_index; /* contains struct packet_index in
>> cycles */
>> GArray *packet_real_index; /* contains struct packet_index in ns */
>> int prot; /* mmap protection */
>> --
>> 1.8.3.2
>>
>>
>
_______________________________________________
lttng-dev mailing list
[email protected]
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
