Re: [PATCH v2 1/2] perf tool: Refactor comm/tgid lookup

[Don Zickus]

On Mon, Mar 30, 2015 at 02:35:57PM -0600, David Ahern wrote:
> Rather than parsing /proc/pid/status file one line at a time, read
> it into a buffer in one shot and search for all strings in one pass.
> 
> tgid conversion also simplified -- removing the isspace walk. As
> noted by Arnaldo those are not needed for atoi == strtol calls.

Seeing Jiri is happy now. :-) 

Acked-by: Don Zickus <dzickus@xxxxxxxxxx>

> 
> Signed-off-by: David Ahern <dsahern@xxxxxxxxx>
> Cc: Don Zickus <dzickus@xxxxxxxxxx>
> Cc: Joe Mario <jmario@xxxxxxxxxx>
> Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
> v2:
> - removed the isspace checks on tgid string
> 
>  tools/perf/util/event.c | 72 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 44 insertions(+), 28 deletions(-)
> 
> diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
> index d5efa5092ce6..023dd3548a94 100644
> --- a/tools/perf/util/event.c
> +++ b/tools/perf/util/event.c
> @@ -49,48 +49,64 @@ static struct perf_sample synth_sample = {
>  	.period	   = 1,
>  };
>  
> +/*
> + * Assumes that the first 4095 bytes of /proc/pid/stat contains
> + * the comm and tgid.
> + */
>  static pid_t perf_event__get_comm_tgid(pid_t pid, char *comm, size_t len)
>  {
>  	char filename[PATH_MAX];
> -	char bf[BUFSIZ];
> -	FILE *fp;
> -	size_t size = 0;
> +	char bf[4096];
> +	int fd;
> +	size_t size = 0, n;
>  	pid_t tgid = -1;
> +	char *nl, *name, *tgids;
>  
>  	snprintf(filename, sizeof(filename), "/proc/%d/status", pid);
>  
> -	fp = fopen(filename, "r");
> -	if (fp == NULL) {
> +	fd = open(filename, O_RDONLY);
> +	if (fd < 0) {
>  		pr_debug("couldn't open %s\n", filename);
>  		return 0;
>  	}
>  
> -	while (!comm[0] || (tgid < 0)) {
> -		if (fgets(bf, sizeof(bf), fp) == NULL) {
> -			pr_warning("couldn't get COMM and pgid, malformed %s\n",
> -				   filename);
> -			break;
> -		}
> +	n = read(fd, bf, sizeof(bf) - 1);
> +	close(fd);
> +	if (n <= 0) {
> +		pr_warning("Couldn't get COMM and tgid for pid %d\n",
> +			   pid);
> +		return -1;
> +	}
> +	bf[n] = '\0';
>  
> -		if (memcmp(bf, "Name:", 5) == 0) {
> -			char *name = bf + 5;
> -			while (*name && isspace(*name))
> -				++name;
> -			size = strlen(name) - 1;
> -			if (size >= len)
> -				size = len - 1;
> -			memcpy(comm, name, size);
> -			comm[size] = '\0';
> -
> -		} else if (memcmp(bf, "Tgid:", 5) == 0) {
> -			char *tgids = bf + 5;
> -			while (*tgids && isspace(*tgids))
> -				++tgids;
> -			tgid = atoi(tgids);
> -		}
> +	name = strstr(bf, "Name:");
> +	tgids = strstr(bf, "Tgid:");
> +
> +	if (name) {
> +		name += 5;  /* strlen("Name:") */
> +
> +		while (*name && isspace(*name))
> +			++name;
> +
> +		nl = strchr(name, '\n');
> +		if (nl)
> +			*nl = '\0';
> +
> +		size = strlen(name);
> +		if (size >= len)
> +			size = len - 1;
> +		memcpy(comm, name, size);
> +		comm[size] = '\0';
> +	} else {
> +		pr_debug("Name: string not found for pid %d\n", pid);
>  	}
>  
> -	fclose(fp);
> +	if (tgids) {
> +		tgids += 5;  /* strlen("Tgid:") */
> +		tgid = atoi(tgids);
> +	} else {
> +		pr_debug("Tgid: string not found for pid %d\n", pid);
> +	}
>  
>  	return tgid;
>  }
> -- 
> 2.2.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/