[PATCH V5 4/9] perf/core: Adding PMU driver specific configuration

Mathieu Poirier [off-list ref] writes:

This patch somewhat mimics the work done on address filters to
add the infrastructure needed to pass PMU specific HW
configuration to the driver before a session starts.

Looks like a lot of work to do something that can be taken care of
entirely in userspace. A few comments below.

Btw, please don't forget to CC me on the kernel perf patches.

+static struct perf_drv_config *
+perf_drv_config_new(int cpu, struct list_head *drv_config_list)
+{
+	int node = cpu_to_node(cpu == -1 ? 0 : cpu);
+	struct perf_drv_config *drv_config;
+
+	drv_config = kzalloc_node(sizeof(*drv_config), GFP_KERNEL, node);
+	if (!drv_config)
+		return ERR_PTR(-ENOMEM);

So it's the only error that this function may return.

+
+	INIT_LIST_HEAD(&drv_config->entry);
+	list_add_tail(&drv_config->entry, drv_config_list);
+
+	return drv_config;
+}
+
+static void free_drv_config_list(struct list_head *drv_config_list)
+{
+	struct perf_drv_config *drv_config, *itr;
+
+	list_for_each_entry_safe(drv_config, itr, drv_config_list, entry) {
+		list_del(&drv_config->entry);
+		kfree(drv_config->config);
+		kfree(drv_config->option);
+		kfree(drv_config);
+	}
+}
+
+/* How long does a configuration option really need to be?  */
+#define PERF_DRV_CONFIG_MAX	128

Considering that you're already limiting the entire input buffer to
PAGE_SIZE, this is redundant.

+
 /*
- * hrtimer based swevent callback
+ * PMU specific driver configuration as specified from user space.
+ * The data come in the form of an ascii string pushed down to the kernel
+ * using an ioctl() call.
+ *
+ * Two format are accepted: a singleton and in pairs.  All of the following
+ * are valid: cfg1, cfg2=config2, cfg3=anything_is_possible.

What's the difference between 'config2' and 'anything_is_possible'?

+ *
+ * It is up to each PMU driver to make sure they can work with the
+ * submitted configurables.
  */
+static int
+perf_event_parse_drv_config(struct perf_event *event, char *options,
+			    struct list_head *drv_config_list)
+{
+	char *token;
+	int ret;
+	struct perf_drv_config *drv_config;
+
+	/*
+	 * First split the @options string in nibbles.  Using the above
+	 * example "cfg1", "cfg2=option2" and "cfg3=anything_is_possible"

"cfg2=config2", if you're referring to the comment at the top.

+	 * will be processed.
+	 */
+	while ((token = strsep(&options, ",")) != NULL) {
+		char *nibble, *config, *option;
+
+		if (!*token)
+			continue;

So empty configs are valid?

+
+		/* Allocate a new driver config structure and queue it. */
+		drv_config = perf_drv_config_new(event->cpu, drv_config_list);
+		if (IS_ERR(drv_config)) {
+			ret = PTR_ERR(drv_config);

We know it's ENOMEM, no need for ERR_PTR()->PTR_ERR().

+			goto fail;
+		}
+
+		/*
+		 * The nibbles are either a "config" or a "config=option"
+		 * pair.  First get the config part.  Since strsep() sets
+		 * @nibble to the next valid token, nibble will be equal to
+		 * the option part or NULL after the first call.
+		 */
+		nibble = token;
+		config = strsep(&nibble, "=");
+		option = nibble;

So '@,=,=,=,=,=,=,=,=,' is a valid driver configuration, by the looks of
it?

+
+		/* This shouldn't be happening */

Indeed.

+		if (!config) {
+			ret = -EINVAL;
+			goto fail;
+		}

Regards,
--
Alex