Re: [PATCH 2/2] perf_counter: add support to the 'perf' tool for tracepoints

[Ingo Molnar <mingo@elte.hu>]

* Jason Baron <jbaron@redhat.com> wrote:

> +static char *tracepoint_id_to_name(u64 config)
> +{
> +	static char tracepoint_name[2 * MAX_EVENT_LENGTH];
> +	DIR *sys_dir, *evt_dir;
> +	struct dirent *sys_next, *evt_next, sys_dirent, evt_dirent;
> +	struct stat st;
> +	char id_buf[4];
> +	int fd;
> +	long long id;

could this become u64?

> +	char evt_path[MAX_PATH_LENGTH];
> +
> +	if (valid_debugfs_mount())
> +		return "unkown";
> +
> +	sys_dir = opendir(default_debugfs_path);
> +	if (!sys_dir)
> +		goto cleanup;
> +	for_each_subsystem(sys_dir, sys_dirent, sys_next, evt_path, st) {
> +		evt_dir = opendir(evt_path);
> +		if (!evt_dir)
> +			goto cleanup;
> +		for_each_event(sys_dirent, evt_dir, evt_dirent, evt_next,
> +								evt_path, st) {
> +			sprintf(evt_path, "%s/%s/%s/id", default_debugfs_path,
> +				sys_dirent.d_name, evt_dirent.d_name);
> +			fd = open(evt_path, O_RDONLY);
> +			if (fd < 0)
> +				continue;
> +			if (read(fd, id_buf, sizeof(id_buf)) < 0) {
> +				close(fd);
> +				continue;
> +			}
> +			close(fd);
> +			id = atoll(id_buf);
> +			if ((u64)id == config) {

... and thus the cast here could be dropped?

> +				closedir(evt_dir);
> +				closedir(sys_dir);
> +				snprintf(tracepoint_name, 2 * MAX_EVENT_LENGTH,
> +					"%s:%s", sys_dirent.d_name,
> +					evt_dirent.d_name);
> +				return tracepoint_name;
> +			}
> +		}
> +		closedir(evt_dir);
> +	}
> +cleanup:
> +	closedir(sys_dir);
> +	return "unkown";
> +}
> +
>  static int is_cache_op_valid(u8 cache_type, u8 cache_op)
>  {
>  	if (hw_cache_stat[cache_type] & COP(cache_op))
> @@ -177,6 +259,9 @@ char *event_name(int counter)
>  			return sw_event_names[config];
>  		return "unknown-software";
>  
> +	case PERF_TYPE_TRACEPOINT:
> +		return tracepoint_id_to_name(config);
> +
>  	default:
>  		break;
>  	}
> @@ -265,6 +350,78 @@ parse_generic_hw_event(const char **str, struct perf_counter_attr *attr)
>  	return 1;
>  }
>  
> +static int parse_tracepoint_event(const char **strp,
> +				    struct perf_counter_attr *attr)
> +{
> +	const char *evt_name;
> +	char sys_name[MAX_EVENT_LENGTH];
> +	DIR *sys_dir, *evt_dir;
> +	struct dirent *sys_next, *evt_next, sys_dirent, evt_dirent;
> +	struct stat st;
> +	char id_buf[4];
> +	int fd;
> +	unsigned int sys_length, evt_length;
> +	u64 id;
> +	char evt_path[MAX_PATH_LENGTH];
> +
> +	if (valid_debugfs_mount())
> +		return 0;
> +
> +	evt_name = strchr(*strp, ':');
> +	if (evt_name) {
> +		sys_length = evt_name - *strp;
> +		if (sys_length >= MAX_EVENT_LENGTH)
> +			return 0;
> +		strncpy(sys_name, *strp, sys_length);
> +		sys_name[sys_length] = '\0';
> +		evt_name = evt_name + 1;
> +		evt_length = strlen(evt_name);
> +		if (evt_length >= MAX_EVENT_LENGTH)
> +			return 0;
> +	} else
> +		return 0;
> +
> +	sys_dir = opendir(default_debugfs_path);
> +	if (!sys_dir)
> +		goto cleanup;
> +	for_each_subsystem(sys_dir, sys_dirent, sys_next, evt_path, st) {

a minor request. Just like you added a newline after the return 0 
above, please add a newline after separate blocks of code as well - 
i.e after the 'goto cleanup' above. It makes the loop which begins 
stand out on its own, and makes the sys_dir initialization look 
self-sufficient as well. (which it is)

> +		if (strcmp(sys_dirent.d_name, sys_name) == 0) {
> +			evt_dir = opendir(evt_path);
> +			if (!evt_dir)
> +				goto cleanup;
> +			for_each_event(sys_dirent, evt_dir, evt_dirent,
> +						       evt_next, evt_path, st) {
> +				if (strcmp(evt_dirent.d_name, evt_name) == 0) {
> +					sprintf(evt_path, "%s/%s/%s/id",
> +						default_debugfs_path,
> +						sys_dirent.d_name,
> +						evt_dirent.d_name);
> +					fd = open(evt_path, O_RDONLY);
> +					if (fd < 0)
> +						continue;
> +					if (read(fd, id_buf,
> +						 sizeof(id_buf)) < 0) {
> +						close(fd);
> +						continue;
> +					}
> +					close(fd);
> +					id = atoll(id_buf);
> +					attr->config = id;
> +					attr->type = PERF_TYPE_TRACEPOINT;
> +					closedir(evt_dir);
> +					closedir(sys_dir);
> +					*strp = evt_name + evt_length;
> +					return 1;
> +				}
> +			}
> +			closedir(evt_dir);
> +		}
> +	}
> +cleanup:
> +	closedir(sys_dir);
> +	return 0;
> +}

Ok, this whole feature looks nice. A few minor details need fixing: 
the two functions above tracepoint_id_to_name() and 
parse_tracepoint_event() are a bit large and look very crowded.

one trick to win an indentation level would be to turn this:

> +		if (strcmp(sys_dirent.d_name, sys_name) == 0) {
> +			evt_dir = opendir(evt_path);

into:

		if (strcmp(sys_dirent.d_name, sys_name))
			continue;

		evt_dir = opendir(evt_path);

(Also, for logic operations please write '!x' instead of 'x == 0' - 
it's easy to see only the beginning of the line and miss the 
effective negation.)

The other thing to do would be to move the inner core of the loop 
out into a helper inline function.

plus, this:

> +	if (evt_name) {
> +		sys_length = evt_name - *strp;
> +		if (sys_length >= MAX_EVENT_LENGTH)
> +			return 0;
> +		strncpy(sys_name, *strp, sys_length);
> +		sys_name[sys_length] = '\0';
> +		evt_name = evt_name + 1;
> +		evt_length = strlen(evt_name);
> +		if (evt_length >= MAX_EVENT_LENGTH)
> +			return 0;
> +	} else
> +		return 0;

should be written as:

	if (!evt_name)
		return 0;

	sys_length = evt_name - *strp;
	...

i.e. we should try to compress the visual complexity of 
code as much as possible.

Also ... all code flows return 0, regardless of error or not error. 
Is this intentional?

	Ingo