Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1647754 > unrolled thread

[PATCH v2 10/13] perf header: add a buffer to struct feat_fd

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 10/13] perf header: add a buffer to struct feat_fd David Carrillo-Cisneros <davidcc@google.com> - 2017-05-23 10:00 +0200
    Re: [PATCH v2 10/13] perf header: add a buffer to struct feat_fd Jiri Olsa <jolsa@redhat.com> - 2017-05-25 10:10 +0200
    Re: [PATCH v2 10/13] perf header: add a buffer to struct feat_fd Jiri Olsa <jolsa@redhat.com> - 2017-05-25 10:20 +0200
    Re: [PATCH v2 10/13] perf header: add a buffer to struct feat_fd Jiri Olsa <jolsa@redhat.com> - 2017-05-25 10:20 +0200
    Re: [PATCH v2 10/13] perf header: add a buffer to struct feat_fd Jiri Olsa <jolsa@redhat.com> - 2017-05-25 10:20 +0200

#1647754 — [PATCH v2 10/13] perf header: add a buffer to struct feat_fd

Extend struct feat_fd to use a temporal buffer in pipe-mode, rather
than a perf.data file.

Revamp write_pmu_mappings to avoid seeking so that is compatible with
pipe-mode.

Print an error when trying to use buf in features that do not support
pipe-mode.

Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 tools/perf/util/header.c | 79 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 62 insertions(+), 17 deletions(-)

diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index 65cd2d1f1721..b7704b30ed52 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -60,6 +60,7 @@ struct perf_file_attr {
 struct feat_fd {
 	struct perf_header *ph;
 	int fd;
+	void *buf;	/* Either buf != NULL or fd >= 0 */
 	ssize_t offset;
 	size_t size;
 };
@@ -82,11 +83,27 @@ bool perf_header__has_feat(const struct perf_header *header, int feat)
 /* Return: 0 if succeded, -ERR if failed. */
 int do_write(struct feat_fd *fd, const void *buf, size_t size)
 {
-	ssize_t ret;
+	void *addr;
 
-	ret  = writen(fd->fd, buf, size);
-	if (ret != (ssize_t)size)
-		return ret < 0 ? (int)ret: -1;
+	if (!fd->buf) {
+		ssize_t ret = writen(fd->fd, buf, size);
+
+		if (ret != (ssize_t)size)
+			return ret < 0 ? (int)ret : -1;
+		return 0;
+	}
+retry:
+	if (size > (fd->size - fd->offset)) {
+		addr = realloc(fd->buf, fd->size << 1);
+		if (!addr)
+			return -ENOSPC;
+		fd->buf = addr;
+		fd->size <<= 1;
+		goto retry;
+	}
+
+	memcpy(fd->buf + fd->offset, buf, size);
+	fd->offset += size;
 
 	return 0;
 }
@@ -126,10 +143,21 @@ static int do_write_string(struct feat_fd *fd, const char *str)
 
 static int __do_read(struct feat_fd *fd, void *addr, ssize_t size)
 {
-	ssize_t ret = readn(fd->fd, addr, size);
+	if (!fd->buf) {
+		ssize_t ret = readn(fd->fd, addr, size);
+
+		if (ret != (ssize_t)size)
+			return ret < 0 ? (int)ret : -1;
+		return 0;
+	}
+
+	assert((ssize_t)fd->size > fd->offset);
+	if (size > (ssize_t)fd->size - fd->offset)
+		return -1;
+
+	memcpy(addr, fd->buf + fd->offset, size);
+	fd->offset += size;
 
-	if (ret != (ssize_t)size)
-		return ret < 0 ? (int)ret : -1;
 	return 0;
 }
 
@@ -187,6 +215,10 @@ static char *do_read_string(struct feat_fd *fd)
 static int write_tracing_data(struct feat_fd *fd,
 			      struct perf_evlist *evlist)
 {
+	if (fd->buf) {
+		pr_err("Unsupported write_tracing_data to memory buffer.\n");
+		return -1;
+	}
 	return read_tracing_data(fd->fd, &evlist->entries);
 }
 
@@ -201,6 +233,10 @@ static int write_build_id(struct feat_fd *fd,
 	if (!perf_session__read_build_ids(session, true))
 		return -1;
 
+	if (fd->buf) {
+		pr_err("Unsupported write_build_id to memory buffer.\n");
+		return -1;
+	}
 	err = perf_session__write_buildid_table(session, fd);
 	if (err < 0) {
 		pr_debug("failed to write buildid table\n");
@@ -795,11 +831,19 @@ static int write_pmu_mappings(struct feat_fd *fd,
 			      struct perf_evlist *evlist __maybe_unused)
 {
 	struct perf_pmu *pmu = NULL;
-	off_t offset = lseek(fd->fd, 0, SEEK_CUR);
-	__u32 pmu_num = 0;
+	u32 pmu_num = 0;
 	int ret;
 
-	/* write real pmu_num later */
+	/*
+	 * Do a first pass to count number of pmu to avoid lseek so this
+	 * works in pipe mode as well.
+	 */
+	while ((pmu = perf_pmu__scan(pmu))) {
+		if (!pmu->name)
+			continue;
+		pmu_num++;
+	}
+
 	ret = do_write(fd, &pmu_num, sizeof(pmu_num));
 	if (ret < 0)
 		return ret;
@@ -807,7 +851,6 @@ static int write_pmu_mappings(struct feat_fd *fd,
 	while ((pmu = perf_pmu__scan(pmu))) {
 		if (!pmu->name)
 			continue;
-		pmu_num++;
 
 		ret = do_write(fd, &pmu->type, sizeof(pmu->type));
 		if (ret < 0)
@@ -818,12 +861,6 @@ static int write_pmu_mappings(struct feat_fd *fd,
 			return ret;
 	}
 
-	if (pwrite(fd->fd, &pmu_num, sizeof(pmu_num), offset) != sizeof(pmu_num)) {
-		/* discard all */
-		lseek(fd->fd, offset, SEEK_SET);
-		return -1;
-	}
-
 	return 0;
 }
 
@@ -909,6 +946,10 @@ static int write_auxtrace(struct feat_fd *fd,
 	struct perf_session *session;
 	int err;
 
+	if (fd->buf) {
+		pr_err("Unsupported write_auxtrace to memory buffer.\n");
+		return -1;
+	}
 	session = container_of(fd->ph, struct perf_session, header);
 
 	err = auxtrace_index__write(fd->fd, &session->auxtrace_index);
@@ -2187,6 +2228,10 @@ static int do_write_feat(struct feat_fd *fd, struct perf_header *h, int type,
 		if (!feat_ops[type].write)
 			return -1;
 
+		if (fd->buf) {
+			pr_err("Called %s for memory buffer.\n", __func__);
+			return -1;
+		}
 		(*p)->offset = lseek(fd->fd, 0, SEEK_CUR);
 
 		err = feat_ops[type].write(fd, evlist);
-- 
2.13.0.219.gdb65acc882-goog

[toc] | [next] | [standalone]

#1650271

On Tue, May 23, 2017 at 12:48:50AM -0700, David Carrillo-Cisneros wrote:

SNIP

> +
> +	memcpy(addr, fd->buf + fd->offset, size);
> +	fd->offset += size;
>  
> -	if (ret != (ssize_t)size)
> -		return ret < 0 ? (int)ret : -1;
>  	return 0;
>  }
>  
> @@ -187,6 +215,10 @@ static char *do_read_string(struct feat_fd *fd)
>  static int write_tracing_data(struct feat_fd *fd,
>  			      struct perf_evlist *evlist)
>  {
> +	if (fd->buf) {
> +		pr_err("Unsupported write_tracing_data to memory buffer.\n");
> +		return -1;
> +	}

could those messsages mention the pipe mode, this one
does not give clue it's pipe mode related

also together with your following patches, this condition
should never hit right? more like the assert stuff..
WARN_ON/WARN_ON_ONCE maybe

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1650276

On Tue, May 23, 2017 at 12:48:50AM -0700, David Carrillo-Cisneros wrote:
> Extend struct feat_fd to use a temporal buffer in pipe-mode, rather
> than a perf.data file.
> 
> Revamp write_pmu_mappings to avoid seeking so that is compatible with
> pipe-mode.
> 
> Print an error when trying to use buf in features that do not support
> pipe-mode.

could you please just list those features in changelog
plus the reason they don't support this

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1650277

On Tue, May 23, 2017 at 12:48:50AM -0700, David Carrillo-Cisneros wrote:

SNIP

> @@ -82,11 +83,27 @@ bool perf_header__has_feat(const struct perf_header *header, int feat)
>  /* Return: 0 if succeded, -ERR if failed. */
>  int do_write(struct feat_fd *fd, const void *buf, size_t size)
>  {
> -	ssize_t ret;
> +	void *addr;
>  
> -	ret  = writen(fd->fd, buf, size);
> -	if (ret != (ssize_t)size)
> -		return ret < 0 ? (int)ret: -1;
> +	if (!fd->buf) {
> +		ssize_t ret = writen(fd->fd, buf, size);
> +
> +		if (ret != (ssize_t)size)
> +			return ret < 0 ? (int)ret : -1;
> +		return 0;
> +	}
> +retry:
> +	if (size > (fd->size - fd->offset)) {
> +		addr = realloc(fd->buf, fd->size << 1);
> +		if (!addr)
> +			return -ENOSPC;
> +		fd->buf = addr;
> +		fd->size <<= 1;
> +		goto retry;
> +	}
> +
> +	memcpy(fd->buf + fd->offset, buf, size);
> +	fd->offset += size;
>  
>  	return 0;

please put those 2 cases in separate functions

>  }
> @@ -126,10 +143,21 @@ static int do_write_string(struct feat_fd *fd, const char *str)
>  
>  static int __do_read(struct feat_fd *fd, void *addr, ssize_t size)
>  {
> -	ssize_t ret = readn(fd->fd, addr, size);
> +	if (!fd->buf) {
> +		ssize_t ret = readn(fd->fd, addr, size);
> +
> +		if (ret != (ssize_t)size)
> +			return ret < 0 ? (int)ret : -1;
> +		return 0;
> +	}
> +
> +	assert((ssize_t)fd->size > fd->offset);
> +	if (size > (ssize_t)fd->size - fd->offset)
> +		return -1;
> +
> +	memcpy(addr, fd->buf + fd->offset, size);
> +	fd->offset += size;
>  
> -	if (ret != (ssize_t)size)
> -		return ret < 0 ? (int)ret : -1;
>  	return 0;
>  }

same for the read

thanks,
jirka

[toc] | [prev] | [next] | [standalone]

#1650286

On Tue, May 23, 2017 at 12:48:50AM -0700, David Carrillo-Cisneros wrote:

SNIP

> @@ -795,11 +831,19 @@ static int write_pmu_mappings(struct feat_fd *fd,
>  			      struct perf_evlist *evlist __maybe_unused)
>  {
>  	struct perf_pmu *pmu = NULL;
> -	off_t offset = lseek(fd->fd, 0, SEEK_CUR);
> -	__u32 pmu_num = 0;
> +	u32 pmu_num = 0;
>  	int ret;
>  
> -	/* write real pmu_num later */
> +	/*
> +	 * Do a first pass to count number of pmu to avoid lseek so this
> +	 * works in pipe mode as well.
> +	 */
> +	while ((pmu = perf_pmu__scan(pmu))) {
> +		if (!pmu->name)
> +			continue;
> +		pmu_num++;
> +	}

please put this functionality into preceeding separate patch

thanks,
jirka

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web