[PATCH v3 00/10] cxl: add monitor support for trace events

[PATCH v3 00/10] cxl: add monitor support for trace events

[Dave Jiang]

Steve, can you please take a look at the usages of libtraceevent and
libtracefs and provide feedback on if they are properly done? Thanks!

v3:
- Change uuid parsing from u8[] to uuid_t (Alison)
- Add event_name to event_ctx for filtering (Alison)
- Add event_pid to event_ctx for filtering (Alison)
- Add parse_event callback to event_ctx for filtering (Alison)

v2:
- Simplify logging functions (Nathan)
- Drop ndctl prefix (Vishal)
- Reduce to single trace event system (Alison)
- Add systemd startup file
- Add man page

This patch series for ndctl implements the monitor command for the cxl
tool. The initial implementation will collect CXL trace events emitted
by the kernel. libtraceevent and libtracefs will be used to parse the
trace event buffer. The monitor will pend on an epoll fd and wait for
new event entries to be posted. The output will be in json format. By
default the events are emitted to stdio, but can also be logged to a
file. Each event is converted to a JSON object and logged as such.
All the fields exported are read by the monitor code and added to the
JSON object.

---

Alison Schofield (1):
      cxl: add an optional pid check to event parsing

Dave Jiang (9):
      cxl: add helper function to parse trace event to json object
      cxl: add helper to parse through all current events
      cxl: add common function to enable event trace
      cxl: add common function to disable event trace
      cxl: add monitor function for event trace events
      cxl: add logging functions for monitor
      cxl: add monitor command to cxl
      cxl: add systemd service for monitor
      cxl: add man page documentation for monitor


 Documentation/cxl/cxl-monitor.txt |  77 +++++++++
 cxl/builtin.h                     |   1 +
 cxl/cxl-monitor.service           |   9 ++
 cxl/cxl.c                         |   1 +
 cxl/event_trace.c                 | 249 ++++++++++++++++++++++++++++++
 cxl/event_trace.h                 |  27 ++++
 cxl/meson.build                   |   8 +
 cxl/monitor.c                     | 240 ++++++++++++++++++++++++++++
 meson.build                       |   3 +
 ndctl.spec.in                     |   1 +
 10 files changed, 616 insertions(+)
 create mode 100644 Documentation/cxl/cxl-monitor.txt
 create mode 100644 cxl/cxl-monitor.service
 create mode 100644 cxl/event_trace.c
 create mode 100644 cxl/event_trace.h
 create mode 100644 cxl/monitor.c

--

[PATCH v3 01/10] cxl: add helper function to parse trace event to json object

[Dave Jiang]

Add the helper function that parses a trace event captured by
libtraceevent in a tep handle. All the parsed fields are added to a json
object. The json object is added to the provided list in the input parameter.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/event_trace.c |  166 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 cxl/event_trace.h |   14 ++++
 cxl/meson.build   |    2 +
 meson.build       |    1 
 4 files changed, 183 insertions(+)
 create mode 100644 cxl/event_trace.c
 create mode 100644 cxl/event_trace.h

diff --git a/cxl/event_trace.c b/cxl/event_trace.c
new file mode 100644
index 000000000000..803df34452f3
--- /dev/null
+++ b/cxl/event_trace.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2022, Intel Corp. All rights reserved.
+#include <stdio.h>
+#include <json-c/json.h>
+#include <util/json.h>
+#include <util/util.h>
+#include <util/parse-options.h>
+#include <util/parse-configs.h>
+#include <util/strbuf.h>
+#include <util/sysfs.h>
+#include <ccan/list/list.h>
+#include <ndctl/ndctl.h>
+#include <ndctl/libndctl.h>
+#include <sys/epoll.h>
+#include <sys/stat.h>
+#include <libcxl.h>
+#include <uuid/uuid.h>
+#include <traceevent/event-parse.h>
+#include "json.h"
+#include "event_trace.h"
+
+#define _GNU_SOURCE
+#include <string.h>
+
+static struct json_object *num_to_json(void *num, int size)
+{
+	if (size <= 4)
+		return json_object_new_int(*(int *)num);
+
+	return util_json_object_hex(*(unsigned long long *)num, 0);
+}
+
+static int cxl_event_to_json_callback(struct tep_event *event,
+		struct tep_record *record, struct list_head *jlist_head)
+{
+	struct tep_format_field **fields;
+	struct json_object *jevent, *jobj, *jarray;
+	struct jlist_node *jnode;
+	int i, j, rc = 0;
+
+	jnode = malloc(sizeof(*jnode));
+	if (!jnode)
+		return -ENOMEM;
+
+	jevent = json_object_new_object();
+	if (!jevent) {
+		rc = -ENOMEM;
+		goto err_jevent;
+	}
+	jnode->jobj = jevent;
+
+	fields = tep_event_fields(event);
+	if (!fields) {
+		rc = -ENOENT;
+		goto err;
+	}
+
+	jobj = json_object_new_string(event->system);
+	if (!jobj) {
+		rc = -ENOMEM;
+		goto err;
+	}
+	json_object_object_add(jevent, "system", jobj);
+
+	jobj = json_object_new_string(event->name);
+	if (!jobj) {
+		rc = -ENOMEM;
+		goto err;
+	}
+	json_object_object_add(jevent, "event", jobj);
+
+	jobj = json_object_new_uint64(record->ts);
+	if (!jobj) {
+		rc = -ENOMEM;
+		goto err;
+	}
+	json_object_object_add(jevent, "timestamp", jobj);
+
+	for (i = 0; fields[i]; i++) {
+		struct tep_format_field *f = fields[i];
+		int len;
+		char *tmp;
+
+		tmp = strcasestr(f->type, "char[]");
+		if (tmp) { /* event field is a string */
+			char *str;
+
+			str = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
+			if (!str)
+				continue;
+
+			jobj = json_object_new_string(str);
+			if (!jobj) {
+				rc = -ENOMEM;
+				goto err;
+			}
+
+			json_object_object_add(jevent, f->name, jobj);
+		} else if (f->arraylen) { /* data array */
+			unsigned char *data;
+			int chunks;
+
+			data = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
+			if (!data)
+				continue;
+
+			jarray = json_object_new_array();
+			if (!jarray) {
+				rc = -ENOMEM;
+				goto err;
+			}
+
+			chunks = f->size / f->elementsize;
+			for (j = 0; j < chunks; j++) {
+				jobj = num_to_json(data, f->elementsize);
+				if (!jobj) {
+					json_object_put(jarray);
+					return -ENOMEM;
+				}
+				json_object_array_add(jarray, jobj);
+				data += f->elementsize;
+			}
+
+			json_object_object_add(jevent, f->name, jarray);
+		} else { /* single number */
+			unsigned char *data;
+
+			data = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
+			if (!data)
+				continue;
+
+			/* check to see if we have a UUID */
+			tmp = strcasestr(f->type, "uuid_t");
+			if (tmp) {
+				char uuid[SYSFS_ATTR_SIZE];
+
+				uuid_unparse(data, uuid);
+				jobj = json_object_new_string(uuid);
+				if (!jobj) {
+					rc = -ENOMEM;
+					goto err;
+				}
+
+				json_object_object_add(jevent, f->name, jobj);
+				continue;
+			}
+
+			jobj = num_to_json(data, f->elementsize);
+			if (!jobj) {
+				rc = -ENOMEM;
+				goto err;
+			}
+
+			json_object_object_add(jevent, f->name, jobj);
+		}
+	}
+
+	list_add_tail(jlist_head, &jnode->list);
+	return 0;
+
+err:
+	json_object_put(jevent);
+err_jevent:
+	free(jnode);
+	return rc;
+}
diff --git a/cxl/event_trace.h b/cxl/event_trace.h
new file mode 100644
index 000000000000..00975a0b5680
--- /dev/null
+++ b/cxl/event_trace.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2022 Intel Corporation. All rights reserved. */
+#ifndef __CXL_EVENT_TRACE_H__
+#define __CXL_EVENT_TRACE_H__
+
+#include <json-c/json.h>
+#include <ccan/list/list.h>
+
+struct jlist_node {
+	struct json_object *jobj;
+	struct list_node list;
+};
+
+#endif
diff --git a/cxl/meson.build b/cxl/meson.build
index f2474aaa6e2e..8c7733431613 100644
--- a/cxl/meson.build
+++ b/cxl/meson.build
@@ -7,6 +7,7 @@ cxl_src = [
   'memdev.c',
   'json.c',
   'filter.c',
+  'event_trace.c',
 ]
 
 cxl_tool = executable('cxl',
@@ -19,6 +20,7 @@ cxl_tool = executable('cxl',
     kmod,
     json,
     versiondep,
+    traceevent,
   ],
   install : true,
   install_dir : rootbindir,
diff --git a/meson.build b/meson.build
index 20a646d135c7..f611e0bdd7f3 100644
--- a/meson.build
+++ b/meson.build
@@ -142,6 +142,7 @@ kmod = dependency('libkmod')
 libudev = dependency('libudev')
 uuid = dependency('uuid')
 json = dependency('json-c')
+traceevent = dependency('libtraceevent')
 if get_option('docs').enabled()
   if get_option('asciidoctor').enabled()
     asciidoc = find_program('asciidoctor', required : true)

[PATCH v3 02/10] cxl: add helper to parse through all current events

[Dave Jiang]

Add common function to iterate through and extract the events in the
current trace buffer. The function uses tracefs_iterate_raw_events() from
libtracefs to go through all the events loaded into a tep_handle. A
callback is provided to the API call in order to parse the event. For cxl
monitor, an array of interested "systems" is provided in order to filter
for the interested events.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/event_trace.c |   36 ++++++++++++++++++++++++++++++++++++
 cxl/event_trace.h |   10 ++++++++++
 cxl/meson.build   |    1 +
 meson.build       |    2 ++
 4 files changed, 49 insertions(+)

diff --git a/cxl/event_trace.c b/cxl/event_trace.c
index 803df34452f3..cf63d2346f6e 100644
--- a/cxl/event_trace.c
+++ b/cxl/event_trace.c
@@ -16,6 +16,7 @@
 #include <libcxl.h>
 #include <uuid/uuid.h>
 #include <traceevent/event-parse.h>
+#include <tracefs/tracefs.h>
 #include "json.h"
 #include "event_trace.h"
 
@@ -164,3 +165,38 @@ err_jevent:
 	free(jnode);
 	return rc;
 }
+
+static int cxl_event_parse_cb(struct tep_event *event, struct tep_record *record,
+		int cpu, void *ctx)
+{
+	struct event_ctx *event_ctx = (struct event_ctx *)ctx;
+
+	/* Filter out all the events that the caller isn't interested in. */
+	if (strcmp(event->system, event_ctx->system) != 0)
+		return 0;
+
+	if (event_ctx->event_name) {
+		if (strcmp(event->name, event_ctx->event_name) != 0)
+			return 0;
+	}
+
+	if (event_ctx->parse_event)
+		return event_ctx->parse_event(event, record, &event_ctx->jlist_head);
+
+	return cxl_event_to_json_callback(event, record, &event_ctx->jlist_head);
+}
+
+int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx)
+{
+	struct tep_handle *tep;
+	int rc;
+
+	tep = tracefs_local_events(NULL);
+	if (!tep)
+		return -ENOMEM;
+
+	rc = tracefs_iterate_raw_events(tep, inst, NULL, 0,
+			cxl_event_parse_cb, ectx);
+	tep_free(tep);
+	return rc;
+}
diff --git a/cxl/event_trace.h b/cxl/event_trace.h
index 00975a0b5680..582882c1eb35 100644
--- a/cxl/event_trace.h
+++ b/cxl/event_trace.h
@@ -11,4 +11,14 @@ struct jlist_node {
 	struct list_node list;
 };
 
+struct event_ctx {
+	const char *system;
+	struct list_head jlist_head;
+	const char *event_name;					/* optional */
+	int (*parse_event)(struct tep_event *event, struct tep_record *record,
+			   struct list_head *jlist_head);	/* optional */
+};
+
+int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx);
+
 #endif
diff --git a/cxl/meson.build b/cxl/meson.build
index 8c7733431613..c59876262e76 100644
--- a/cxl/meson.build
+++ b/cxl/meson.build
@@ -21,6 +21,7 @@ cxl_tool = executable('cxl',
     json,
     versiondep,
     traceevent,
+    tracefs,
   ],
   install : true,
   install_dir : rootbindir,
diff --git a/meson.build b/meson.build
index f611e0bdd7f3..c204c8ac52de 100644
--- a/meson.build
+++ b/meson.build
@@ -143,6 +143,8 @@ libudev = dependency('libudev')
 uuid = dependency('uuid')
 json = dependency('json-c')
 traceevent = dependency('libtraceevent')
+tracefs = dependency('libtracefs')
+
 if get_option('docs').enabled()
   if get_option('asciidoctor').enabled()
     asciidoc = find_program('asciidoctor', required : true)

[PATCH v3 03/10] cxl: add common function to enable event trace

[Dave Jiang]

Add a common function for cxl command to enable event tracing for the
instance created. The interested "systems" will be enabled for tracing
as well.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/event_trace.c |   21 +++++++++++++++++++++
 cxl/event_trace.h |    1 +
 2 files changed, 22 insertions(+)

diff --git a/cxl/event_trace.c b/cxl/event_trace.c
index cf63d2346f6e..d59e54c33df6 100644
--- a/cxl/event_trace.c
+++ b/cxl/event_trace.c
@@ -200,3 +200,24 @@ int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx)
 	tep_free(tep);
 	return rc;
 }
+
+int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system)
+{
+	int rc;
+	char *path;
+
+	rc = asprintf(&path, "events/%s/enable", system);
+	if (rc == -1)
+		return -errno;
+
+	rc = tracefs_instance_file_write(inst, path, "1");
+	free(path);
+	if (rc == -1)
+		return -errno;
+
+	if (tracefs_trace_is_on(inst))
+		return 0;
+
+	tracefs_trace_on(inst);
+	return 0;
+}
diff --git a/cxl/event_trace.h b/cxl/event_trace.h
index 582882c1eb35..0258b8dc65a3 100644
--- a/cxl/event_trace.h
+++ b/cxl/event_trace.h
@@ -20,5 +20,6 @@ struct event_ctx {
 };
 
 int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx);
+int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system);
 
 #endif

[PATCH v3 04/10] cxl: add common function to disable event trace

[Dave Jiang]

Add a common function for cxl command that disables the event trace for the
instance created.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/event_trace.c |    8 ++++++++
 cxl/event_trace.h |    1 +
 2 files changed, 9 insertions(+)

diff --git a/cxl/event_trace.c b/cxl/event_trace.c
index d59e54c33df6..bcd4f8b2968e 100644
--- a/cxl/event_trace.c
+++ b/cxl/event_trace.c
@@ -221,3 +221,11 @@ int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system)
 	tracefs_trace_on(inst);
 	return 0;
 }
+
+int cxl_event_tracing_disable(struct tracefs_instance *inst)
+{
+	if (!tracefs_trace_is_on(inst))
+		return 0;
+
+	return tracefs_trace_off(inst);
+}
diff --git a/cxl/event_trace.h b/cxl/event_trace.h
index 0258b8dc65a3..17d922f922c1 100644
--- a/cxl/event_trace.h
+++ b/cxl/event_trace.h
@@ -21,5 +21,6 @@ struct event_ctx {
 
 int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx);
 int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system);
+int cxl_event_tracing_disable(struct tracefs_instance *inst);
 
 #endif

[PATCH v3 05/10] cxl: add monitor function for event trace events

[Dave Jiang]

Add function that creates an event trace instance and utilize the cxl event
trace common functions to extract interested events from the trace buffer.
The monitoring function will pend on an epoll fd and wait for new events
to appear in the trace buffer.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/meson.build |    1 
 cxl/monitor.c   |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 141 insertions(+)
 create mode 100644 cxl/monitor.c

diff --git a/cxl/meson.build b/cxl/meson.build
index c59876262e76..eb8b2b1070ed 100644
--- a/cxl/meson.build
+++ b/cxl/meson.build
@@ -8,6 +8,7 @@ cxl_src = [
   'json.c',
   'filter.c',
   'event_trace.c',
+  'monitor.c',
 ]
 
 cxl_tool = executable('cxl',
diff --git a/cxl/monitor.c b/cxl/monitor.c
new file mode 100644
index 000000000000..85559d9a4b94
--- /dev/null
+++ b/cxl/monitor.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2022, Intel Corp. All rights reserved.
+/* Some bits copied from ndctl monitor code */
+#include <stdio.h>
+#include <unistd.h>
+#include <errno.h>
+#include <json-c/json.h>
+#include <libgen.h>
+#include <time.h>
+#include <dirent.h>
+#include <ccan/list/list.h>
+#include <util/json.h>
+#include <util/util.h>
+#include <util/parse-options.h>
+#include <util/parse-configs.h>
+#include <util/strbuf.h>
+#include <sys/epoll.h>
+#include <sys/stat.h>
+#include <traceevent/event-parse.h>
+#include <tracefs/tracefs.h>
+#include <cxl/libcxl.h>
+
+/* reuse the core log helpers for the monitor logger */
+#ifndef ENABLE_LOGGING
+#define ENABLE_LOGGING
+#endif
+#ifndef ENABLE_DEBUG
+#define ENABLE_DEBUG
+#endif
+#include <util/log.h>
+
+#include "event_trace.h"
+
+static const char *cxl_system = "cxl";
+
+static struct monitor {
+	struct log_ctx ctx;
+	FILE *log_file;
+	bool human;
+} monitor;
+
+static int monitor_event(struct cxl_ctx *ctx)
+{
+	int fd, epollfd, rc = 0, timeout = -1;
+	struct epoll_event ev, *events;
+	struct tracefs_instance *inst;
+	struct event_ctx ectx;
+	int jflag;
+
+	events = calloc(1, sizeof(struct epoll_event));
+	if (!events) {
+		err(&monitor, "alloc for events error\n");
+		return -ENOMEM;
+	}
+
+	epollfd = epoll_create1(0);
+	if (epollfd == -1) {
+		rc = -errno;
+		err(&monitor, "epoll_create1() error: %d\n", rc);
+		goto epoll_err;
+	}
+
+	inst = tracefs_instance_create("cxl_monitor");
+	if (!inst) {
+		rc = -errno;
+		err(&monitor, "tracefs_instance_crate( failed: %d\n", rc);
+		goto inst_err;
+	}
+
+	fd = tracefs_instance_file_open(inst, "trace_pipe", -1);
+	if (fd < 0) {
+		rc = fd;
+		err(&monitor, "tracefs_instance_file_open() err: %d\n", rc);
+		goto inst_file_err;
+	}
+
+	memset(&ev, 0, sizeof(ev));
+	ev.events = EPOLLIN;
+	ev.data.fd = fd;
+
+	if (epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev) != 0) {
+		rc = -errno;
+		err(&monitor, "epoll_ctl() error: %d\n", rc);
+		goto epoll_ctl_err;
+	}
+
+	rc = cxl_event_tracing_enable(inst, cxl_system);
+	if (rc < 0) {
+		err(&monitor, "cxl_trace_event_enable() failed: %d\n", rc);
+		goto event_en_err;
+	}
+
+	memset(&ectx, 0, sizeof(ectx));
+	ectx.system = cxl_system;
+	if (monitor.human)
+		jflag = JSON_C_TO_STRING_PRETTY;
+	else
+		jflag = JSON_C_TO_STRING_PLAIN;
+
+	while (1) {
+		struct jlist_node *jnode, *next;
+
+		rc = epoll_wait(epollfd, events, 1, timeout);
+		if (rc < 0) {
+			rc = -errno;
+			if (errno != EINTR)
+				err(&monitor, "epoll_wait error: %d\n", -errno);
+			break;
+		}
+
+		list_head_init(&ectx.jlist_head);
+		rc = cxl_parse_events(inst, &ectx);
+		if (rc < 0)
+			goto parse_err;
+
+		if (list_empty(&ectx.jlist_head))
+			continue;
+
+		list_for_each_safe(&ectx.jlist_head, jnode, next, list) {
+			notice(&monitor, "%s\n",
+				json_object_to_json_string_ext(jnode->jobj, jflag));
+			list_del(&jnode->list);
+			json_object_put(jnode->jobj);
+			free(jnode);
+		}
+	}
+
+parse_err:
+	rc = cxl_event_tracing_disable(inst);
+event_en_err:
+epoll_ctl_err:
+	close(fd);
+inst_file_err:
+	tracefs_instance_free(inst);
+inst_err:
+	close(epollfd);
+epoll_err:
+	free(events);
+	return rc;
+}

[PATCH v3 06/10] cxl: add logging functions for monitor

[Dave Jiang]

Duplicate log functions from ndctl/monitor to use for stdout and file
logging.

Signed-off-by: Dave Jiang <dave.jiang@gmail.com>
---
 cxl/monitor.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/cxl/monitor.c b/cxl/monitor.c
index 85559d9a4b94..9af12954b89b 100644
--- a/cxl/monitor.c
+++ b/cxl/monitor.c
@@ -39,6 +39,31 @@ static struct monitor {
 	bool human;
 } monitor;
 
+static void log_standard(struct log_ctx *ctx, int priority, const char *file,
+		int line, const char *fn, const char *format, va_list args)
+{
+	if (priority == 6)
+		vfprintf(stdout, format, args);
+	else
+		vfprintf(stderr, format, args);
+}
+
+static void log_file(struct log_ctx *ctx, int priority, const char *file,
+		int line, const char *fn, const char *format, va_list args)
+{
+	FILE *f = monitor.log_file;
+
+	if (priority != LOG_NOTICE) {
+		struct timespec ts;
+
+		clock_gettime(CLOCK_REALTIME, &ts);
+		fprintf(f, "[%10ld.%09ld] [%d] ", ts.tv_sec, ts.tv_nsec, getpid());
+	}
+
+	vfprintf(f, format, args);
+	fflush(f);
+}
+
 static int monitor_event(struct cxl_ctx *ctx)
 {
 	int fd, epollfd, rc = 0, timeout = -1;

[PATCH v3 07/10] cxl: add monitor command to cxl

[Dave Jiang]

Connect the monitoring functionality to the cxl monitor command. Add basic
functionality to the cxl monitor command where it can be launched as a daemon
and logging can be designated to stdout or a file.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/builtin.h |    1 +
 cxl/cxl.c     |    1 +
 cxl/monitor.c |   75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+)

diff --git a/cxl/builtin.h b/cxl/builtin.h
index b28c2213993b..34c5cfb49051 100644
--- a/cxl/builtin.h
+++ b/cxl/builtin.h
@@ -22,4 +22,5 @@ int cmd_create_region(int argc, const char **argv, struct cxl_ctx *ctx);
 int cmd_enable_region(int argc, const char **argv, struct cxl_ctx *ctx);
 int cmd_disable_region(int argc, const char **argv, struct cxl_ctx *ctx);
 int cmd_destroy_region(int argc, const char **argv, struct cxl_ctx *ctx);
+int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx);
 #endif /* _CXL_BUILTIN_H_ */
diff --git a/cxl/cxl.c b/cxl/cxl.c
index dd1be7a054a1..3be7026f43d3 100644
--- a/cxl/cxl.c
+++ b/cxl/cxl.c
@@ -76,6 +76,7 @@ static struct cmd_struct commands[] = {
 	{ "enable-region", .c_fn = cmd_enable_region },
 	{ "disable-region", .c_fn = cmd_disable_region },
 	{ "destroy-region", .c_fn = cmd_destroy_region },
+	{ "monitor", .c_fn = cmd_monitor },
 };
 
 int main(int argc, const char **argv)
diff --git a/cxl/monitor.c b/cxl/monitor.c
index 9af12954b89b..1ca8540d887f 100644
--- a/cxl/monitor.c
+++ b/cxl/monitor.c
@@ -32,11 +32,15 @@
 #include "event_trace.h"
 
 static const char *cxl_system = "cxl";
+const char *default_log = "/var/log/cxl-monitor.log";
 
 static struct monitor {
+	const char *log;
 	struct log_ctx ctx;
 	FILE *log_file;
 	bool human;
+	bool verbose;
+	bool daemon;
 } monitor;
 
 static void log_standard(struct log_ctx *ctx, int priority, const char *file,
@@ -163,3 +167,74 @@ epoll_err:
 	free(events);
 	return rc;
 }
+
+int cmd_monitor(int argc, const char **argv, struct cxl_ctx *ctx)
+{
+	const struct option options[] = {
+		OPT_FILENAME('l', "log", &monitor.log,
+				"<file> | standard",
+				"where to output the monitor's notification"),
+		OPT_BOOLEAN('\0', "daemon", &monitor.daemon,
+				"run cxl monitor as a daemon"),
+		OPT_BOOLEAN('u', "human", &monitor.human,
+				"use human friendly output formats"),
+		OPT_BOOLEAN('v', "verbose", &monitor.verbose,
+				"emit extra debug messages to log"),
+		OPT_END(),
+	};
+	const char * const u[] = {
+		"cxl monitor [<options>]",
+		NULL
+	};
+	const char *prefix ="./";
+	int rc = 0, i;
+
+	argc = parse_options_prefix(argc, argv, prefix, options, u, 0);
+	for (i = 0; i < argc; i++)
+		error("unknown parameter \"%s\"\n", argv[i]);
+	if (argc)
+		usage_with_options(u, options);
+
+	log_init(&monitor.ctx, "cxl/monitor", "CXL_MONITOR_LOG");
+	monitor.ctx.log_fn = log_standard;
+
+	if (monitor.verbose)
+		monitor.ctx.log_priority = LOG_DEBUG;
+	else
+		monitor.ctx.log_priority = LOG_INFO;
+
+	if (monitor.log) {
+		if (strncmp(monitor.log, "./", 2) != 0)
+			fix_filename(prefix, (const char **)&monitor.log);
+		if (strncmp(monitor.log, "./standard", 10) == 0 && !monitor.daemon) {
+			monitor.ctx.log_fn = log_standard;
+		} else {
+			const char *log = monitor.log;
+
+			if (!monitor.log)
+				log = default_log;
+			monitor.log_file = fopen(log, "a+");
+			if (!monitor.log_file) {
+				rc = -errno;
+				error("open %s failed: %d\n", monitor.log, rc);
+				goto out;
+			}
+			monitor.ctx.log_fn = log_file;
+		}
+	}
+
+	if (monitor.daemon) {
+		if (daemon(0, 0) != 0) {
+			err(&monitor, "daemon start failed\n");
+			goto out;
+		}
+		info(&monitor, "cxl monitor daemon started.\n");
+	}
+
+	rc = monitor_event(ctx);
+
+out:
+	if (monitor.log_file)
+		fclose(monitor.log_file);
+	return rc;
+}

[PATCH v3 08/10] cxl: add an optional pid check to event parsing

[Dave Jiang]

From: Alison Schofield <alison.schofield@intel.com>

When parsing CXL events, callers may only be interested in events
that originate from the current process. Introduce an optional
argument to the event trace context: event_pid. When event_pid is
present, only include events with a matching pid in the returned
JSON list. It is not a failure to see other, non matching results.
Simply skip those.

The initial use case for this is the listing of media errors,
where only the media-errors requested by this process are wanted.

Signed-off-by: Alison Schofield <alison.schofield@intel.com>
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/event_trace.c |   18 ++++++++++++++++++
 cxl/event_trace.h |    1 +
 2 files changed, 19 insertions(+)

diff --git a/cxl/event_trace.c b/cxl/event_trace.c
index bcd4f8b2968e..0be6317e6ada 100644
--- a/cxl/event_trace.c
+++ b/cxl/event_trace.c
@@ -166,6 +166,19 @@ err_jevent:
 	return rc;
 }
 
+static bool cxl_match_pid(struct tep_event *event, struct tep_record *record,
+			  int pid)
+{
+	unsigned long long val;
+
+	if (tep_get_common_field_val(NULL, event, "common_pid", record, &val, 0))
+		return false;
+	if (pid != (int)val)
+		return false;
+
+	return true;
+}
+
 static int cxl_event_parse_cb(struct tep_event *event, struct tep_record *record,
 		int cpu, void *ctx)
 {
@@ -180,6 +193,11 @@ static int cxl_event_parse_cb(struct tep_event *event, struct tep_record *record
 			return 0;
 	}
 
+	if (event_ctx->event_pid) {
+		if (!cxl_match_pid(event, record, event_ctx->event_pid))
+			return 0;
+	}
+
 	if (event_ctx->parse_event)
 		return event_ctx->parse_event(event, record, &event_ctx->jlist_head);
 
diff --git a/cxl/event_trace.h b/cxl/event_trace.h
index 17d922f922c1..64f07854b91b 100644
--- a/cxl/event_trace.h
+++ b/cxl/event_trace.h
@@ -15,6 +15,7 @@ struct event_ctx {
 	const char *system;
 	struct list_head jlist_head;
 	const char *event_name;					/* optional */
+	int event_pid;						/* optional */
 	int (*parse_event)(struct tep_event *event, struct tep_record *record,
 			   struct list_head *jlist_head);	/* optional */
 };

[PATCH v3 09/10] cxl: add systemd service for monitor

[Dave Jiang]

Add a systemd service file for cxl monitor to start the monitoring service
on boot initialization. Add the installation setup for the service file.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 cxl/cxl-monitor.service |    9 +++++++++
 cxl/meson.build         |    4 ++++
 ndctl.spec.in           |    1 +
 3 files changed, 14 insertions(+)
 create mode 100644 cxl/cxl-monitor.service

diff --git a/cxl/cxl-monitor.service b/cxl/cxl-monitor.service
new file mode 100644
index 000000000000..87c842b6f595
--- /dev/null
+++ b/cxl/cxl-monitor.service
@@ -0,0 +1,9 @@
+[Unit]
+Description=Cxl Monitor Daemon
+
+[Service]
+Type=simple
+ExecStart=/usr/bin/cxl monitor
+
+[Install]
+WantedBy=multi-user.target
diff --git a/cxl/meson.build b/cxl/meson.build
index eb8b2b1070ed..fc2e946707a8 100644
--- a/cxl/meson.build
+++ b/cxl/meson.build
@@ -11,6 +11,10 @@ cxl_src = [
   'monitor.c',
 ]
 
+if get_option('systemd').enabled()
+  install_data('cxl-monitor.service', install_dir : systemdunitdir)
+endif
+
 cxl_tool = executable('cxl',
   cxl_src,
   include_directories : root_inc,
diff --git a/ndctl.spec.in b/ndctl.spec.in
index cfcafa2ba816..c883317c5ce7 100644
--- a/ndctl.spec.in
+++ b/ndctl.spec.in
@@ -194,6 +194,7 @@ fi
 %{_bindir}/cxl
 %{_mandir}/man1/cxl*
 %{bashcompdir}/cxl
+%{_unitdir}/cxl-monitor.service
 
 %files -n LNAME
 %defattr(-,root,root)

[PATCH v3 10/10] cxl: add man page documentation for monitor

[Dave Jiang]

Add man page documentation to explain the usage of cxl monitor.

Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 Documentation/cxl/cxl-monitor.txt |   77 +++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 Documentation/cxl/cxl-monitor.txt

diff --git a/Documentation/cxl/cxl-monitor.txt b/Documentation/cxl/cxl-monitor.txt
new file mode 100644
index 000000000000..43c2ece72220
--- /dev/null
+++ b/Documentation/cxl/cxl-monitor.txt
@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0
+
+cxl-monitor(1)
+================
+
+NAME
+----
+cxl-monitor - Monitor the CXL kernel trace events
+
+SYNOPSIS
+--------
+[verse]
+'cxl monitor' [<options>]
+
+DESCRIPTION
+-----------
+Cxl monitor is used for monitoring the CXL trace events emitted by
+the kernel and convert them to json objects and dumping the json format
+notifications to standard output or a logfile.
+
+Both, the values in configuration file and in options will work. If
+there is a conflict, the values in options will override the values in
+the configuration file. Any updated values in the configuration file will
+take effect only after the monitor process is restarted.
+
+EXAMPLES
+--------
+
+Run a monitor as a daemon to monitor events and output to a log file.
+[verse]
+cxl monitor --daemon --log=/var/log/cxl-monitor.log
+
+Run a monitor as a one-shot command and output the notifications to stdio.
+[verse]
+cxl monitor
+
+Run a monitor daemon as a system service
+[verse]
+systemctl start cxl-monitor.service
+
+OPTIONS
+-------
+-l::
+--log=::
+	Send log messages to the specified destination.
+	- "<file>":
+	  Send log messages to specified <file>. When fopen() is not able
+	  to open <file>, log messages will be forwarded to syslog.
+	- "standard":
+	  Send messages to standard output.
+
+The default log destination is '/var/log/cxl-monitor.log' if "--daemon" is specified,
+otherwise 'standard'. Note that standard and relative path for <file>
+will not work if "--daemon" is specified.
+
+--daemon::
+	Run a monitor as a daemon.
+
+-u::
+--human::
+	Output monitor notification as human friendly json format instead
+	of the default machine friendly json format.
+
+-v::
+--verbose::
+	Emit extra debug messages to log.
+
+COPYRIGHT
+---------
+Copyright (c) 2022, Intel Corp. License GPLv2: GNU GPL version 2
+<http://gnu.org/licenses/gpl.html>. This is free software: you are
+free to change and redistribute it. There is NO WARRANTY, to the
+extent permitted by law.
+
+SEE ALSO
+--------
+linkcxl:cxl-list[1]

Re: [PATCH v3 01/10] cxl: add helper function to parse trace event to json object

[Steven Rostedt]

On Wed, 02 Nov 2022 14:20:09 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add the helper function that parses a trace event captured by
> libtraceevent in a tep handle. All the parsed fields are added to a json
> object. The json object is added to the provided list in the input parameter.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  cxl/event_trace.c |  166 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  cxl/event_trace.h |   14 ++++
>  cxl/meson.build   |    2 +
>  meson.build       |    1 
>  4 files changed, 183 insertions(+)
>  create mode 100644 cxl/event_trace.c
>  create mode 100644 cxl/event_trace.h
> 
> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> new file mode 100644
> index 000000000000..803df34452f3
> --- /dev/null
> +++ b/cxl/event_trace.c
> @@ -0,0 +1,166 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2022, Intel Corp. All rights reserved.
> +#include <stdio.h>
> +#include <json-c/json.h>
> +#include <util/json.h>
> +#include <util/util.h>
> +#include <util/parse-options.h>
> +#include <util/parse-configs.h>
> +#include <util/strbuf.h>
> +#include <util/sysfs.h>
> +#include <ccan/list/list.h>
> +#include <ndctl/ndctl.h>
> +#include <ndctl/libndctl.h>
> +#include <sys/epoll.h>
> +#include <sys/stat.h>
> +#include <libcxl.h>
> +#include <uuid/uuid.h>
> +#include <traceevent/event-parse.h>
> +#include "json.h"
> +#include "event_trace.h"
> +
> +#define _GNU_SOURCE
> +#include <string.h>
> +
> +static struct json_object *num_to_json(void *num, int size)
> +{
> +	if (size <= 4)
> +		return json_object_new_int(*(int *)num);
> +
> +	return util_json_object_hex(*(unsigned long long *)num, 0);
> +}
> +
> +static int cxl_event_to_json_callback(struct tep_event *event,
> +		struct tep_record *record, struct list_head *jlist_head)
> +{
> +	struct tep_format_field **fields;
> +	struct json_object *jevent, *jobj, *jarray;
> +	struct jlist_node *jnode;
> +	int i, j, rc = 0;
> +
> +	jnode = malloc(sizeof(*jnode));
> +	if (!jnode)
> +		return -ENOMEM;
> +
> +	jevent = json_object_new_object();
> +	if (!jevent) {
> +		rc = -ENOMEM;
> +		goto err_jevent;
> +	}
> +	jnode->jobj = jevent;
> +
> +	fields = tep_event_fields(event);
> +	if (!fields) {
> +		rc = -ENOENT;
> +		goto err;
> +	}
> +
> +	jobj = json_object_new_string(event->system);
> +	if (!jobj) {
> +		rc = -ENOMEM;
> +		goto err;
> +	}
> +	json_object_object_add(jevent, "system", jobj);
> +
> +	jobj = json_object_new_string(event->name);
> +	if (!jobj) {
> +		rc = -ENOMEM;
> +		goto err;
> +	}
> +	json_object_object_add(jevent, "event", jobj);
> +
> +	jobj = json_object_new_uint64(record->ts);
> +	if (!jobj) {
> +		rc = -ENOMEM;
> +		goto err;
> +	}
> +	json_object_object_add(jevent, "timestamp", jobj);
> +
> +	for (i = 0; fields[i]; i++) {
> +		struct tep_format_field *f = fields[i];
> +		int len;
> +		char *tmp;
> +
> +		tmp = strcasestr(f->type, "char[]");

Instead of looking for strings, you could use the field flags.

	f->flags & TEP_FIELD_IS_STRING

> +		if (tmp) { /* event field is a string */
> +			char *str;
> +
> +			str = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> +			if (!str)
> +				continue;
> +
> +			jobj = json_object_new_string(str);
> +			if (!jobj) {
> +				rc = -ENOMEM;
> +				goto err;
> +			}
> +
> +			json_object_object_add(jevent, f->name, jobj);
> +		} else if (f->arraylen) { /* data array */

		} else if (f->flags & TEP_FIELD_IS_ARRAY) {

 too.

> +			unsigned char *data;
> +			int chunks;
> +
> +			data = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> +			if (!data)
> +				continue;
> +
> +			jarray = json_object_new_array();
> +			if (!jarray) {
> +				rc = -ENOMEM;
> +				goto err;
> +			}
> +
> +			chunks = f->size / f->elementsize;
> +			for (j = 0; j < chunks; j++) {
> +				jobj = num_to_json(data, f->elementsize);
> +				if (!jobj) {
> +					json_object_put(jarray);
> +					return -ENOMEM;
> +				}
> +				json_object_array_add(jarray, jobj);
> +				data += f->elementsize;
> +			}
> +
> +			json_object_object_add(jevent, f->name, jarray);
> +		} else { /* single number */
> +			unsigned char *data;
> +
> +			data = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> +			if (!data)
> +				continue;
> +
> +			/* check to see if we have a UUID */
> +			tmp = strcasestr(f->type, "uuid_t");

I didn't even know event fields for uuid existed.

-- Steve

> +			if (tmp) {
> +				char uuid[SYSFS_ATTR_SIZE];
> +
> +				uuid_unparse(data, uuid);
> +				jobj = json_object_new_string(uuid);
> +				if (!jobj) {
> +					rc = -ENOMEM;
> +					goto err;
> +				}
> +
> +				json_object_object_add(jevent, f->name, jobj);
> +				continue;
> +			}
> +
> +			jobj = num_to_json(data, f->elementsize);
> +			if (!jobj) {
> +				rc = -ENOMEM;
> +				goto err;
> +			}
> +
> +			json_object_object_add(jevent, f->name, jobj);
> +		}
> +	}
> +
> +	list_add_tail(jlist_head, &jnode->list);
> +	return 0;
> +
> +err:
> +	json_object_put(jevent);
> +err_jevent:
> +	free(jnode);
> +	return rc;
> +}

Re: [PATCH v3 03/10] cxl: add common function to enable event trace

[Steven Rostedt]

On Wed, 02 Nov 2022 14:20:21 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add a common function for cxl command to enable event tracing for the
> instance created. The interested "systems" will be enabled for tracing
> as well.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  cxl/event_trace.c |   21 +++++++++++++++++++++
>  cxl/event_trace.h |    1 +
>  2 files changed, 22 insertions(+)
> 
> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> index cf63d2346f6e..d59e54c33df6 100644
> --- a/cxl/event_trace.c
> +++ b/cxl/event_trace.c
> @@ -200,3 +200,24 @@ int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx)
>  	tep_free(tep);
>  	return rc;
>  }
> +
> +int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system)
> +{
> +	int rc;
> +	char *path;
> +
> +	rc = asprintf(&path, "events/%s/enable", system);
> +	if (rc == -1)
> +		return -errno;
> +
> +	rc = tracefs_instance_file_write(inst, path, "1");
> +	free(path);
> +	if (rc == -1)
> +		return -errno;

Latest libtracefs has:

	tracefs_event_enable(inst, system, NULL);

That enables all events for a system in the instance "inst".

-- Steve


> +
> +	if (tracefs_trace_is_on(inst))
> +		return 0;
> +
> +	tracefs_trace_on(inst);
> +	return 0;
> +}
> diff --git a/cxl/event_trace.h b/cxl/event_trace.h
> index 582882c1eb35..0258b8dc65a3 100644
> --- a/cxl/event_trace.h
> +++ b/cxl/event_trace.h
> @@ -20,5 +20,6 @@ struct event_ctx {
>  };
>  
>  int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx);
> +int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system);
>  
>  #endif
> 

Re: [PATCH v3 04/10] cxl: add common function to disable event trace

[Steven Rostedt]

On Wed, 02 Nov 2022 14:20:26 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> Add a common function for cxl command that disables the event trace for the
> instance created.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  cxl/event_trace.c |    8 ++++++++
>  cxl/event_trace.h |    1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> index d59e54c33df6..bcd4f8b2968e 100644
> --- a/cxl/event_trace.c
> +++ b/cxl/event_trace.c
> @@ -221,3 +221,11 @@ int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system)
>  	tracefs_trace_on(inst);
>  	return 0;
>  }
> +
> +int cxl_event_tracing_disable(struct tracefs_instance *inst)
> +{
> +	if (!tracefs_trace_is_on(inst))
> +		return 0;

Why bother checking if tracing is on or not. It's not any more
efficient. You're not eliminating a system call. You are actually
adding another one.

-- Steve


> +
> +	return tracefs_trace_off(inst);
> +}
> diff --git a/cxl/event_trace.h b/cxl/event_trace.h
> index 0258b8dc65a3..17d922f922c1 100644
> --- a/cxl/event_trace.h
> +++ b/cxl/event_trace.h
> @@ -21,5 +21,6 @@ struct event_ctx {
>  
>  int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx);
>  int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system);
> +int cxl_event_tracing_disable(struct tracefs_instance *inst);
>  
>  #endif
> 

Re: [PATCH v3 05/10] cxl: add monitor function for event trace events

[Steven Rostedt]

On Wed, 02 Nov 2022 14:20:32 -0700
Dave Jiang <dave.jiang@intel.com> wrote:
> ---
>  cxl/meson.build |    1 
>  cxl/monitor.c   |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 141 insertions(+)
>  create mode 100644 cxl/monitor.c
> 
> diff --git a/cxl/meson.build b/cxl/meson.build
> index c59876262e76..eb8b2b1070ed 100644
> --- a/cxl/meson.build
> +++ b/cxl/meson.build
> @@ -8,6 +8,7 @@ cxl_src = [
>    'json.c',
>    'filter.c',
>    'event_trace.c',
> +  'monitor.c',
>  ]
>  
>  cxl_tool = executable('cxl',
> diff --git a/cxl/monitor.c b/cxl/monitor.c
> new file mode 100644
> index 000000000000..85559d9a4b94
> --- /dev/null
> +++ b/cxl/monitor.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2022, Intel Corp. All rights reserved.
> +/* Some bits copied from ndctl monitor code */
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <json-c/json.h>
> +#include <libgen.h>
> +#include <time.h>
> +#include <dirent.h>
> +#include <ccan/list/list.h>
> +#include <util/json.h>
> +#include <util/util.h>
> +#include <util/parse-options.h>
> +#include <util/parse-configs.h>
> +#include <util/strbuf.h>
> +#include <sys/epoll.h>
> +#include <sys/stat.h>
> +#include <traceevent/event-parse.h>
> +#include <tracefs/tracefs.h>
> +#include <cxl/libcxl.h>
> +
> +/* reuse the core log helpers for the monitor logger */
> +#ifndef ENABLE_LOGGING
> +#define ENABLE_LOGGING
> +#endif
> +#ifndef ENABLE_DEBUG
> +#define ENABLE_DEBUG
> +#endif
> +#include <util/log.h>
> +
> +#include "event_trace.h"
> +
> +static const char *cxl_system = "cxl";
> +
> +static struct monitor {
> +	struct log_ctx ctx;
> +	FILE *log_file;
> +	bool human;
> +} monitor;
> +
> +static int monitor_event(struct cxl_ctx *ctx)
> +{
> +	int fd, epollfd, rc = 0, timeout = -1;
> +	struct epoll_event ev, *events;
> +	struct tracefs_instance *inst;
> +	struct event_ctx ectx;
> +	int jflag;
> +
> +	events = calloc(1, sizeof(struct epoll_event));
> +	if (!events) {
> +		err(&monitor, "alloc for events error\n");
> +		return -ENOMEM;
> +	}
> +
> +	epollfd = epoll_create1(0);
> +	if (epollfd == -1) {
> +		rc = -errno;
> +		err(&monitor, "epoll_create1() error: %d\n", rc);
> +		goto epoll_err;
> +	}
> +
> +	inst = tracefs_instance_create("cxl_monitor");
> +	if (!inst) {
> +		rc = -errno;
> +		err(&monitor, "tracefs_instance_crate( failed: %d\n", rc);

 "crate"? Been coding a bit too much Rust lately?

> +		goto inst_err;
> +	}
> +
> +	fd = tracefs_instance_file_open(inst, "trace_pipe", -1);

I'm curious to why you are opening trace_pipe?

> +	if (fd < 0) {
> +		rc = fd;
> +		err(&monitor, "tracefs_instance_file_open() err: %d\n", rc);
> +		goto inst_file_err;
> +	}
> +
> +	memset(&ev, 0, sizeof(ev));
> +	ev.events = EPOLLIN;
> +	ev.data.fd = fd;

Is it a way to know if there's something to read?

-- Steve

> +
> +	if (epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev) != 0) {
> +		rc = -errno;
> +		err(&monitor, "epoll_ctl() error: %d\n", rc);
> +		goto epoll_ctl_err;
> +	}
> +
> +	rc = cxl_event_tracing_enable(inst, cxl_system);
> +	if (rc < 0) {
> +		err(&monitor, "cxl_trace_event_enable() failed: %d\n", rc);
> +		goto event_en_err;
> +	}
> +
> +	memset(&ectx, 0, sizeof(ectx));
> +	ectx.system = cxl_system;
> +	if (monitor.human)
> +		jflag = JSON_C_TO_STRING_PRETTY;
> +	else
> +		jflag = JSON_C_TO_STRING_PLAIN;
> +
> +	while (1) {
> +		struct jlist_node *jnode, *next;
> +
> +		rc = epoll_wait(epollfd, events, 1, timeout);
> +		if (rc < 0) {
> +			rc = -errno;
> +			if (errno != EINTR)
> +				err(&monitor, "epoll_wait error: %d\n", -errno);
> +			break;
> +		}
> +
> +		list_head_init(&ectx.jlist_head);
> +		rc = cxl_parse_events(inst, &ectx);
> +		if (rc < 0)
> +			goto parse_err;
> +
> +		if (list_empty(&ectx.jlist_head))
> +			continue;
> +
> +		list_for_each_safe(&ectx.jlist_head, jnode, next, list) {
> +			notice(&monitor, "%s\n",
> +				json_object_to_json_string_ext(jnode->jobj, jflag));
> +			list_del(&jnode->list);
> +			json_object_put(jnode->jobj);
> +			free(jnode);
> +		}
> +	}
> +
> +parse_err:
> +	rc = cxl_event_tracing_disable(inst);
> +event_en_err:
> +epoll_ctl_err:
> +	close(fd);
> +inst_file_err:
> +	tracefs_instance_free(inst);
> +inst_err:
> +	close(epollfd);
> +epoll_err:
> +	free(events);
> +	return rc;
> +}
> 

Re: [PATCH v3 08/10] cxl: add an optional pid check to event parsing

[Steven Rostedt]

On Wed, 02 Nov 2022 14:20:49 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> From: Alison Schofield <alison.schofield@intel.com>
> 
> When parsing CXL events, callers may only be interested in events
> that originate from the current process. Introduce an optional
> argument to the event trace context: event_pid. When event_pid is
> present, only include events with a matching pid in the returned
> JSON list. It is not a failure to see other, non matching results.
> Simply skip those.
> 
> The initial use case for this is the listing of media errors,
> where only the media-errors requested by this process are wanted.
> 
> Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  cxl/event_trace.c |   18 ++++++++++++++++++
>  cxl/event_trace.h |    1 +
>  2 files changed, 19 insertions(+)
> 
> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> index bcd4f8b2968e..0be6317e6ada 100644
> --- a/cxl/event_trace.c
> +++ b/cxl/event_trace.c
> @@ -166,6 +166,19 @@ err_jevent:
>  	return rc;
>  }
>  
> +static bool cxl_match_pid(struct tep_event *event, struct tep_record *record,
> +			  int pid)
> +{
> +	unsigned long long val;
> +
> +	if (tep_get_common_field_val(NULL, event, "common_pid", record, &val, 0))

There's also a way to get the pid directly:

   pid = tep_data_pid(tep, record);

-- Steve

> +		return false;
> +	if (pid != (int)val)
> +		return false;
> +
> +	return true;
> +}
> +
>  static int cxl_event_parse_cb(struct tep_event *event, struct tep_record *record,
>  		int cpu, void *ctx)
>  {
> @@ -180,6 +193,11 @@ static int cxl_event_parse_cb(struct tep_event *event, struct tep_record *record
>  			return 0;
>  	}
>  
> +	if (event_ctx->event_pid) {
> +		if (!cxl_match_pid(event, record, event_ctx->event_pid))
> +			return 0;
> +	}
> +
>  	if (event_ctx->parse_event)
>  		return event_ctx->parse_event(event, record, &event_ctx->jlist_head);
>  
> diff --git a/cxl/event_trace.h b/cxl/event_trace.h
> index 17d922f922c1..64f07854b91b 100644
> --- a/cxl/event_trace.h
> +++ b/cxl/event_trace.h
> @@ -15,6 +15,7 @@ struct event_ctx {
>  	const char *system;
>  	struct list_head jlist_head;
>  	const char *event_name;					/* optional */
> +	int event_pid;						/* optional */
>  	int (*parse_event)(struct tep_event *event, struct tep_record *record,
>  			   struct list_head *jlist_head);	/* optional */
>  };
> 

Re: [PATCH v3 08/10] cxl: add an optional pid check to event parsing

[Alison Schofield]

On Thu, Nov 03, 2022 at 02:08:19AM -0400, Steven Rostedt wrote:
> On Wed, 02 Nov 2022 14:20:49 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > When parsing CXL events, callers may only be interested in events
> > that originate from the current process. Introduce an optional
> > argument to the event trace context: event_pid. When event_pid is
> > present, only include events with a matching pid in the returned
> > JSON list. It is not a failure to see other, non matching results.
> > Simply skip those.
> > 
> > The initial use case for this is the listing of media errors,
> > where only the media-errors requested by this process are wanted.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > ---
> >  cxl/event_trace.c |   18 ++++++++++++++++++
> >  cxl/event_trace.h |    1 +
> >  2 files changed, 19 insertions(+)
> > 
> > diff --git a/cxl/event_trace.c b/cxl/event_trace.c
> > index bcd4f8b2968e..0be6317e6ada 100644
> > --- a/cxl/event_trace.c
> > +++ b/cxl/event_trace.c
> > @@ -166,6 +166,19 @@ err_jevent:
> >  	return rc;
> >  }
> >  
> > +static bool cxl_match_pid(struct tep_event *event, struct tep_record *record,
> > +			  int pid)
> > +{
> > +	unsigned long long val;
> > +
> > +	if (tep_get_common_field_val(NULL, event, "common_pid", record, &val, 0))
> 
> There's also a way to get the pid directly:
> 
>    pid = tep_data_pid(tep, record);

I saw that and thought I'd lost the required tep_handle a couple of
layers back. Of course, it's in the tep_event too!
Will do, Thanks!

> 
> -- Steve
> 
> > +		return false;
> > +	if (pid != (int)val)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  static int cxl_event_parse_cb(struct tep_event *event, struct tep_record *record,
> >  		int cpu, void *ctx)
> >  {
> > @@ -180,6 +193,11 @@ static int cxl_event_parse_cb(struct tep_event *event, struct tep_record *record
> >  			return 0;
> >  	}
> >  
> > +	if (event_ctx->event_pid) {
> > +		if (!cxl_match_pid(event, record, event_ctx->event_pid))
> > +			return 0;
> > +	}
> > +
> >  	if (event_ctx->parse_event)
> >  		return event_ctx->parse_event(event, record, &event_ctx->jlist_head);
> >  
> > diff --git a/cxl/event_trace.h b/cxl/event_trace.h
> > index 17d922f922c1..64f07854b91b 100644
> > --- a/cxl/event_trace.h
> > +++ b/cxl/event_trace.h
> > @@ -15,6 +15,7 @@ struct event_ctx {
> >  	const char *system;
> >  	struct list_head jlist_head;
> >  	const char *event_name;					/* optional */
> > +	int event_pid;						/* optional */
> >  	int (*parse_event)(struct tep_event *event, struct tep_record *record,
> >  			   struct list_head *jlist_head);	/* optional */
> >  };
> > 
> 

Re: [PATCH v3 01/10] cxl: add helper function to parse trace event to json object

[Dave Jiang]

On 11/2/2022 10:48 PM, Steven Rostedt wrote:
> On Wed, 02 Nov 2022 14:20:09 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add the helper function that parses a trace event captured by
>> libtraceevent in a tep handle. All the parsed fields are added to a json
>> object. The json object is added to the provided list in the input parameter.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   cxl/event_trace.c |  166 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   cxl/event_trace.h |   14 ++++
>>   cxl/meson.build   |    2 +
>>   meson.build       |    1
>>   4 files changed, 183 insertions(+)
>>   create mode 100644 cxl/event_trace.c
>>   create mode 100644 cxl/event_trace.h
>>
>> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
>> new file mode 100644
>> index 000000000000..803df34452f3
>> --- /dev/null
>> +++ b/cxl/event_trace.c
>> @@ -0,0 +1,166 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2022, Intel Corp. All rights reserved.
>> +#include <stdio.h>
>> +#include <json-c/json.h>
>> +#include <util/json.h>
>> +#include <util/util.h>
>> +#include <util/parse-options.h>
>> +#include <util/parse-configs.h>
>> +#include <util/strbuf.h>
>> +#include <util/sysfs.h>
>> +#include <ccan/list/list.h>
>> +#include <ndctl/ndctl.h>
>> +#include <ndctl/libndctl.h>
>> +#include <sys/epoll.h>
>> +#include <sys/stat.h>
>> +#include <libcxl.h>
>> +#include <uuid/uuid.h>
>> +#include <traceevent/event-parse.h>
>> +#include "json.h"
>> +#include "event_trace.h"
>> +
>> +#define _GNU_SOURCE
>> +#include <string.h>
>> +
>> +static struct json_object *num_to_json(void *num, int size)
>> +{
>> +	if (size <= 4)
>> +		return json_object_new_int(*(int *)num);
>> +
>> +	return util_json_object_hex(*(unsigned long long *)num, 0);
>> +}
>> +
>> +static int cxl_event_to_json_callback(struct tep_event *event,
>> +		struct tep_record *record, struct list_head *jlist_head)
>> +{
>> +	struct tep_format_field **fields;
>> +	struct json_object *jevent, *jobj, *jarray;
>> +	struct jlist_node *jnode;
>> +	int i, j, rc = 0;
>> +
>> +	jnode = malloc(sizeof(*jnode));
>> +	if (!jnode)
>> +		return -ENOMEM;
>> +
>> +	jevent = json_object_new_object();
>> +	if (!jevent) {
>> +		rc = -ENOMEM;
>> +		goto err_jevent;
>> +	}
>> +	jnode->jobj = jevent;
>> +
>> +	fields = tep_event_fields(event);
>> +	if (!fields) {
>> +		rc = -ENOENT;
>> +		goto err;
>> +	}
>> +
>> +	jobj = json_object_new_string(event->system);
>> +	if (!jobj) {
>> +		rc = -ENOMEM;
>> +		goto err;
>> +	}
>> +	json_object_object_add(jevent, "system", jobj);
>> +
>> +	jobj = json_object_new_string(event->name);
>> +	if (!jobj) {
>> +		rc = -ENOMEM;
>> +		goto err;
>> +	}
>> +	json_object_object_add(jevent, "event", jobj);
>> +
>> +	jobj = json_object_new_uint64(record->ts);
>> +	if (!jobj) {
>> +		rc = -ENOMEM;
>> +		goto err;
>> +	}
>> +	json_object_object_add(jevent, "timestamp", jobj);
>> +
>> +	for (i = 0; fields[i]; i++) {
>> +		struct tep_format_field *f = fields[i];
>> +		int len;
>> +		char *tmp;
>> +
>> +		tmp = strcasestr(f->type, "char[]");
> 
> Instead of looking for strings, you could use the field flags.
> 
> 	f->flags & TEP_FIELD_IS_STRING

Thanks for the review Steve. This is good to know! I will update.

> 
>> +		if (tmp) { /* event field is a string */
>> +			char *str;
>> +
>> +			str = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
>> +			if (!str)
>> +				continue;
>> +
>> +			jobj = json_object_new_string(str);
>> +			if (!jobj) {
>> +				rc = -ENOMEM;
>> +				goto err;
>> +			}
>> +
>> +			json_object_object_add(jevent, f->name, jobj);
>> +		} else if (f->arraylen) { /* data array */
> 
> 		} else if (f->flags & TEP_FIELD_IS_ARRAY) {
> 
>   too.

Will update.

> 
>> +			unsigned char *data;
>> +			int chunks;
>> +
>> +			data = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
>> +			if (!data)
>> +				continue;
>> +
>> +			jarray = json_object_new_array();
>> +			if (!jarray) {
>> +				rc = -ENOMEM;
>> +				goto err;
>> +			}
>> +
>> +			chunks = f->size / f->elementsize;
>> +			for (j = 0; j < chunks; j++) {
>> +				jobj = num_to_json(data, f->elementsize);
>> +				if (!jobj) {
>> +					json_object_put(jarray);
>> +					return -ENOMEM;
>> +				}
>> +				json_object_array_add(jarray, jobj);
>> +				data += f->elementsize;
>> +			}
>> +
>> +			json_object_object_add(jevent, f->name, jarray);
>> +		} else { /* single number */
>> +			unsigned char *data;
>> +
>> +			data = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
>> +			if (!data)
>> +				continue;
>> +
>> +			/* check to see if we have a UUID */
>> +			tmp = strcasestr(f->type, "uuid_t");
> 
> I didn't even know event fields for uuid existed.

Ira figured out that you can you can use __field_struct() to accommodate 
uuid_t. Here's what he changed below:

diff --git a/include/trace/events/cxl.h b/include/trace/events/cxl.h
index 6777a1119e68..22fcd677b6d0 100644
--- a/include/trace/events/cxl.h
+++ b/include/trace/events/cxl.h
@@ -68,7 +68,7 @@ TRACE_EVENT(cxl_overflow,
#define CXL_EVT_TP_entry                                       \
         __string(dev_name, dev_name)                            \
         __field(int, log)                                       \
-       __array(u8, hdr_uuid, UUID_SIZE)                        \
+       __field_struct(uuid_t, hdr_uuid)                        \
         __field(u32, hdr_flags)                                 \
         __field(u16, hdr_handle)                                \
         __field(u16, hdr_related_handle)                        \
@@ -79,7 +79,7 @@ TRACE_EVENT(cxl_overflow,
#define CXL_EVT_TP_fast_assign(dname, l, hdr)                   \
         __assign_str(dev_name, (dname));                        \
         __entry->log = (l);                                     \
-       memcpy(__entry->hdr_uuid, &(hdr).id, UUID_SIZE);        \
+       memcpy(&__entry->hdr_uuid, &(hdr).id, sizeof(uuid_t));  \
         __entry->hdr_length = (hdr).length;                     \
         __entry->hdr_flags = get_unaligned_le24((hdr).flags);   \
         __entry->hdr_handle = le16_to_cpu((hdr).handle);        \
@@ -93,7 +93,7 @@ TRACE_EVENT(cxl_overflow,
                 "handle=%x related_handle=%x maint_op_class=%u" \
                 " : " fmt,                                      \
                __get_str(dev_name),cxl_event_log_type_str(__entry->log), \
-               __entry->hdr_timestamp, __entry->hdr_uuid, 
__entry->hdr_length, \
+               __entry->hdr_timestamp, &__entry->hdr_uuid, 
__entry->hdr_length,        \
                 show_hdr_flags(__entry->hdr_flags), 
__entry->hdr_handle,        \
                 __entry->hdr_related_handle, 
__entry->hdr_maint_op_class,       \
                 ##__VA_ARGS__)

> 
> -- Steve
> 
>> +			if (tmp) {
>> +				char uuid[SYSFS_ATTR_SIZE];
>> +
>> +				uuid_unparse(data, uuid);
>> +				jobj = json_object_new_string(uuid);
>> +				if (!jobj) {
>> +					rc = -ENOMEM;
>> +					goto err;
>> +				}
>> +
>> +				json_object_object_add(jevent, f->name, jobj);
>> +				continue;
>> +			}
>> +
>> +			jobj = num_to_json(data, f->elementsize);
>> +			if (!jobj) {
>> +				rc = -ENOMEM;
>> +				goto err;
>> +			}
>> +
>> +			json_object_object_add(jevent, f->name, jobj);
>> +		}
>> +	}
>> +
>> +	list_add_tail(jlist_head, &jnode->list);
>> +	return 0;
>> +
>> +err:
>> +	json_object_put(jevent);
>> +err_jevent:
>> +	free(jnode);
>> +	return rc;
>> +}

Re: [PATCH v3 03/10] cxl: add common function to enable event trace

[Dave Jiang]

On 11/2/2022 11:00 PM, Steven Rostedt wrote:
> On Wed, 02 Nov 2022 14:20:21 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add a common function for cxl command to enable event tracing for the
>> instance created. The interested "systems" will be enabled for tracing
>> as well.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   cxl/event_trace.c |   21 +++++++++++++++++++++
>>   cxl/event_trace.h |    1 +
>>   2 files changed, 22 insertions(+)
>>
>> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
>> index cf63d2346f6e..d59e54c33df6 100644
>> --- a/cxl/event_trace.c
>> +++ b/cxl/event_trace.c
>> @@ -200,3 +200,24 @@ int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx)
>>   	tep_free(tep);
>>   	return rc;
>>   }
>> +
>> +int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system)
>> +{
>> +	int rc;
>> +	char *path;
>> +
>> +	rc = asprintf(&path, "events/%s/enable", system);
>> +	if (rc == -1)
>> +		return -errno;
>> +
>> +	rc = tracefs_instance_file_write(inst, path, "1");
>> +	free(path);
>> +	if (rc == -1)
>> +		return -errno;
> 
> Latest libtracefs has:
> 
> 	tracefs_event_enable(inst, system, NULL);
> 
> That enables all events for a system in the instance "inst".

Great. Thanks! I will switch to that.

> 
> -- Steve
> 
> 
>> +
>> +	if (tracefs_trace_is_on(inst))
>> +		return 0;
>> +
>> +	tracefs_trace_on(inst);
>> +	return 0;
>> +}
>> diff --git a/cxl/event_trace.h b/cxl/event_trace.h
>> index 582882c1eb35..0258b8dc65a3 100644
>> --- a/cxl/event_trace.h
>> +++ b/cxl/event_trace.h
>> @@ -20,5 +20,6 @@ struct event_ctx {
>>   };
>>   
>>   int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx);
>> +int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system);
>>   
>>   #endif
>>
> 

Re: [PATCH v3 04/10] cxl: add common function to disable event trace

[Dave Jiang]

On 11/2/2022 11:02 PM, Steven Rostedt wrote:
> On Wed, 02 Nov 2022 14:20:26 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> Add a common function for cxl command that disables the event trace for the
>> instance created.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>> ---
>>   cxl/event_trace.c |    8 ++++++++
>>   cxl/event_trace.h |    1 +
>>   2 files changed, 9 insertions(+)
>>
>> diff --git a/cxl/event_trace.c b/cxl/event_trace.c
>> index d59e54c33df6..bcd4f8b2968e 100644
>> --- a/cxl/event_trace.c
>> +++ b/cxl/event_trace.c
>> @@ -221,3 +221,11 @@ int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system)
>>   	tracefs_trace_on(inst);
>>   	return 0;
>>   }
>> +
>> +int cxl_event_tracing_disable(struct tracefs_instance *inst)
>> +{
>> +	if (!tracefs_trace_is_on(inst))
>> +		return 0;
> 
> Why bother checking if tracing is on or not. It's not any more
> efficient. You're not eliminating a system call. You are actually
> adding another one.

Ok, will remove.
> 
> -- Steve
> 
> 
>> +
>> +	return tracefs_trace_off(inst);
>> +}
>> diff --git a/cxl/event_trace.h b/cxl/event_trace.h
>> index 0258b8dc65a3..17d922f922c1 100644
>> --- a/cxl/event_trace.h
>> +++ b/cxl/event_trace.h
>> @@ -21,5 +21,6 @@ struct event_ctx {
>>   
>>   int cxl_parse_events(struct tracefs_instance *inst, struct event_ctx *ectx);
>>   int cxl_event_tracing_enable(struct tracefs_instance *inst, const char *system);
>> +int cxl_event_tracing_disable(struct tracefs_instance *inst);
>>   
>>   #endif
>>
> 

Re: [PATCH v3 05/10] cxl: add monitor function for event trace events

[Dave Jiang]

On 11/2/2022 11:06 PM, Steven Rostedt wrote:
> On Wed, 02 Nov 2022 14:20:32 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
>> ---
>>   cxl/meson.build |    1
>>   cxl/monitor.c   |  140 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 141 insertions(+)
>>   create mode 100644 cxl/monitor.c
>>
>> diff --git a/cxl/meson.build b/cxl/meson.build
>> index c59876262e76..eb8b2b1070ed 100644
>> --- a/cxl/meson.build
>> +++ b/cxl/meson.build
>> @@ -8,6 +8,7 @@ cxl_src = [
>>     'json.c',
>>     'filter.c',
>>     'event_trace.c',
>> +  'monitor.c',
>>   ]
>>   
>>   cxl_tool = executable('cxl',
>> diff --git a/cxl/monitor.c b/cxl/monitor.c
>> new file mode 100644
>> index 000000000000..85559d9a4b94
>> --- /dev/null
>> +++ b/cxl/monitor.c
>> @@ -0,0 +1,140 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2022, Intel Corp. All rights reserved.
>> +/* Some bits copied from ndctl monitor code */
>> +#include <stdio.h>
>> +#include <unistd.h>
>> +#include <errno.h>
>> +#include <json-c/json.h>
>> +#include <libgen.h>
>> +#include <time.h>
>> +#include <dirent.h>
>> +#include <ccan/list/list.h>
>> +#include <util/json.h>
>> +#include <util/util.h>
>> +#include <util/parse-options.h>
>> +#include <util/parse-configs.h>
>> +#include <util/strbuf.h>
>> +#include <sys/epoll.h>
>> +#include <sys/stat.h>
>> +#include <traceevent/event-parse.h>
>> +#include <tracefs/tracefs.h>
>> +#include <cxl/libcxl.h>
>> +
>> +/* reuse the core log helpers for the monitor logger */
>> +#ifndef ENABLE_LOGGING
>> +#define ENABLE_LOGGING
>> +#endif
>> +#ifndef ENABLE_DEBUG
>> +#define ENABLE_DEBUG
>> +#endif
>> +#include <util/log.h>
>> +
>> +#include "event_trace.h"
>> +
>> +static const char *cxl_system = "cxl";
>> +
>> +static struct monitor {
>> +	struct log_ctx ctx;
>> +	FILE *log_file;
>> +	bool human;
>> +} monitor;
>> +
>> +static int monitor_event(struct cxl_ctx *ctx)
>> +{
>> +	int fd, epollfd, rc = 0, timeout = -1;
>> +	struct epoll_event ev, *events;
>> +	struct tracefs_instance *inst;
>> +	struct event_ctx ectx;
>> +	int jflag;
>> +
>> +	events = calloc(1, sizeof(struct epoll_event));
>> +	if (!events) {
>> +		err(&monitor, "alloc for events error\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	epollfd = epoll_create1(0);
>> +	if (epollfd == -1) {
>> +		rc = -errno;
>> +		err(&monitor, "epoll_create1() error: %d\n", rc);
>> +		goto epoll_err;
>> +	}
>> +
>> +	inst = tracefs_instance_create("cxl_monitor");
>> +	if (!inst) {
>> +		rc = -errno;
>> +		err(&monitor, "tracefs_instance_crate( failed: %d\n", rc);
> 
>   "crate"? Been coding a bit too much Rust lately?

Ooops. Will fix.

> 
>> +		goto inst_err;
>> +	}
>> +
>> +	fd = tracefs_instance_file_open(inst, "trace_pipe", -1);
> 
> I'm curious to why you are opening trace_pipe?
> 
>> +	if (fd < 0) {
>> +		rc = fd;
>> +		err(&monitor, "tracefs_instance_file_open() err: %d\n", rc);
>> +		goto inst_file_err;
>> +	}
>> +
>> +	memset(&ev, 0, sizeof(ev));
>> +	ev.events = EPOLLIN;
>> +	ev.data.fd = fd;
> 
> Is it a way to know if there's something to read?

Yes. Since trace_pipe() is the read once data stream, seems like the 
right place to put the epoll on. Is that the wrong way to do this?

> 
> -- Steve
> 
>> +
>> +	if (epoll_ctl(epollfd, EPOLL_CTL_ADD, fd, &ev) != 0) {
>> +		rc = -errno;
>> +		err(&monitor, "epoll_ctl() error: %d\n", rc);
>> +		goto epoll_ctl_err;
>> +	}
>> +
>> +	rc = cxl_event_tracing_enable(inst, cxl_system);
>> +	if (rc < 0) {
>> +		err(&monitor, "cxl_trace_event_enable() failed: %d\n", rc);
>> +		goto event_en_err;
>> +	}
>> +
>> +	memset(&ectx, 0, sizeof(ectx));
>> +	ectx.system = cxl_system;
>> +	if (monitor.human)
>> +		jflag = JSON_C_TO_STRING_PRETTY;
>> +	else
>> +		jflag = JSON_C_TO_STRING_PLAIN;
>> +
>> +	while (1) {
>> +		struct jlist_node *jnode, *next;
>> +
>> +		rc = epoll_wait(epollfd, events, 1, timeout);
>> +		if (rc < 0) {
>> +			rc = -errno;
>> +			if (errno != EINTR)
>> +				err(&monitor, "epoll_wait error: %d\n", -errno);
>> +			break;
>> +		}
>> +
>> +		list_head_init(&ectx.jlist_head);
>> +		rc = cxl_parse_events(inst, &ectx);
>> +		if (rc < 0)
>> +			goto parse_err;
>> +
>> +		if (list_empty(&ectx.jlist_head))
>> +			continue;
>> +
>> +		list_for_each_safe(&ectx.jlist_head, jnode, next, list) {
>> +			notice(&monitor, "%s\n",
>> +				json_object_to_json_string_ext(jnode->jobj, jflag));
>> +			list_del(&jnode->list);
>> +			json_object_put(jnode->jobj);
>> +			free(jnode);
>> +		}
>> +	}
>> +
>> +parse_err:
>> +	rc = cxl_event_tracing_disable(inst);
>> +event_en_err:
>> +epoll_ctl_err:
>> +	close(fd);
>> +inst_file_err:
>> +	tracefs_instance_free(inst);
>> +inst_err:
>> +	close(epollfd);
>> +epoll_err:
>> +	free(events);
>> +	return rc;
>> +}
>>
> 

Re: [PATCH v3 01/10] cxl: add helper function to parse trace event to json object

[Ira Weiny]

On Thu, Nov 03, 2022 at 09:15:03AM -0700, Jiang, Dave wrote:
> 
> 
> On 11/2/2022 10:48 PM, Steven Rostedt wrote:
> > On Wed, 02 Nov 2022 14:20:09 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> > 
> > > Add the helper function that parses a trace event captured by
> > > libtraceevent in a tep handle. All the parsed fields are added to a json
> > > object. The json object is added to the provided list in the input parameter.
> > > 
> > > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > > ---
> > >   cxl/event_trace.c |  166 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >   cxl/event_trace.h |   14 ++++

[snip]

> > > +		} else { /* single number */
> > > +			unsigned char *data;
> > > +
> > > +			data = tep_get_field_raw(NULL, event, f->name, record, &len, 0);
> > > +			if (!data)
> > > +				continue;
> > > +
> > > +			/* check to see if we have a UUID */
> > > +			tmp = strcasestr(f->type, "uuid_t");
> > 
> > I didn't even know event fields for uuid existed.
> 
> Ira figured out that you can you can use __field_struct() to accommodate
> uuid_t. Here's what he changed below:

I guess this is a hack.  I looked into creating a new entry specifier and saw
that __field_struct did what I needed.

It all seems to work fine until Alison pointed out this morning that trace-cmd
can't seem to parse this.

:-/

So now I'm not sure what is technically correct.  I've never used trace-cmd
before and this all works fine on the cmd line and with this monitor command.

Is this messing up trace-cmd?

Ira

> 
> diff --git a/include/trace/events/cxl.h b/include/trace/events/cxl.h
> index 6777a1119e68..22fcd677b6d0 100644
> --- a/include/trace/events/cxl.h
> +++ b/include/trace/events/cxl.h
> @@ -68,7 +68,7 @@ TRACE_EVENT(cxl_overflow,
> #define CXL_EVT_TP_entry                                       \
>         __string(dev_name, dev_name)                            \
>         __field(int, log)                                       \
> -       __array(u8, hdr_uuid, UUID_SIZE)                        \
> +       __field_struct(uuid_t, hdr_uuid)                        \
>         __field(u32, hdr_flags)                                 \
>         __field(u16, hdr_handle)                                \
>         __field(u16, hdr_related_handle)                        \
> @@ -79,7 +79,7 @@ TRACE_EVENT(cxl_overflow,
> #define CXL_EVT_TP_fast_assign(dname, l, hdr)                   \
>         __assign_str(dev_name, (dname));                        \
>         __entry->log = (l);                                     \
> -       memcpy(__entry->hdr_uuid, &(hdr).id, UUID_SIZE);        \
> +       memcpy(&__entry->hdr_uuid, &(hdr).id, sizeof(uuid_t));  \
>         __entry->hdr_length = (hdr).length;                     \
>         __entry->hdr_flags = get_unaligned_le24((hdr).flags);   \
>         __entry->hdr_handle = le16_to_cpu((hdr).handle);        \
> @@ -93,7 +93,7 @@ TRACE_EVENT(cxl_overflow,
>                 "handle=%x related_handle=%x maint_op_class=%u" \
>                 " : " fmt,                                      \
>                __get_str(dev_name),cxl_event_log_type_str(__entry->log), \
> -               __entry->hdr_timestamp, __entry->hdr_uuid,
> __entry->hdr_length, \
> +               __entry->hdr_timestamp, &__entry->hdr_uuid,
> __entry->hdr_length,        \
>                 show_hdr_flags(__entry->hdr_flags), __entry->hdr_handle,
> \
>                 __entry->hdr_related_handle, __entry->hdr_maint_op_class,
> \
>                 ##__VA_ARGS__)
> 

Re: [PATCH v3 01/10] cxl: add helper function to parse trace event to json object

[Steven Rostedt]

On Fri, 4 Nov 2022 11:56:46 -0700
Ira Weiny <ira.weiny@intel.com> wrote:

> > Ira figured out that you can you can use __field_struct() to accommodate
> > uuid_t. Here's what he changed below:  
> 
> I guess this is a hack.  I looked into creating a new entry specifier and saw
> that __field_struct did what I needed.
> 
> It all seems to work fine until Alison pointed out this morning that trace-cmd
> can't seem to parse this.
> 
> :-/

If it works in a trace file, it should work in trace-cmd. If it does not,
then trace-cmd needs to be fixed (unless it needs more information from the
kernel, which it might).

> 
> So now I'm not sure what is technically correct.  I've never used trace-cmd
> before and this all works fine on the cmd line and with this monitor command.
> 
> Is this messing up trace-cmd?

If you see an issue, would you be able to send me a trace.dat file that
includes the failed parsing?

That is:

 trace-cmd record -e <your-event>

Do something to trigger your event.

Then send me the produced trace.dat file that fails to parse from the
 trace-cmd report.

Thanks!

-- Steve

Re: [PATCH v3 01/10] cxl: add helper function to parse trace event to json object

[Alison Schofield]

On Mon, Nov 14, 2022 at 12:32:38PM -0500, Steven Rostedt wrote:
> On Fri, 4 Nov 2022 11:56:46 -0700
> Ira Weiny <ira.weiny@intel.com> wrote:
> 
> > > Ira figured out that you can you can use __field_struct() to accommodate
> > > uuid_t. Here's what he changed below:  
> > 
> > I guess this is a hack.  I looked into creating a new entry specifier and saw
> > that __field_struct did what I needed.
> > 
> > It all seems to work fine until Alison pointed out this morning that trace-cmd
> > can't seem to parse this.
> > 
> > :-/
> 
> If it works in a trace file, it should work in trace-cmd. If it does not,
> then trace-cmd needs to be fixed (unless it needs more information from the
> kernel, which it might).
> 
> > 
> > So now I'm not sure what is technically correct.  I've never used trace-cmd
> > before and this all works fine on the cmd line and with this monitor command.
> > 
> > Is this messing up trace-cmd?
> 
> If you see an issue, would you be able to send me a trace.dat file that
> includes the failed parsing?
> 
> That is:
> 
>  trace-cmd record -e <your-event>
> 
> Do something to trigger your event.
> 
> Then send me the produced trace.dat file that fails to parse from the
>  trace-cmd report.

Hi Steve,

I tried to use the uuid_t type in another TRACE event (cxl_poison), and
ran into the problem mentioned above.

It seems to be a problem in libtraceevent (or a user error ;))

When we have:
	TP_STRUCT__entry
		__field_struct(uuid_t, hdr_uuid)

	TP__fast_assign
		memcpy(&__entry->hdr.uuid, &(hdr).id, sizeof(uuid_t));

	TP_printk("uuid= %pU",  &__entry->hdr.uuid)

Libtraceevent fails to handle that "&" in "&__entry->hrd.uuid" when
creating the pretty print string. It fails in process_op() and then
later in pretty_print() prefaces the string w '[[FAILED TO PARSE]"
follwed by the raw values.

If I omit that '&', it works. But - I get unacceptable compiler
warnings from the kernel build - trying to assign to %p.

So, it's really not 'trace-cmd' per se that is failing, that is
just where the failure is visible.

The 'other' way that we view these CXL events are through monitor
and ndctl, where we pull the fields directly from the event, and
don't care about that TP_printk string. uuid_t works fine in that
case.

CXL events were not the first to use uuid_t. I see events/afs.h
using it also. I don't see a difference in their syntax, that would
makes it work there, while CXL fails. Maybe you do.

I don't have the environment up at the moment, so I'm just spewing
from notes and memory. If you need actual duplication, ask me again.

Thanks for helping w this Steve!

Alison

-- Steve



> 
> Thanks!
> 
> -- Steve

Re: [PATCH v3 01/10] cxl: add helper function to parse trace event to json object

[Steven Rostedt]

On Mon, 14 Nov 2022 12:44:51 -0800
Alison Schofield <alison.schofield@intel.com> wrote:

> Hi Steve,
> 
> I tried to use the uuid_t type in another TRACE event (cxl_poison), and
> ran into the problem mentioned above.
> 
> It seems to be a problem in libtraceevent (or a user error ;))

Correct. When I said trace-cmd, I really meant libtraceevent, but I tend to
blur the two as libtracevenet was born out of trace-cmd.

> 
> When we have:
> 	TP_STRUCT__entry
> 		__field_struct(uuid_t, hdr_uuid)
> 
> 	TP__fast_assign
> 		memcpy(&__entry->hdr.uuid, &(hdr).id, sizeof(uuid_t));
> 
> 	TP_printk("uuid= %pU",  &__entry->hdr.uuid)
> 
> Libtraceevent fails to handle that "&" in "&__entry->hrd.uuid" when
> creating the pretty print string. It fails in process_op() and then
> later in pretty_print() prefaces the string w '[[FAILED TO PARSE]"
> follwed by the raw values.
> 
> If I omit that '&', it works. But - I get unacceptable compiler
> warnings from the kernel build - trying to assign to %p.
> 
> So, it's really not 'trace-cmd' per se that is failing, that is
> just where the failure is visible.

Right.

> 
> The 'other' way that we view these CXL events are through monitor
> and ndctl, where we pull the fields directly from the event, and
> don't care about that TP_printk string. uuid_t works fine in that
> case.
> 
> CXL events were not the first to use uuid_t. I see events/afs.h
> using it also. I don't see a difference in their syntax, that would
> makes it work there, while CXL fails. Maybe you do.

I'm guessing they don't work ;-)

> 
> I don't have the environment up at the moment, so I'm just spewing
> from notes and memory. If you need actual duplication, ask me again.
> 
> Thanks for helping w this Steve!
> 

No problem.

I'll can add this to a test event and then update libtraceevent to handle
it. I'm currently working on a new release of the libraries anyway.

-- Steve