On Wed, Mar 27, 2019 at 6:04 PM Yordan Karadzhov <ykaradzhov@xxxxxxxxxx> wrote:
>
> The NumPy API is meant to operate on top of the C API-s of trace-cmd and
> KernelShark and to provide only the minimum of basic functionalities needed
> in order to processor the tracing data. The NumPy API itself is made of two
> layers. A bottom-one written in C and a top-one which implements the
> interface, written in Cython (C-Python). This patch introduces the C layer.
Why is there a need for two layers? Since the upper layer is Cython,
it can access the libkernelshark C API just fine. I don't understand
the need for this middle layer.
Also there's already a Python binding for traceevent, maybe we can
utilize it for this instead of providing an ad hoc binding for trace
events here too?
On an unrelated note, Python 2 will be hitting its end of life date in
a couple of months[1]. Does it make sense to start with Python 2 in
2019 when we'll need to port it to Python 3 in the very near future? I
think going for Python 3 only might be a better choice.
[1] https://pythonclock.org/
Several non high-level comments follow inline.
>
> Signed-off-by: Yordan Karadzhov <ykaradzhov@xxxxxxxxxx>
> ---
> kernel-shark/src/libkshark-py.c | 176 ++++++++++++++++++++++++++++++++
> 1 file changed, 176 insertions(+)
> create mode 100644 kernel-shark/src/libkshark-py.c
>
> diff --git a/kernel-shark/src/libkshark-py.c b/kernel-shark/src/libkshark-py.c
> new file mode 100644
> index 0000000..4c589f3
> --- /dev/null
> +++ b/kernel-shark/src/libkshark-py.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: LGPL-2.1
> +
> +/*
> + * Copyright (C) 2019 VMware Inc, Yordan Karadzhov <y.karadz@xxxxxxxxx>
> + */
> +
> + /**
> + * @file libpykshark-py.c
^ libkshark-py.c?
> + * @brief Python API for processing of FTRACE (trace-cmd) data.
> + */
> +
> +// KernelShark
> +#include "libkshark.h"
> +#include "libkshark-model.h"
> +
> +bool kspy_open(const char *fname)
> +{
> + struct kshark_context *kshark_ctx = NULL;
> +
> + if (!kshark_instance(&kshark_ctx))
> + return false;
> +
> + return kshark_open(kshark_ctx, fname);
> +}
> +
> +void kspy_close(void)
> +{
> + struct kshark_context *kshark_ctx = NULL;
> +
> + if (!kshark_instance(&kshark_ctx))
> + return;
> +
> + kshark_close(kshark_ctx);
> + kshark_free(kshark_ctx);
> +}
> +
> +static int compare(const void * a, const void * b)
> +{
> + return ( *(int*)a - *(int*)b );
> +}
> +
> +size_t kspy_get_tasks(int **pids, char ***names)
> +{
> + struct kshark_context *kshark_ctx = NULL;
> + const char *comm;
> + ssize_t i, n;
> + int ret;
> +
> + if (!kshark_instance(&kshark_ctx))
> + return 0;
> +
> + n = kshark_get_task_pids(kshark_ctx, pids);
> + if (n == 0)
> + return 0;
> +
> + qsort(*pids, n, sizeof(**pids), compare);
> +
> + *names = calloc(n, sizeof(char*));
> + if (!(*names))
> + goto fail;
> +
> + for (i = 0; i < n; ++i) {
> + comm = tep_data_comm_from_pid(kshark_ctx->pevent, (*pids)[i]);
> + ret = asprintf(&(*names)[i], "%s", comm);
> + if (ret < 1)
> + goto fail;
> + }
> +
> + return n;
> +
> + fail:
> + free(*pids);
> + free(*names);
> + return 0;
> +}
> +
> +size_t kspy_trace2matrix(uint64_t **offset_array,
> + uint8_t **cpu_array,
> + uint64_t **ts_array,
> + uint16_t **pid_array,
> + int **event_array)
> +{
> + struct kshark_context *kshark_ctx = NULL;
> + size_t total = 0;
> +
> + if (!kshark_instance(&kshark_ctx))
> + return false;
> +
> + total = kshark_load_data_matrix(kshark_ctx, offset_array,
> + cpu_array,
> + ts_array,
> + pid_array,
> + event_array);
> +
> + return total;
> +}
> +
> +int kspy_get_event_id(const char *sys, const char *evt)
> +{
> + struct kshark_context *kshark_ctx = NULL;
> + struct tep_event *event;
> +
> + if (!kshark_instance(&kshark_ctx))
> + return -1;
> +
> + event = tep_find_event_by_name(kshark_ctx->pevent, sys, evt);
> +
> + return event->id;
> +}
> +
> +uint64_t kspy_read_event_field(uint64_t offset, const char *sys,
> + const char *evt,
> + const char *field)
> +{
> + struct kshark_context *kshark_ctx = NULL;
> + struct tep_format_field *evt_field;
> + struct tep_record *record;
> + struct tep_event *event;
> + unsigned long long val;
> + int ret;
> +
> + if (!kshark_instance(&kshark_ctx))
> + return 0;
> +
> + event = tep_find_event_by_name(kshark_ctx->pevent, sys, evt);
tep_find_event_by_name() is linear in the number of events and it does
a couple of thousand string comparisons. So calling
kspy_read_event_field() in a loop can get *really* slow. I haven't
measured, but I would guess that a good chunk of the running time of
the script in patch 7 is spent in this function.
We might want to provide an efficient API using event ids instead and
use tep_find_event() (which does binary search; that is 10s of
numerical comparisons) here to prevent Python users shooting
themselves in the foot performance-wise.
> + if (!event)
> + return 0;
> +
> + evt_field = tep_find_any_field(event, field);
> + if (!evt_field)
> + return 0;
> +
> + record = tracecmd_read_at(kshark_ctx->handle, offset, NULL);
> + if (!record)
> + return 0;
> +
> + ret = tep_read_number_field(evt_field, record->data, &val);
> + free_record(record);
> +
> + if (ret != 0)
> + return 0;
> +
> + return val;
> +}
> +
> +void kspy_new_session_file(const char *data_file, const char *session_file)
> +{
> + struct kshark_context *kshark_ctx = NULL;
> + struct kshark_trace_histo histo;
> + struct kshark_config_doc *session;
> + struct kshark_config_doc *filters;
> + struct kshark_config_doc *markers;
> + struct kshark_config_doc *model;
> + struct kshark_config_doc *file;
> +
> + if (!kshark_instance(&kshark_ctx))
> + return;
> +
> + session = kshark_config_new("kshark.config.session",
> + KS_CONFIG_JSON);
> +
> + file = kshark_export_trace_file(data_file, KS_CONFIG_JSON);
> + kshark_config_doc_add(session, "Data", file);
> +
> + filters = kshark_export_all_filters(kshark_ctx, KS_CONFIG_JSON);
> + kshark_config_doc_add(session, "Filters", filters);
> +
> + ksmodel_init(&histo);
> + model = kshark_export_model(&histo, KS_CONFIG_JSON);
> + kshark_config_doc_add(session, "Model", model);
> +
> + markers = kshark_config_new("kshark.config.markers", KS_CONFIG_JSON);
> + kshark_config_doc_add(session, "Markers", markers);
> +
> + kshark_save_config_file(session_file, session);
> +}
Aren't the struct kshark_config_doc variables leaking from this function?
Also, this function doesn't seem related to the Python bindings at
all. If it's useful on it's own, you might want to have it in
libkernelshark.
The Python binding layer should not contain any additional logic
besides defining the binding itself imo.
-- Slavi
|