[PATCH 0/3] perf tools: Add data file write interface

[PATCH 0/3] perf tools: Add data file write interface

[Jiri Olsa]

hi,
adding perf_data_file__write function to centralize
output file writes. Using it in record and inject
commands.

David,
patch 2 could colide with your mmap changes, not sure
what's your plan with that in near future, let me know
and I can rebase if needed.

thanks,
jirka


Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Stephane Eranian <eranian@google.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
---
Jiri Olsa (3):
      perf tools: Add perf_data_file__write interface
      perf record: Use perf_data_file__write for output file
      perf inject: Handle output file via perf_data_file object

 tools/perf/builtin-inject.c | 71 +++++++++++++++++++++++++++++++----------------------------------------
 tools/perf/builtin-record.c | 42 +++++++++++++-----------------------------
 tools/perf/util/data.c      | 20 ++++++++++++++++++++
 tools/perf/util/data.h      | 15 ++++++++-------
 4 files changed, 72 insertions(+), 76 deletions(-)

[PATCH 1/3] perf tools: Add perf_data_file__write interface

[Jiri Olsa]

Adding perf_data_file__write interface to centralize
output to files. The function prototype is:

  ssize_t perf_data_file__write(struct perf_data_file *file,
                                void *buf, size_t size);

Returns number of bytes written or -1 in case of error.

NOTE Indenting 'struct perf_data_file' members,
     no functional change done.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Stephane Eranian <eranian@google.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/util/data.c | 20 ++++++++++++++++++++
 tools/perf/util/data.h | 15 ++++++++-------
 2 files changed, 28 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
index 7d09faf..cce1256 100644
--- a/tools/perf/util/data.c
+++ b/tools/perf/util/data.c
@@ -118,3 +118,23 @@ void perf_data_file__close(struct perf_data_file *file)
 {
 	close(file->fd);
 }
+
+ssize_t perf_data_file__write(struct perf_data_file *file,
+			      void *buf, size_t size)
+{
+	ssize_t total = size;
+
+	while (size) {
+		ssize_t ret = write(file->fd, buf, size);
+
+		if (ret < 0) {
+			pr_err("failed to write perf data, error: %m\n");
+			return -1;
+		}
+
+		size -= ret;
+		buf  += ret;
+	}
+
+	return total;
+}
diff --git a/tools/perf/util/data.h b/tools/perf/util/data.h
index 8c2df80..02c53dc 100644
--- a/tools/perf/util/data.h
+++ b/tools/perf/util/data.h
@@ -9,12 +9,12 @@ enum perf_data_mode {
 };
 
 struct perf_data_file {
-	const char *path;
-	int fd;
-	bool is_pipe;
-	bool force;
-	unsigned long size;
-	enum perf_data_mode mode;
+	const char		*path;
+	int			 fd;
+	bool			 is_pipe;
+	bool			 force;
+	unsigned long		 size;
+	enum perf_data_mode	 mode;
 };
 
 static inline bool perf_data_file__is_read(struct perf_data_file *file)
@@ -44,5 +44,6 @@ static inline unsigned long perf_data_file__size(struct perf_data_file *file)
 
 int perf_data_file__open(struct perf_data_file *file);
 void perf_data_file__close(struct perf_data_file *file);
-
+ssize_t perf_data_file__write(struct perf_data_file *file,
+			      void *buf, size_t size);
 #endif /* __PERF_DATA_H */
-- 
1.8.3.1

Re: [PATCH 1/3] perf tools: Add perf_data_file__write interface

[Arnaldo Carvalho de Melo]

Em Fri, Nov 22, 2013 at 03:24:26PM +0100, Jiri Olsa escreveu:
>  }
> +
> +ssize_t perf_data_file__write(struct perf_data_file *file,
> +			      void *buf, size_t size)
> +{
> +	ssize_t total = size;
> +
> +	while (size) {
> +		ssize_t ret = write(file->fd, buf, size);
> +
> +		if (ret < 0) {
> +			pr_err("failed to write perf data, error: %m\n");
> +			return -1;
> +		}
> +
> +		size -= ret;
> +		buf  += ret;
> +	}
> +
> +	return total;

So this is the functional equivalent of "readn", so please move it to
just after "readn" and make this just a simple wrapper.

- Arnaldo

Re: [PATCH 1/3] perf tools: Add perf_data_file__write interface

[Jiri Olsa]

On Mon, Nov 25, 2013 at 04:29:09PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 22, 2013 at 03:24:26PM +0100, Jiri Olsa escreveu:
> >  }
> > +
> > +ssize_t perf_data_file__write(struct perf_data_file *file,
> > +			      void *buf, size_t size)
> > +{
> > +	ssize_t total = size;
> > +
> > +	while (size) {
> > +		ssize_t ret = write(file->fd, buf, size);
> > +
> > +		if (ret < 0) {
> > +			pr_err("failed to write perf data, error: %m\n");
> > +			return -1;
> > +		}
> > +
> > +		size -= ret;
> > +		buf  += ret;
> > +	}
> > +
> > +	return total;
> 
> So this is the functional equivalent of "readn", so please move it to
> just after "readn" and make this just a simple wrapper.

ok, thanks

jirka

[PATCH] tools/perf/util: Document and clean up readn() a bit

[Ingo Molnar]

* Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:

> Em Fri, Nov 22, 2013 at 03:24:26PM +0100, Jiri Olsa escreveu:
> >  }
> > +
> > +ssize_t perf_data_file__write(struct perf_data_file *file,
> > +			      void *buf, size_t size)
> > +{
> > +	ssize_t total = size;
> > +
> > +	while (size) {
> > +		ssize_t ret = write(file->fd, buf, size);
> > +
> > +		if (ret < 0) {
> > +			pr_err("failed to write perf data, error: %m\n");
> > +			return -1;
> > +		}
> > +
> > +		size -= ret;
> > +		buf  += ret;
> > +	}
> > +
> > +	return total;
> 
> So this is the functional equivalent of "readn", so please move it to
> just after "readn" and make this just a simple wrapper.

Btw., would be nice to add a small comment to readn() that describes 
its semantics, it looks like a useful helper.

I also added a check for the input parameter 'n', plus I added a 
'left' variable to make the flow clearer, and added a debug check for 
the return value - I think returning 'n' is more obvious.

Totally untested though.

Thanks,

	Ingo

Signed-off-by: Ingo Molnar <mingo@kernel.org>

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 28a0a89..4789081 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -6,6 +6,7 @@
 #endif
 #include <stdio.h>
 #include <stdlib.h>
+#include <linux/kernel.h>
 
 /*
  * XXX We need to find a better place for these things...
@@ -151,21 +152,29 @@ unsigned long convert_unit(unsigned long value, char *unit)
 	return value;
 }
 
-int readn(int fd, void *buf, size_t n)
+/*
+ * Read exactly 'n' bytes or return an error:
+ */
+int readn(int fd, void *buf, ssize_t n)
 {
 	void *buf_start = buf;
+	size_t left = n;
+
+	BUG_ON(n <= 0);
 
-	while (n) {
-		int ret = read(fd, buf, n);
+	while (left) {
+		int ret = read(fd, buf, left);
 
 		if (ret <= 0)
 			return ret;
 
-		n -= ret;
+		left -= ret;
 		buf += ret;
 	}
 
-	return buf - buf_start;
+	BUG_ON(buf-buf_start != n);
+
+	return n;
 }
 
 size_t hex_width(u64 v)
diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
index c8f362d..bb0a336 100644
--- a/tools/perf/util/util.h
+++ b/tools/perf/util/util.h
@@ -253,7 +253,7 @@ bool strlazymatch(const char *str, const char *pat);
 int strtailcmp(const char *s1, const char *s2);
 char *strxfrchar(char *s, char from, char to);
 unsigned long convert_unit(unsigned long value, char *unit);
-int readn(int fd, void *buf, size_t size);
+int readn(int fd, void *buf, ssize_t size);
 
 struct perf_event_attr;
 

Re: [PATCH] tools/perf/util: Document and clean up readn() a bit

[Adrian Hunter]

On 26/11/13 19:53, Ingo Molnar wrote:
> 
> * Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:
> 
>> Em Fri, Nov 22, 2013 at 03:24:26PM +0100, Jiri Olsa escreveu:
>>>  }
>>> +
>>> +ssize_t perf_data_file__write(struct perf_data_file *file,
>>> +			      void *buf, size_t size)
>>> +{
>>> +	ssize_t total = size;
>>> +
>>> +	while (size) {
>>> +		ssize_t ret = write(file->fd, buf, size);
>>> +
>>> +		if (ret < 0) {
>>> +			pr_err("failed to write perf data, error: %m\n");
>>> +			return -1;
>>> +		}
>>> +
>>> +		size -= ret;
>>> +		buf  += ret;
>>> +	}
>>> +
>>> +	return total;
>>
>> So this is the functional equivalent of "readn", so please move it to
>> just after "readn" and make this just a simple wrapper.
> 
> Btw., would be nice to add a small comment to readn() that describes 
> its semantics, it looks like a useful helper.
> 
> I also added a check for the input parameter 'n', plus I added a 
> 'left' variable to make the flow clearer, and added a debug check for 
> the return value - I think returning 'n' is more obvious.

It would be nicer to match what 'read' does.

> 
> Totally untested though.
> 
> Thanks,
> 
> 	Ingo
> 
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> 
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 28a0a89..4789081 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -6,6 +6,7 @@
>  #endif
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <linux/kernel.h>
>  
>  /*
>   * XXX We need to find a better place for these things...
> @@ -151,21 +152,29 @@ unsigned long convert_unit(unsigned long value, char *unit)
>  	return value;
>  }
>  
> -int readn(int fd, void *buf, size_t n)
> +/*
> + * Read exactly 'n' bytes or return an error:
> + */
> +int readn(int fd, void *buf, ssize_t n)

Should really be the same prototype as 'read' i.e.

	ssize_t readn(int fd, void *buf, size_t n)

Need to change callers that are using 'int' too.

>  {
>  	void *buf_start = buf;
> +	size_t left = n;
> +
> +	BUG_ON(n <= 0);

	BUG_ON(n == 0 || n > SSIZE_MAX);

>  
> -	while (n) {
> -		int ret = read(fd, buf, n);
> +	while (left) {
> +		int ret = read(fd, buf, left);

Should use the correct return type:

		ssize_t ret = read(fd, buf, left);

>  
>  		if (ret <= 0)
>  			return ret;

Don't return 0 if not all the bytes were read:

		if (ret < 0)
			return ret;
		if (!ret)
			break;

>  
> -		n -= ret;
> +		left -= ret;
>  		buf += ret;
>  	}
>  
> -	return buf - buf_start;
> +	BUG_ON(buf-buf_start != n);
> +
> +	return n;

Should return the same value as 'read' i.e the original

	return buf - buf_start;

>  }
>  
>  size_t hex_width(u64 v)
> diff --git a/tools/perf/util/util.h b/tools/perf/util/util.h
> index c8f362d..bb0a336 100644
> --- a/tools/perf/util/util.h
> +++ b/tools/perf/util/util.h
> @@ -253,7 +253,7 @@ bool strlazymatch(const char *str, const char *pat);
>  int strtailcmp(const char *s1, const char *s2);
>  char *strxfrchar(char *s, char from, char to);
>  unsigned long convert_unit(unsigned long value, char *unit);
> -int readn(int fd, void *buf, size_t size);
> +int readn(int fd, void *buf, ssize_t size);
>  
>  struct perf_event_attr;
>  
> 
> 

Re: [PATCH] tools/perf/util: Document and clean up readn() a bit

[Ingo Molnar]

* Adrian Hunter <adrian.hunter@intel.com> wrote:

> On 26/11/13 19:53, Ingo Molnar wrote:
> > 
> > * Arnaldo Carvalho de Melo <acme@ghostprotocols.net> wrote:
> > 
> >> Em Fri, Nov 22, 2013 at 03:24:26PM +0100, Jiri Olsa escreveu:
> >>>  }
> >>> +
> >>> +ssize_t perf_data_file__write(struct perf_data_file *file,
> >>> +			      void *buf, size_t size)
> >>> +{
> >>> +	ssize_t total = size;
> >>> +
> >>> +	while (size) {
> >>> +		ssize_t ret = write(file->fd, buf, size);
> >>> +
> >>> +		if (ret < 0) {
> >>> +			pr_err("failed to write perf data, error: %m\n");
> >>> +			return -1;
> >>> +		}
> >>> +
> >>> +		size -= ret;
> >>> +		buf  += ret;
> >>> +	}
> >>> +
> >>> +	return total;
> >>
> >> So this is the functional equivalent of "readn", so please move it to
> >> just after "readn" and make this just a simple wrapper.
> > 
> > Btw., would be nice to add a small comment to readn() that describes 
> > its semantics, it looks like a useful helper.
> > 
> > I also added a check for the input parameter 'n', plus I added a 
> > 'left' variable to make the flow clearer, and added a debug check for 
> > the return value - I think returning 'n' is more obvious.
> 
> It would be nicer to match what 'read' does.
> 
> > 
> > Totally untested though.
> > 
> > Thanks,
> > 
> > 	Ingo
> > 
> > Signed-off-by: Ingo Molnar <mingo@kernel.org>
> > 
> > diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> > index 28a0a89..4789081 100644
> > --- a/tools/perf/util/util.c
> > +++ b/tools/perf/util/util.c
> > @@ -6,6 +6,7 @@
> >  #endif
> >  #include <stdio.h>
> >  #include <stdlib.h>
> > +#include <linux/kernel.h>
> >  
> >  /*
> >   * XXX We need to find a better place for these things...
> > @@ -151,21 +152,29 @@ unsigned long convert_unit(unsigned long value, char *unit)
> >  	return value;
> >  }
> >  
> > -int readn(int fd, void *buf, size_t n)
> > +/*
> > + * Read exactly 'n' bytes or return an error:
> > + */
> > +int readn(int fd, void *buf, ssize_t n)
> 
> Should really be the same prototype as 'read' i.e.
> 
> 	ssize_t readn(int fd, void *buf, size_t n)
> 
> Need to change callers that are using 'int' too.
> 
> >  {
> >  	void *buf_start = buf;
> > +	size_t left = n;
> > +
> > +	BUG_ON(n <= 0);
> 
> 	BUG_ON(n == 0 || n > SSIZE_MAX);
> 
> >  
> > -	while (n) {
> > -		int ret = read(fd, buf, n);
> > +	while (left) {
> > +		int ret = read(fd, buf, left);
> 
> Should use the correct return type:
> 
> 		ssize_t ret = read(fd, buf, left);
> 
> >  
> >  		if (ret <= 0)
> >  			return ret;
> 
> Don't return 0 if not all the bytes were read:
> 
> 		if (ret < 0)
> 			return ret;
> 		if (!ret)
> 			break;

Okay, I thought this was an intentional 'all or nothing' interface - 
but looking at the readn() users they can tolerate partial results 
just fine.

> 
> >  
> > -		n -= ret;
> > +		left -= ret;
> >  		buf += ret;
> >  	}
> >  
> > -	return buf - buf_start;
> > +	BUG_ON(buf-buf_start != n);
> > +
> > +	return n;
> 
> Should return the same value as 'read' i.e the original
> 
> 	return buf - buf_start;

It does, in the original version I patched buf-buf_start == n, see the 
assert that checks for that.

If partial reads are returned then this bit has to change too, yes.

Thanks,

	Ingo

Re: [PATCH] tools/perf/util: Document and clean up readn() a bit

[David Ahern]

On 11/27/13, 4:57 AM, Ingo Molnar wrote:
> Okay, I thought this was an intentional 'all or nothing' interface -
> but looking at the readn() users they can tolerate partial results
> just fine.

I believe that is the intent -- an all or nothing interface.

David

Re: [PATCH] tools/perf/util: Document and clean up readn() a bit

[Jiri Olsa]

On Wed, Nov 27, 2013 at 08:19:13AM -0700, David Ahern wrote:
> On 11/27/13, 4:57 AM, Ingo Molnar wrote:
> >Okay, I thought this was an intentional 'all or nothing' interface -
> >but looking at the readn() users they can tolerate partial results
> >just fine.
> 
> I believe that is the intent -- an all or nothing interface.

all the users either checks the returned value with the size
or do (ret < 0) and fail

and one instance in the read_attr does not check anything and
blindly hopes it will read all ;-)

I have similar patch that also change callers to use proper
ssize_t instead of int.. I can rebase and send it separately
or combine it with yours.. let me know

jirka

Re: [PATCH] tools/perf/util: Document and clean up readn() a bit

[Ingo Molnar]

* Jiri Olsa <jolsa@redhat.com> wrote:

> On Wed, Nov 27, 2013 at 08:19:13AM -0700, David Ahern wrote:
> > On 11/27/13, 4:57 AM, Ingo Molnar wrote:
> > >Okay, I thought this was an intentional 'all or nothing' interface -
> > >but looking at the readn() users they can tolerate partial results
> > >just fine.
> > 
> > I believe that is the intent -- an all or nothing interface.
> 
> all the users either checks the returned value with the size
> or do (ret < 0) and fail

so, a 'ret < 0' check would actually be sensitive to whether readn() 
is an all-or-nothing interface (today), or a partial interface (the 
suggestion).

So it appears keeping it all-or-nothing (i.e. my patch) is the right 
approach.

> and one instance in the read_attr does not check anything and 
> blindly hopes it will read all ;-)
> 
> I have similar patch that also change callers to use proper ssize_t 
> instead of int.. I can rebase and send it separately or combine it 
> with yours.. let me know

Sure ... I just noticed a few patterns. Feel free to use all (or none 
;-) of my patch in your series.

Thanks,

	Ingo

[PATCH 2/3] perf record: Use perf_data_file__write for output file

[Jiri Olsa]

Changing the file output code to use the newly
added perf_data_file__write interface.

No functional change intended.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Stephane Eranian <eranian@google.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/builtin-record.c | 42 +++++++++++++-----------------------------
 1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 65615a8..bc3c7be 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -76,42 +76,27 @@ struct perf_record {
 	long			samples;
 };
 
-static int do_write_output(struct perf_record *rec, void *buf, size_t size)
+static ssize_t perf_record__write(struct perf_record *rec,
+				  void *buf, size_t size)
 {
-	struct perf_data_file *file = &rec->file;
-
-	while (size) {
-		ssize_t ret = write(file->fd, buf, size);
-
-		if (ret < 0) {
-			pr_err("failed to write perf data, error: %m\n");
-			return -1;
-		}
-
-		size -= ret;
-		buf += ret;
+	struct perf_session *session = rec->session;
+	ssize_t ret;
 
-		rec->bytes_written += ret;
-	}
+	ret = perf_data_file__write(session->file, buf, size);
+	if (ret < 0)
+		return -1;
 
+	rec->bytes_written += ret;
 	return 0;
 }
 
-static int write_output(struct perf_record *rec, void *buf, size_t size)
-{
-	return do_write_output(rec, buf, size);
-}
-
 static int process_synthesized_event(struct perf_tool *tool,
 				     union perf_event *event,
 				     struct perf_sample *sample __maybe_unused,
 				     struct machine *machine __maybe_unused)
 {
 	struct perf_record *rec = container_of(tool, struct perf_record, tool);
-	if (write_output(rec, event, event->header.size) < 0)
-		return -1;
-
-	return 0;
+	return perf_record__write(rec, event, event->header.size);
 }
 
 static int perf_record__mmap_read(struct perf_record *rec,
@@ -136,7 +121,7 @@ static int perf_record__mmap_read(struct perf_record *rec,
 		size = md->mask + 1 - (old & md->mask);
 		old += size;
 
-		if (write_output(rec, buf, size) < 0) {
+		if (perf_record__write(rec, buf, size) < 0) {
 			rc = -1;
 			goto out;
 		}
@@ -146,7 +131,7 @@ static int perf_record__mmap_read(struct perf_record *rec,
 	size = head - old;
 	old += size;
 
-	if (write_output(rec, buf, size) < 0) {
+	if (perf_record__write(rec, buf, size) < 0) {
 		rc = -1;
 		goto out;
 	}
@@ -322,8 +307,7 @@ static struct perf_event_header finished_round_event = {
 
 static int perf_record__mmap_read_all(struct perf_record *rec)
 {
-	int i;
-	int rc = 0;
+	int i, rc = 0;
 
 	for (i = 0; i < rec->evlist->nr_mmaps; i++) {
 		if (rec->evlist->mmap[i].base) {
@@ -335,7 +319,7 @@ static int perf_record__mmap_read_all(struct perf_record *rec)
 	}
 
 	if (perf_header__has_feat(&rec->session->header, HEADER_TRACING_DATA))
-		rc = write_output(rec, &finished_round_event,
+		rc = perf_record__write(rec, &finished_round_event,
 				  sizeof(finished_round_event));
 
 out:
-- 
1.8.3.1

Re: [PATCH 2/3] perf record: Use perf_data_file__write for output file

[Arnaldo Carvalho de Melo]

Em Fri, Nov 22, 2013 at 03:24:27PM +0100, Jiri Olsa escreveu:
> Changing the file output code to use the newly
> added perf_data_file__write interface.

So I like renaming write_output() to perf_record__write(), but then
you're lumping together this change and the other, which is to make
write_output, now renamed to perf_record__write() to use perf_data_file.

Can you please split it like that?

- Arnaldo
 
> No functional change intended.
> 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Stephane Eranian <eranian@google.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/builtin-record.c | 42 +++++++++++++-----------------------------
>  1 file changed, 13 insertions(+), 29 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 65615a8..bc3c7be 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -76,42 +76,27 @@ struct perf_record {
>  	long			samples;
>  };
>  
> -static int do_write_output(struct perf_record *rec, void *buf, size_t size)
> +static ssize_t perf_record__write(struct perf_record *rec,
> +				  void *buf, size_t size)
>  {
> -	struct perf_data_file *file = &rec->file;
> -
> -	while (size) {
> -		ssize_t ret = write(file->fd, buf, size);
> -
> -		if (ret < 0) {
> -			pr_err("failed to write perf data, error: %m\n");
> -			return -1;
> -		}
> -
> -		size -= ret;
> -		buf += ret;
> +	struct perf_session *session = rec->session;
> +	ssize_t ret;
>  
> -		rec->bytes_written += ret;
> -	}
> +	ret = perf_data_file__write(session->file, buf, size);
> +	if (ret < 0)
> +		return -1;
>  
> +	rec->bytes_written += ret;
>  	return 0;
>  }
>  
> -static int write_output(struct perf_record *rec, void *buf, size_t size)
> -{
> -	return do_write_output(rec, buf, size);
> -}
> -
>  static int process_synthesized_event(struct perf_tool *tool,
>  				     union perf_event *event,
>  				     struct perf_sample *sample __maybe_unused,
>  				     struct machine *machine __maybe_unused)
>  {
>  	struct perf_record *rec = container_of(tool, struct perf_record, tool);
> -	if (write_output(rec, event, event->header.size) < 0)
> -		return -1;
> -
> -	return 0;
> +	return perf_record__write(rec, event, event->header.size);
>  }
>  
>  static int perf_record__mmap_read(struct perf_record *rec,
> @@ -136,7 +121,7 @@ static int perf_record__mmap_read(struct perf_record *rec,
>  		size = md->mask + 1 - (old & md->mask);
>  		old += size;
>  
> -		if (write_output(rec, buf, size) < 0) {
> +		if (perf_record__write(rec, buf, size) < 0) {
>  			rc = -1;
>  			goto out;
>  		}
> @@ -146,7 +131,7 @@ static int perf_record__mmap_read(struct perf_record *rec,
>  	size = head - old;
>  	old += size;
>  
> -	if (write_output(rec, buf, size) < 0) {
> +	if (perf_record__write(rec, buf, size) < 0) {
>  		rc = -1;
>  		goto out;
>  	}
> @@ -322,8 +307,7 @@ static struct perf_event_header finished_round_event = {
>  
>  static int perf_record__mmap_read_all(struct perf_record *rec)
>  {
> -	int i;
> -	int rc = 0;
> +	int i, rc = 0;
>  
>  	for (i = 0; i < rec->evlist->nr_mmaps; i++) {
>  		if (rec->evlist->mmap[i].base) {
> @@ -335,7 +319,7 @@ static int perf_record__mmap_read_all(struct perf_record *rec)
>  	}
>  
>  	if (perf_header__has_feat(&rec->session->header, HEADER_TRACING_DATA))
> -		rc = write_output(rec, &finished_round_event,
> +		rc = perf_record__write(rec, &finished_round_event,
>  				  sizeof(finished_round_event));
>  
>  out:
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] perf record: Use perf_data_file__write for output file

[Jiri Olsa]

On Mon, Nov 25, 2013 at 04:31:37PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 22, 2013 at 03:24:27PM +0100, Jiri Olsa escreveu:
> > Changing the file output code to use the newly
> > added perf_data_file__write interface.
> 
> So I like renaming write_output() to perf_record__write(), but then
> you're lumping together this change and the other, which is to make
> write_output, now renamed to perf_record__write() to use perf_data_file.
> 
> Can you please split it like that?
> 

ok, will do

thanks,
jirka

[PATCH 3/3] perf inject: Handle output file via perf_data_file object

[Jiri Olsa]

Using the perf_data_file object to handle output
file processing.

No functional change intended.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Stephane Eranian <eranian@google.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
---
 tools/perf/builtin-inject.c | 71 ++++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 40 deletions(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 6a25085..654a33e 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -22,14 +22,13 @@
 #include <linux/list.h>
 
 struct perf_inject {
-	struct perf_tool tool;
-	bool		 build_ids;
-	bool		 sched_stat;
-	const char	 *input_name;
-	int		 pipe_output,
-			 output;
-	u64		 bytes_written;
-	struct list_head samples;
+	struct perf_tool	 tool;
+	bool			 build_ids;
+	bool			 sched_stat;
+	const char		*input_name;
+	struct perf_data_file	 output;
+	u64			 bytes_written;
+	struct list_head	 samples;
 };
 
 struct event_entry {
@@ -42,21 +41,14 @@ static int perf_event__repipe_synth(struct perf_tool *tool,
 				    union perf_event *event)
 {
 	struct perf_inject *inject = container_of(tool, struct perf_inject, tool);
-	uint32_t size;
-	void *buf = event;
+	ssize_t size;
 
-	size = event->header.size;
-
-	while (size) {
-		int ret = write(inject->output, buf, size);
-		if (ret < 0)
-			return -errno;
-
-		size -= ret;
-		buf += ret;
-		inject->bytes_written += ret;
-	}
+	size = perf_data_file__write(&inject->output, event,
+				     event->header.size);
+	if (size < 0)
+		return -errno;
 
+	inject->bytes_written += size;
 	return 0;
 }
 
@@ -80,7 +72,7 @@ static int perf_event__repipe_attr(struct perf_tool *tool,
 	if (ret)
 		return ret;
 
-	if (!inject->pipe_output)
+	if (!perf_data_file__is_pipe(&inject->output))
 		return 0;
 
 	return perf_event__repipe_synth(tool, event);
@@ -351,10 +343,12 @@ static int __cmd_inject(struct perf_inject *inject)
 {
 	struct perf_session *session;
 	int ret = -EINVAL;
-	struct perf_data_file file = {
+	struct perf_data_file file_in = {
 		.path = inject->input_name,
 		.mode = PERF_DATA_MODE_READ,
 	};
+	struct perf_data_file *file_out = &inject->output;
+	int out_fd = perf_data_file__fd(file_out);
 
 	signal(SIGINT, sig_handler);
 
@@ -365,7 +359,7 @@ static int __cmd_inject(struct perf_inject *inject)
 		inject->tool.tracing_data = perf_event__repipe_tracing_data;
 	}
 
-	session = perf_session__new(&file, true, &inject->tool);
+	session = perf_session__new(&file_in, true, &inject->tool);
 	if (session == NULL)
 		return -ENOMEM;
 
@@ -391,14 +385,15 @@ static int __cmd_inject(struct perf_inject *inject)
 		}
 	}
 
-	if (!inject->pipe_output)
-		lseek(inject->output, session->header.data_offset, SEEK_SET);
+	if (!perf_data_file__is_pipe(file_out))
+		lseek(out_fd, session->header.data_offset, SEEK_SET);
 
 	ret = perf_session__process_events(session, &inject->tool);
 
-	if (!inject->pipe_output) {
+	if (!perf_data_file__is_pipe(file_out)) {
 		session->header.data_size = inject->bytes_written;
-		perf_session__write_header(session, session->evlist, inject->output, true);
+		perf_session__write_header(session, session->evlist, out_fd,
+					   true);
 	}
 
 	perf_session__delete(session);
@@ -427,14 +422,17 @@ int cmd_inject(int argc, const char **argv, const char *prefix __maybe_unused)
 		},
 		.input_name  = "-",
 		.samples = LIST_HEAD_INIT(inject.samples),
+		.output = {
+			.path = "-",
+			.mode = PERF_DATA_MODE_WRITE,
+		},
 	};
-	const char *output_name = "-";
 	const struct option options[] = {
 		OPT_BOOLEAN('b', "build-ids", &inject.build_ids,
 			    "Inject build-ids into the output stream"),
 		OPT_STRING('i', "input", &inject.input_name, "file",
 			   "input file name"),
-		OPT_STRING('o', "output", &output_name, "file",
+		OPT_STRING('o', "output", &inject.output.path, "file",
 			   "output file name"),
 		OPT_BOOLEAN('s', "sched-stat", &inject.sched_stat,
 			    "Merge sched-stat and sched-switch for getting events "
@@ -456,16 +454,9 @@ int cmd_inject(int argc, const char **argv, const char *prefix __maybe_unused)
 	if (argc)
 		usage_with_options(inject_usage, options);
 
-	if (!strcmp(output_name, "-")) {
-		inject.pipe_output = 1;
-		inject.output = STDOUT_FILENO;
-	} else {
-		inject.output = open(output_name, O_CREAT | O_WRONLY | O_TRUNC,
-						  S_IRUSR | S_IWUSR);
-		if (inject.output < 0) {
-			perror("failed to create output file");
-			return -1;
-		}
+	if (perf_data_file__open(&inject.output)) {
+		perror("failed to create output file");
+		return -1;
 	}
 
 	if (symbol__init() < 0)
-- 
1.8.3.1

Re: [PATCH 3/3] perf inject: Handle output file via perf_data_file object

[Arnaldo Carvalho de Melo]

Em Fri, Nov 22, 2013 at 03:24:28PM +0100, Jiri Olsa escreveu:
> Using the perf_data_file object to handle output
> file processing.
> 
> No functional change intended.
> 
> Signed-off-by: Jiri Olsa <jolsa@redhat.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Stephane Eranian <eranian@google.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/builtin-inject.c | 71 ++++++++++++++++++++-------------------------
>  1 file changed, 31 insertions(+), 40 deletions(-)
> 
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 6a25085..654a33e 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -22,14 +22,13 @@
>  #include <linux/list.h>
>  
>  struct perf_inject {
> -	struct perf_tool tool;
> -	bool		 build_ids;
> -	bool		 sched_stat;
> -	const char	 *input_name;
> -	int		 pipe_output,
> -			 output;
> -	u64		 bytes_written;
> -	struct list_head samples;
> +	struct perf_tool	 tool;
> +	bool			 build_ids;
> +	bool			 sched_stat;
> +	const char		*input_name;
> +	struct perf_data_file	 output;
> +	u64			 bytes_written;
> +	struct list_head	 samples;
>  };
>  
>  struct event_entry {
> @@ -42,21 +41,14 @@ static int perf_event__repipe_synth(struct perf_tool *tool,
>  				    union perf_event *event)
>  {
>  	struct perf_inject *inject = container_of(tool, struct perf_inject, tool);
> -	uint32_t size;
> -	void *buf = event;
> +	ssize_t size;
>  
> -	size = event->header.size;
> -
> -	while (size) {
> -		int ret = write(inject->output, buf, size);
> -		if (ret < 0)
> -			return -errno;
> -
> -		size -= ret;
> -		buf += ret;
> -		inject->bytes_written += ret;
> -	}
> +	size = perf_data_file__write(&inject->output, event,
> +				     event->header.size);
> +	if (size < 0)
> +		return -errno;
>  
> +	inject->bytes_written += size;
>  	return 0;
>  }
>  
> @@ -80,7 +72,7 @@ static int perf_event__repipe_attr(struct perf_tool *tool,
>  	if (ret)
>  		return ret;
>  
> -	if (!inject->pipe_output)
> +	if (!perf_data_file__is_pipe(&inject->output))
>  		return 0;
>  
>  	return perf_event__repipe_synth(tool, event);
> @@ -351,10 +343,12 @@ static int __cmd_inject(struct perf_inject *inject)
>  {
>  	struct perf_session *session;
>  	int ret = -EINVAL;
> -	struct perf_data_file file = {
> +	struct perf_data_file file_in = {

Why don't leave it as 'file', and on a follow up patch _then_ rename it
to file_in? This way patch review gets easier, i.e. try avoiding doing
multiple things per patch.

>  		.path = inject->input_name,
>  		.mode = PERF_DATA_MODE_READ,
>  	};
> +	struct perf_data_file *file_out = &inject->output;
> +	int out_fd = perf_data_file__fd(file_out);
>  
>  	signal(SIGINT, sig_handler);
>  
> @@ -365,7 +359,7 @@ static int __cmd_inject(struct perf_inject *inject)
>  		inject->tool.tracing_data = perf_event__repipe_tracing_data;
>  	}
>  
> -	session = perf_session__new(&file, true, &inject->tool);
> +	session = perf_session__new(&file_in, true, &inject->tool);

This hunk, for example, wouldn't be here, the this patch would be
shorter, easier to review.

>  	if (session == NULL)
>  		return -ENOMEM;
>  
> @@ -391,14 +385,15 @@ static int __cmd_inject(struct perf_inject *inject)
>  		}
>  	}
>  
> -	if (!inject->pipe_output)
> -		lseek(inject->output, session->header.data_offset, SEEK_SET);
> +	if (!perf_data_file__is_pipe(file_out))
> +		lseek(out_fd, session->header.data_offset, SEEK_SET);

Couldn't this be left as:

-	if (!inject->pipe_output)
-		lseek(inject->output, session->header.data_offset, SEEK_SET);
+	if (!perf_data_file__is_pipe(file_out))
+		lseek(inject->output->fd, session->header.data_offset, SEEK_SET);

I.e. why wrap access to the fd like that?

>  
>  	ret = perf_session__process_events(session, &inject->tool);
>  
> -	if (!inject->pipe_output) {
> +	if (!perf_data_file__is_pipe(file_out)) {
>  		session->header.data_size = inject->bytes_written;
> -		perf_session__write_header(session, session->evlist, inject->output, true);
> +		perf_session__write_header(session, session->evlist, out_fd,
> +					   true);

Why a line for 'true' all by itself?

>  	}
>  
>  	perf_session__delete(session);
> @@ -427,14 +422,17 @@ int cmd_inject(int argc, const char **argv, const char *prefix __maybe_unused)
>  		},
>  		.input_name  = "-",
>  		.samples = LIST_HEAD_INIT(inject.samples),
> +		.output = {
> +			.path = "-",
> +			.mode = PERF_DATA_MODE_WRITE,
> +		},
>  	};
> -	const char *output_name = "-";
>  	const struct option options[] = {
>  		OPT_BOOLEAN('b', "build-ids", &inject.build_ids,
>  			    "Inject build-ids into the output stream"),
>  		OPT_STRING('i', "input", &inject.input_name, "file",
>  			   "input file name"),
> -		OPT_STRING('o', "output", &output_name, "file",
> +		OPT_STRING('o', "output", &inject.output.path, "file",

see, here you directly access a perf_data_file member instead of having
another wrapper :-)

>  			   "output file name"),
>  		OPT_BOOLEAN('s', "sched-stat", &inject.sched_stat,
>  			    "Merge sched-stat and sched-switch for getting events "
> @@ -456,16 +454,9 @@ int cmd_inject(int argc, const char **argv, const char *prefix __maybe_unused)
>  	if (argc)
>  		usage_with_options(inject_usage, options);
>  
> -	if (!strcmp(output_name, "-")) {
> -		inject.pipe_output = 1;
> -		inject.output = STDOUT_FILENO;
> -	} else {
> -		inject.output = open(output_name, O_CREAT | O_WRONLY | O_TRUNC,
> -						  S_IRUSR | S_IWUSR);
> -		if (inject.output < 0) {
> -			perror("failed to create output file");
> -			return -1;
> -		}
> +	if (perf_data_file__open(&inject.output)) {
> +		perror("failed to create output file");
> +		return -1;
>  	}
>  
>  	if (symbol__init() < 0)
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] perf inject: Handle output file via perf_data_file object

[Jiri Olsa]

On Mon, Nov 25, 2013 at 04:40:32PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 22, 2013 at 03:24:28PM +0100, Jiri Olsa escreveu:
> > Using the perf_data_file object to handle output

SNIP

> > +	if (!perf_data_file__is_pipe(&inject->output))
> >  		return 0;
> >  
> >  	return perf_event__repipe_synth(tool, event);
> > @@ -351,10 +343,12 @@ static int __cmd_inject(struct perf_inject *inject)
> >  {
> >  	struct perf_session *session;
> >  	int ret = -EINVAL;
> > -	struct perf_data_file file = {
> > +	struct perf_data_file file_in = {
> 
> Why don't leave it as 'file', and on a follow up patch _then_ rename it
> to file_in? This way patch review gets easier, i.e. try avoiding doing
> multiple things per patch.

the input file needed to be renamed, because new 'output' file was added

> 
> >  		.path = inject->input_name,
> >  		.mode = PERF_DATA_MODE_READ,
> >  	};
> > +	struct perf_data_file *file_out = &inject->output;
> > +	int out_fd = perf_data_file__fd(file_out);
> >  
> >  	signal(SIGINT, sig_handler);
> >  
> > @@ -365,7 +359,7 @@ static int __cmd_inject(struct perf_inject *inject)
> >  		inject->tool.tracing_data = perf_event__repipe_tracing_data;
> >  	}
> >  
> > -	session = perf_session__new(&file, true, &inject->tool);
> > +	session = perf_session__new(&file_in, true, &inject->tool);
> 
> This hunk, for example, wouldn't be here, the this patch would be
> shorter, easier to review.
> 
> >  	if (session == NULL)
> >  		return -ENOMEM;
> >  
> > @@ -391,14 +385,15 @@ static int __cmd_inject(struct perf_inject *inject)
> >  		}
> >  	}
> >  
> > -	if (!inject->pipe_output)
> > -		lseek(inject->output, session->header.data_offset, SEEK_SET);
> > +	if (!perf_data_file__is_pipe(file_out))
> > +		lseek(out_fd, session->header.data_offset, SEEK_SET);
> 
> Couldn't this be left as:
> 
> -	if (!inject->pipe_output)
> -		lseek(inject->output, session->header.data_offset, SEEK_SET);
> +	if (!perf_data_file__is_pipe(file_out))
> +		lseek(inject->output->fd, session->header.data_offset, SEEK_SET);
> 
> I.e. why wrap access to the fd like that?

well, inject->output->fd is used on 2 places within the function,
so it seems logical to put it into variable and use it like that

> 
> >  
> >  	ret = perf_session__process_events(session, &inject->tool);
> >  
> > -	if (!inject->pipe_output) {
> > +	if (!perf_data_file__is_pipe(file_out)) {
> >  		session->header.data_size = inject->bytes_written;
> > -		perf_session__write_header(session, session->evlist, inject->output, true);
> > +		perf_session__write_header(session, session->evlist, out_fd,
> > +					   true);
> 
> Why a line for 'true' all by itself?

line was crossing 80 chars limit

> 
> >  	}
> >  
> >  	perf_session__delete(session);
> > @@ -427,14 +422,17 @@ int cmd_inject(int argc, const char **argv, const char *prefix __maybe_unused)
> >  		},
> >  		.input_name  = "-",
> >  		.samples = LIST_HEAD_INIT(inject.samples),
> > +		.output = {
> > +			.path = "-",
> > +			.mode = PERF_DATA_MODE_WRITE,
> > +		},
> >  	};
> > -	const char *output_name = "-";
> >  	const struct option options[] = {
> >  		OPT_BOOLEAN('b', "build-ids", &inject.build_ids,
> >  			    "Inject build-ids into the output stream"),
> >  		OPT_STRING('i', "input", &inject.input_name, "file",
> >  			   "input file name"),
> > -		OPT_STRING('o', "output", &output_name, "file",
> > +		OPT_STRING('o', "output", &inject.output.path, "file",
> 
> see, here you directly access a perf_data_file member instead of having
> another wrapper :-)

yes

I dont have strong opinions about wrappers, sometimes it seems
appropriate, sometimes it does not.. tell me the guidance here
and I'll kick the patch to fit ;-)

thanks,
jirka

Re: [PATCH 3/3] perf inject: Handle output file via perf_data_file object

[Arnaldo Carvalho de Melo]

Em Tue, Nov 26, 2013 at 11:03:24AM +0100, Jiri Olsa escreveu:
> On Mon, Nov 25, 2013 at 04:40:32PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Nov 22, 2013 at 03:24:28PM +0100, Jiri Olsa escreveu:
> > > Using the perf_data_file object to handle output
 
> SNIP
 
> > > +	if (!perf_data_file__is_pipe(&inject->output))
> > >  		return 0;

> > >  	return perf_event__repipe_synth(tool, event);
> > > @@ -351,10 +343,12 @@ static int __cmd_inject(struct perf_inject *inject)
> > >  {
> > >  	struct perf_session *session;
> > >  	int ret = -EINVAL;
> > > -	struct perf_data_file file = {
> > > +	struct perf_data_file file_in = {
> > 
> > Why don't leave it as 'file', and on a follow up patch _then_ rename it
> > to file_in? This way patch review gets easier, i.e. try avoiding doing
> > multiple things per patch.
> 
> the input file needed to be renamed, because new 'output' file was added

Why? Is 'file' going to be reused somehow?
 
> > 
> > >  		.path = inject->input_name,
> > >  		.mode = PERF_DATA_MODE_READ,
> > >  	};
> > > +	struct perf_data_file *file_out = &inject->output;
> > > +	int out_fd = perf_data_file__fd(file_out);
> > >  
> > >  	signal(SIGINT, sig_handler);
> > >  
> > > @@ -365,7 +359,7 @@ static int __cmd_inject(struct perf_inject *inject)
> > >  		inject->tool.tracing_data = perf_event__repipe_tracing_data;
> > >  	}
> > >  
> > > -	session = perf_session__new(&file, true, &inject->tool);
> > > +	session = perf_session__new(&file_in, true, &inject->tool);
> > 
> > This hunk, for example, wouldn't be here, the this patch would be
> > shorter, easier to review.
> > 
> > >  	if (session == NULL)
> > >  		return -ENOMEM;
> > >  
> > > @@ -391,14 +385,15 @@ static int __cmd_inject(struct perf_inject *inject)
> > >  		}
> > >  	}
> > >  
> > > -	if (!inject->pipe_output)
> > > -		lseek(inject->output, session->header.data_offset, SEEK_SET);
> > > +	if (!perf_data_file__is_pipe(file_out))
> > > +		lseek(out_fd, session->header.data_offset, SEEK_SET);
> > 
> > Couldn't this be left as:
> > 
> > -	if (!inject->pipe_output)
> > -		lseek(inject->output, session->header.data_offset, SEEK_SET);
> > +	if (!perf_data_file__is_pipe(file_out))
> > +		lseek(inject->output->fd, session->header.data_offset, SEEK_SET);
> > 
> > I.e. why wrap access to the fd like that?
> 
> well, inject->output->fd is used on 2 places within the function,
> so it seems logical to put it into variable and use it like that

What I'm trying to convey here is that for both this case and the other,
having looking at these two lines:

-                 inject->output
+                 inject->output->fd

Makes me instantaneously understand that inject->output now
encapsulates, among other things (probably), the file descriptor that
was called just inject->output, i.e. this patch probably isn't doing
anything more than using a new abstraction, the code flow probably
wasn't altered.

I.e. the smaller the patch, the better.

> > > -	if (!inject->pipe_output) {
> > > +	if (!perf_data_file__is_pipe(file_out)) {
> > >  		session->header.data_size = inject->bytes_written;
> > > -		perf_session__write_header(session, session->evlist, inject->output, true);
+		perf_session__write_header(session, session->evlist, inject->output->fd, true);
> > 
> > Why a line for 'true' all by itself?
> 
> line was crossing 80 chars limit

[1]+  Stopped                 mutt
[acme@zoo ~]$ 
[acme@zoo ~]$ echo $COLUMNS
173
[acme@zoo ~]$ 

I'm not really that strict with that old convention :-\ All in one line
would make it ~95 columns, not a big deal, even more since it _was_
already more than 80 columns.

I.e. your change was to replace 'inject->output' with 'out_fd', but you
did that _and_ reflowed, i.e. two changes into one. ;-)

Looking at this line makes me think: why do we have to pass 'session'
_and_ 'session->evlist', i.e. the 2nd parameter can be obtained from the
1st. Fixing that could get us more compact code _and_ a shorter line.

Will check that.

> > > -		OPT_STRING('o', "output", &output_name, "file",
> > > +		OPT_STRING('o', "output", &inject.output.path, "file",
> > 
> > see, here you directly access a perf_data_file member instead of having
> > another wrapper :-)
> 
> yes
> 
> I dont have strong opinions about wrappers, sometimes it seems
> appropriate, sometimes it does not.. tell me the guidance here
> and I'll kick the patch to fit ;-)

Well, a wrapper like perf_data_file__is_pipe(file_out) that maps to
file_out->is_pipe and will produce the same results at every call and
that we don't have the slightest intention of somehow hooking, I would
do away with it and use file_out->is_pipe directly.

- Arnaldo

Re: [PATCH 3/3] perf inject: Handle output file via perf_data_file object

[Jiri Olsa]

On Tue, Nov 26, 2013 at 09:42:13AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Nov 26, 2013 at 11:03:24AM +0100, Jiri Olsa escreveu:
> > On Mon, Nov 25, 2013 at 04:40:32PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Fri, Nov 22, 2013 at 03:24:28PM +0100, Jiri Olsa escreveu:
> > > > Using the perf_data_file object to handle output
>  
> > SNIP
>  
> > > > +	if (!perf_data_file__is_pipe(&inject->output))
> > > >  		return 0;
> 
> > > >  	return perf_event__repipe_synth(tool, event);
> > > > @@ -351,10 +343,12 @@ static int __cmd_inject(struct perf_inject *inject)
> > > >  {
> > > >  	struct perf_session *session;
> > > >  	int ret = -EINVAL;
> > > > -	struct perf_data_file file = {
> > > > +	struct perf_data_file file_in = {
> > > 
> > > Why don't leave it as 'file', and on a follow up patch _then_ rename it
> > > to file_in? This way patch review gets easier, i.e. try avoiding doing
> > > multiple things per patch.
> > 
> > the input file needed to be renamed, because new 'output' file was added
> 
> Why? Is 'file' going to be reused somehow?

nope, just having file_in and file_out seemed symmetric,
but nevermind.. I can switch to file and file_out ;-)

SNIP

> > 
> > well, inject->output->fd is used on 2 places within the function,
> > so it seems logical to put it into variable and use it like that
> 
> What I'm trying to convey here is that for both this case and the other,
> having looking at these two lines:
> 
> -                 inject->output
> +                 inject->output->fd
> 
> Makes me instantaneously understand that inject->output now
> encapsulates, among other things (probably), the file descriptor that
> was called just inject->output, i.e. this patch probably isn't doing
> anything more than using a new abstraction, the code flow probably
> wasn't altered.

yes, that's what happened.. encapsulating output file processing
and getting rid of common code that's now handled by perf_data_file
object..

> 
> I.e. the smaller the patch, the better.

I dont think thats always true.. I prefer readable code
despite of 'unreadable' patches ;-)

> 
> > > > -	if (!inject->pipe_output) {
> > > > +	if (!perf_data_file__is_pipe(file_out)) {
> > > >  		session->header.data_size = inject->bytes_written;
> > > > -		perf_session__write_header(session, session->evlist, inject->output, true);
> +		perf_session__write_header(session, session->evlist, inject->output->fd, true);
> > > 
> > > Why a line for 'true' all by itself?
> > 
> > line was crossing 80 chars limit
> 
> [1]+  Stopped                 mutt
> [acme@zoo ~]$ 
> [acme@zoo ~]$ echo $COLUMNS
> 173
> [acme@zoo ~]$ 
> 
> I'm not really that strict with that old convention :-\ All in one line
> would make it ~95 columns, not a big deal, even more since it _was_
> already more than 80 columns.

I see.. I'm using checkpatch script that screams about this
so I tend to keep to that rule

> 
> I.e. your change was to replace 'inject->output' with 'out_fd', but you
> did that _and_ reflowed, i.e. two changes into one. ;-)

the smaller the patch, the better, right? :-)

> 
> Looking at this line makes me think: why do we have to pass 'session'
> _and_ 'session->evlist', i.e. the 2nd parameter can be obtained from the
> 1st. Fixing that could get us more compact code _and_ a shorter line.

we do actually, and the reason is this code, because
the session keeps the input file, while we are using
it for output file..

> 
> Will check that.
> 
> > > > -		OPT_STRING('o', "output", &output_name, "file",
> > > > +		OPT_STRING('o', "output", &inject.output.path, "file",
> > > 
> > > see, here you directly access a perf_data_file member instead of having
> > > another wrapper :-)
> > 
> > yes
> > 
> > I dont have strong opinions about wrappers, sometimes it seems
> > appropriate, sometimes it does not.. tell me the guidance here
> > and I'll kick the patch to fit ;-)
> 
> Well, a wrapper like perf_data_file__is_pipe(file_out) that maps to
> file_out->is_pipe and will produce the same results at every call and
> that we don't have the slightest intention of somehow hooking, I would
> do away with it and use file_out->is_pipe directly.

ok

thanks,
jirka

Re: [PATCH 0/3] perf tools: Add data file write interface

[David Ahern]

On 11/22/13, 7:24 AM, Jiri Olsa wrote:
> David,
> patch 2 could colide with your mmap changes, not sure
> what's your plan with that in near future, let me know
> and I can rebase if needed.

The day job requires some focus for a few days. I was hoping to revisit 
the mmap output end of next week during the holidays here. Make mmap 
optional -- default to write with an option for mmap. Add some builtin 
detection to do the right thing if the user did not specify a preference 
-- system wide tracing should flip to mmap, system wide faults should 
write. Log a warning to the user if the requested option goes against 
that. If Peter or Ingo have other thoughts or ideas to the contrary I'd 
love to hear them before the next iteration.

David

Re: [PATCH 0/3] perf tools: Add data file write interface

[Jiri Olsa]

On Fri, Nov 22, 2013 at 08:01:47AM -0700, David Ahern wrote:
> On 11/22/13, 7:24 AM, Jiri Olsa wrote:
> >David,
> >patch 2 could colide with your mmap changes, not sure
> >what's your plan with that in near future, let me know
> >and I can rebase if needed.
> 
> The day job requires some focus for a few days. I was hoping to
> revisit the mmap output end of next week during the holidays here.
> Make mmap optional -- default to write with an option for mmap. Add
> some builtin detection to do the right thing if the user did not
> specify a preference -- system wide tracing should flip to mmap,
> system wide faults should write. Log a warning to the user if the
> requested option goes against that. If Peter or Ingo have other
> thoughts or ideas to the contrary I'd love to hear them before the
> next iteration.

so.. I understand you'll make changes anyway
and dont mind to rebase? ;-)

thanks,
jirka

Re: [PATCH 0/3] perf tools: Add data file write interface

[David Ahern]

On 11/23/13, 2:44 AM, Jiri Olsa wrote:
>
> so.. I understand you'll make changes anyway
> and dont mind to rebase? ;-)

I can rebase.

David