[PATCH V4 1/3] tracing: add a possibility of exporting function trace to other places instead of ring buffer only

On Mon, 15 Aug 2016 19:50:01 +0800
Chunyan Zhang [off-list ref] wrote:

Why is this marked inline? It is never called directly here.

+{
+	struct trace_entry *entry;
+	struct trace_export *export = tr->export;
+	unsigned int size = 0;
+
+	entry = ring_buffer_event_data(event);
+
+	trace_entry_size(size, entry->type);
+	if (!size)
+		return;
+
+	if (export->write)
+		export->write((char *)entry, size);
+}
+
+static inline void

Same here.

+trace_rb_commit(struct trace_array *tr,
+	       struct ring_buffer_event *event)
+{
+	__buffer_unlock_commit(tr->trace_buffer.buffer, event);
+}
+
+static DEFINE_MUTEX(trace_export_lock);
+
+static struct trace_export trace_export_rb __read_mostly = {
+	.name		= "rb",
+	.commit	= trace_rb_commit,

Make sure equal signs are lined up.

+	.next		= NULL,
+};
+static struct trace_export *trace_exports_list __read_mostly = &trace_export_rb;

trace_exports_list needs to be annotated as rcu, if you are going to
dereference it via rcu_dereference. That was the kbuild warning you
received.

+
+inline void
+trace_exports(struct trace_array *tr, struct ring_buffer_event *event)
+{
+	struct trace_export *export;
+
+	preempt_disable_notrace();
+
+	for (export = rcu_dereference_raw_notrace(trace_exports_list);
+	     export && export->commit;
+	     export = rcu_dereference_raw_notrace(export->next)) {
+		tr->export = export;
+		export->commit(tr, event);
+	}
+
+	preempt_enable_notrace();
+}

I'm not too happy about the added overhead to normal function tracing
(it will be noticeable), but I can fix that later.

+
+static void
+add_trace_export(struct trace_export **list, struct trace_export *export)
+{
+	export->next = *list;

As export->next will most likely need to be marked __rcu as well, this
may need an rcu_assign_pointer() too.

+	/*
+	 * We are entering export into the list but another
+	 * CPU might be walking that list. We need to make sure
+	 * the export->next pointer is valid before another CPU sees
+	 * the export pointer included into the list.
+	 */
+	rcu_assign_pointer(*list, export);
+
+}
+
+static int
+rm_trace_export(struct trace_export **list, struct trace_export *export)
+{
+	struct trace_export **p;
+
+	for (p = list; *p != &trace_export_rb; p = &(*p)->next)
+		if (*p == export)
+			break;
+
+	if (*p != export)
+		return -1;
+
+	*p = (*p)->next;

I believe this requires an rcu_assign_pointer() as well.

+
+	return 0;
+}
+
+int register_trace_export(struct trace_export *export)
+{
+	if (!export->write) {
+		pr_warn("trace_export must have the write() call back.\n");

Probably should be a "WARN_ON_ONCE()".

+		return -1;
+	}
+
+	if (!export->name) {
+		pr_warn("trace_export must have a name.\n");
+		return -1;
+	}

If name isn't used don't add it till it is. That is, this patch should
not have "name" in the structure. It's confusing, because I don't see a
purpose for it.

+
+	mutex_lock(&trace_export_lock);
+
+	export->tr = trace_exports_list->tr;

I don't see where tr is ever assigned.

+	export->commit = trace_generic_commit;

Shouldn't the caller pass in the commit function too?

How do you handle the discard? If the entry doesn't match the filter,
it will try to discard the event. I don't see how the "trace_exports"
handle that.

I really really dislike this. This is a big if statement that all
needs to go down one by one. Very inefficient!

-- Steve

+	} while (0)
+
+/*
  * An option specific to a tracer. This is a boolean value.
  * The bit is the bit index that sets its value on the
  * flags value in struct tracer_flags.