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

[Alexander Shishkin <alexander.shishkin@linux.intel.com>]

Mathieu Poirier <mathieu.poirier@linaro.org> 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