Re: [PATCH 6/7] perf auxtrace: Make auxtrace_queues__add_buffer() allocate struct buffer

[Arnaldo Carvalho de Melo]

Em Thu, Apr 05, 2018 at 09:17:01AM +0300, Adrian Hunter escreveu:
> On 07/03/18 16:11, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Mar 07, 2018 at 10:06:50AM +0200, Adrian Hunter escreveu:
> >> On 06/03/18 22:25, Arnaldo Carvalho de Melo wrote:
> >>> Em Tue, Mar 06, 2018 at 11:13:17AM +0200, Adrian Hunter escreveu:
>  In preparation for supporting AUX area sampling buffers,
>  auxtrace_queues__add_buffer() needs to be more generic. To that end, move
>  memory allocation for struct buffer into it.
> 
>  Signed-off-by: Adrian Hunter 
>  ---
>   tools/perf/util/auxtrace.c | 54 
>  +-
>   1 file changed, 24 insertions(+), 30 deletions(-)
> 
>  diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
>  index fb357a00dd86..e1aff91c54a8 100644
>  --- a/tools/perf/util/auxtrace.c
>  +++ b/tools/perf/util/auxtrace.c
>  @@ -308,7 +308,11 @@ static int auxtrace_queues__add_buffer(struct 
>  auxtrace_queues *queues,
>  struct auxtrace_buffer *buffer,
>  struct auxtrace_buffer 
>  **buffer_ptr)
>   {
>  -int err;
>  +int err = -ENOMEM;
>  +
>  +buffer = memdup(buffer, sizeof(*buffer));
> >>>
> >>> this is a bit strange, why not make buffer a local variable in this
> >>> function then?
> >>
> >> Do you mean the following?
> >>
> >>struct auxtrace_buffer *new_buf;
> >>
> >>new_buf = memdup(buffer, sizeof(*buffer));
> > 
> > I hadn't noticed that you were using buffer as both r and l value :-\
> > 
> > If all you want is to receive that buffer, duplicate it and then use
> > just the duplicate, not needing any reference to the original buffer,
> > then your code is correct, it just looked strange from a quick look, so
> > nevermind, I'll continue processing this one and the others.
> 
> Looks like this patch and patch 7 "perf auxtrace: Make
> auxtrace_queues__add_buffer() do CPU filtering" got left behind.  They still
> apply cleanly.

Thanks for the reminder, I'll process it.

- Arnaldo

Re: [PATCH 6/7] perf auxtrace: Make auxtrace_queues__add_buffer() allocate struct buffer

[Arnaldo Carvalho de Melo]

Em Thu, Apr 05, 2018 at 09:17:01AM +0300, Adrian Hunter escreveu:
> On 07/03/18 16:11, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Mar 07, 2018 at 10:06:50AM +0200, Adrian Hunter escreveu:
> >> On 06/03/18 22:25, Arnaldo Carvalho de Melo wrote:
> >>> Em Tue, Mar 06, 2018 at 11:13:17AM +0200, Adrian Hunter escreveu:
>  In preparation for supporting AUX area sampling buffers,
>  auxtrace_queues__add_buffer() needs to be more generic. To that end, move
>  memory allocation for struct buffer into it.
> 
>  Signed-off-by: Adrian Hunter 
>  ---
>   tools/perf/util/auxtrace.c | 54 
>  +-
>   1 file changed, 24 insertions(+), 30 deletions(-)
> 
>  diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
>  index fb357a00dd86..e1aff91c54a8 100644
>  --- a/tools/perf/util/auxtrace.c
>  +++ b/tools/perf/util/auxtrace.c
>  @@ -308,7 +308,11 @@ static int auxtrace_queues__add_buffer(struct 
>  auxtrace_queues *queues,
>  struct auxtrace_buffer *buffer,
>  struct auxtrace_buffer 
>  **buffer_ptr)
>   {
>  -int err;
>  +int err = -ENOMEM;
>  +
>  +buffer = memdup(buffer, sizeof(*buffer));
> >>>
> >>> this is a bit strange, why not make buffer a local variable in this
> >>> function then?
> >>
> >> Do you mean the following?
> >>
> >>struct auxtrace_buffer *new_buf;
> >>
> >>new_buf = memdup(buffer, sizeof(*buffer));
> > 
> > I hadn't noticed that you were using buffer as both r and l value :-\
> > 
> > If all you want is to receive that buffer, duplicate it and then use
> > just the duplicate, not needing any reference to the original buffer,
> > then your code is correct, it just looked strange from a quick look, so
> > nevermind, I'll continue processing this one and the others.
> 
> Looks like this patch and patch 7 "perf auxtrace: Make
> auxtrace_queues__add_buffer() do CPU filtering" got left behind.  They still
> apply cleanly.

Thanks for the reminder, I'll process it.

- Arnaldo

Re: [PATCH 6/7] perf auxtrace: Make auxtrace_queues__add_buffer() allocate struct buffer

[Adrian Hunter]

On 07/03/18 16:11, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 07, 2018 at 10:06:50AM +0200, Adrian Hunter escreveu:
>> On 06/03/18 22:25, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Mar 06, 2018 at 11:13:17AM +0200, Adrian Hunter escreveu:
 In preparation for supporting AUX area sampling buffers,
 auxtrace_queues__add_buffer() needs to be more generic. To that end, move
 memory allocation for struct buffer into it.

 Signed-off-by: Adrian Hunter 
 ---
  tools/perf/util/auxtrace.c | 54 
 +-
  1 file changed, 24 insertions(+), 30 deletions(-)

 diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
 index fb357a00dd86..e1aff91c54a8 100644
 --- a/tools/perf/util/auxtrace.c
 +++ b/tools/perf/util/auxtrace.c
 @@ -308,7 +308,11 @@ static int auxtrace_queues__add_buffer(struct 
 auxtrace_queues *queues,
   struct auxtrace_buffer *buffer,
   struct auxtrace_buffer **buffer_ptr)
  {
 -  int err;
 +  int err = -ENOMEM;
 +
 +  buffer = memdup(buffer, sizeof(*buffer));
>>>
>>> this is a bit strange, why not make buffer a local variable in this
>>> function then?
>>
>> Do you mean the following?
>>
>>  struct auxtrace_buffer *new_buf;
>>
>>  new_buf = memdup(buffer, sizeof(*buffer));
> 
> I hadn't noticed that you were using buffer as both r and l value :-\
> 
> If all you want is to receive that buffer, duplicate it and then use
> just the duplicate, not needing any reference to the original buffer,
> then your code is correct, it just looked strange from a quick look, so
> nevermind, I'll continue processing this one and the others.

Looks like this patch and patch 7 "perf auxtrace: Make
auxtrace_queues__add_buffer() do CPU filtering" got left behind.  They still
apply cleanly.

Re: [PATCH 6/7] perf auxtrace: Make auxtrace_queues__add_buffer() allocate struct buffer

[Adrian Hunter]

On 07/03/18 16:11, Arnaldo Carvalho de Melo wrote:
> Em Wed, Mar 07, 2018 at 10:06:50AM +0200, Adrian Hunter escreveu:
>> On 06/03/18 22:25, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Mar 06, 2018 at 11:13:17AM +0200, Adrian Hunter escreveu:
 In preparation for supporting AUX area sampling buffers,
 auxtrace_queues__add_buffer() needs to be more generic. To that end, move
 memory allocation for struct buffer into it.

 Signed-off-by: Adrian Hunter 
 ---
  tools/perf/util/auxtrace.c | 54 
 +-
  1 file changed, 24 insertions(+), 30 deletions(-)

 diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
 index fb357a00dd86..e1aff91c54a8 100644
 --- a/tools/perf/util/auxtrace.c
 +++ b/tools/perf/util/auxtrace.c
 @@ -308,7 +308,11 @@ static int auxtrace_queues__add_buffer(struct 
 auxtrace_queues *queues,
   struct auxtrace_buffer *buffer,
   struct auxtrace_buffer **buffer_ptr)
  {
 -  int err;
 +  int err = -ENOMEM;
 +
 +  buffer = memdup(buffer, sizeof(*buffer));
>>>
>>> this is a bit strange, why not make buffer a local variable in this
>>> function then?
>>
>> Do you mean the following?
>>
>>  struct auxtrace_buffer *new_buf;
>>
>>  new_buf = memdup(buffer, sizeof(*buffer));
> 
> I hadn't noticed that you were using buffer as both r and l value :-\
> 
> If all you want is to receive that buffer, duplicate it and then use
> just the duplicate, not needing any reference to the original buffer,
> then your code is correct, it just looked strange from a quick look, so
> nevermind, I'll continue processing this one and the others.

Looks like this patch and patch 7 "perf auxtrace: Make
auxtrace_queues__add_buffer() do CPU filtering" got left behind.  They still
apply cleanly.

Re: [PATCH 6/7] perf auxtrace: Make auxtrace_queues__add_buffer() allocate struct buffer

[Arnaldo Carvalho de Melo]

Em Wed, Mar 07, 2018 at 10:06:50AM +0200, Adrian Hunter escreveu:
> On 06/03/18 22:25, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Mar 06, 2018 at 11:13:17AM +0200, Adrian Hunter escreveu:
> >> In preparation for supporting AUX area sampling buffers,
> >> auxtrace_queues__add_buffer() needs to be more generic. To that end, move
> >> memory allocation for struct buffer into it.
> >>
> >> Signed-off-by: Adrian Hunter 
> >> ---
> >>  tools/perf/util/auxtrace.c | 54 
> >> +-
> >>  1 file changed, 24 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> >> index fb357a00dd86..e1aff91c54a8 100644
> >> --- a/tools/perf/util/auxtrace.c
> >> +++ b/tools/perf/util/auxtrace.c
> >> @@ -308,7 +308,11 @@ static int auxtrace_queues__add_buffer(struct 
> >> auxtrace_queues *queues,
> >>   struct auxtrace_buffer *buffer,
> >>   struct auxtrace_buffer **buffer_ptr)
> >>  {
> >> -  int err;
> >> +  int err = -ENOMEM;
> >> +
> >> +  buffer = memdup(buffer, sizeof(*buffer));
> > 
> > this is a bit strange, why not make buffer a local variable in this
> > function then?
> 
> Do you mean the following?
> 
>   struct auxtrace_buffer *new_buf;
> 
>   new_buf = memdup(buffer, sizeof(*buffer));

I hadn't noticed that you were using buffer as both r and l value :-\

If all you want is to receive that buffer, duplicate it and then use
just the duplicate, not needing any reference to the original buffer,
then your code is correct, it just looked strange from a quick look, so
nevermind, I'll continue processing this one and the others.

Thanks,

- Arnaldo
 
> > 
> >> +  if (!buffer)
> >> +  return -ENOMEM;
> >>  
> >>if (session->one_mmap) {
> >>buffer->data = buffer->data_offset - session->one_mmap_offset +
> >> @@ -316,24 +320,28 @@ static int auxtrace_queues__add_buffer(struct 
> >> auxtrace_queues *queues,
> >>} else if (perf_data__is_pipe(session->data)) {
> >>buffer->data = auxtrace_copy_data(buffer->size, session);
> >>if (!buffer->data)
> >> -  return -ENOMEM;
> >> +  goto out_free;
> >>buffer->data_needs_freeing = true;
> >>} else if (BITS_PER_LONG == 32 &&
> >>   buffer->size > BUFFER_LIMIT_FOR_32_BIT) {
> >>err = auxtrace_queues__split_buffer(queues, idx, buffer);
> >>if (err)
> >> -  return err;
> >> +  goto out_free;
> >>}
> >>  
> >>err = auxtrace_queues__queue_buffer(queues, idx, buffer);
> >>if (err)
> >> -  return err;
> >> +  goto out_free;
> >>  
> >>/* FIXME: Doesn't work for split buffer */
> >>if (buffer_ptr)
> >>*buffer_ptr = buffer;
> >>  
> >>return 0;
> >> +
> >> +out_free:
> >> +  auxtrace_buffer__free(buffer);
> >> +  return err;
> >>  }
> >>  
> >>  static bool filter_cpu(struct perf_session *session, int cpu)
> >> @@ -348,36 +356,22 @@ int auxtrace_queues__add_event(struct 
> >> auxtrace_queues *queues,
> >>   union perf_event *event, off_t data_offset,
> >>   struct auxtrace_buffer **buffer_ptr)
> >>  {
> >> -  struct auxtrace_buffer *buffer;
> >> -  unsigned int idx;
> >> -  int err;
> >> +  struct auxtrace_buffer buffer = {
> >> +  .pid = -1,
> >> +  .tid = event->auxtrace.tid,
> >> +  .cpu = event->auxtrace.cpu,
> >> +  .data_offset = data_offset,
> >> +  .offset = event->auxtrace.offset,
> >> +  .reference = event->auxtrace.reference,
> >> +  .size = event->auxtrace.size,
> >> +  };
> >> +  unsigned int idx = event->auxtrace.idx;
> >>  
> >>if (filter_cpu(session, event->auxtrace.cpu))
> >>return 0;
> >>  
> >> -  buffer = zalloc(sizeof(struct auxtrace_buffer));
> >> -  if (!buffer)
> >> -  return -ENOMEM;
> >> -
> >> -  buffer->pid = -1;
> >> -  buffer->tid = event->auxtrace.tid;
> >> -  buffer->cpu = event->auxtrace.cpu;
> >> -  buffer->data_offset = data_offset;
> >> -  buffer->offset = event->auxtrace.offset;
> >> -  buffer->reference = event->auxtrace.reference;
> >> -  buffer->size = event->auxtrace.size;
> >> -  idx = event->auxtrace.idx;
> >> -
> >> -  err = auxtrace_queues__add_buffer(queues, session, idx, buffer,
> >> -buffer_ptr);
> >> -  if (err)
> >> -  goto out_err;
> >> -
> >> -  return 0;
> >> -
> >> -out_err:
> >> -  auxtrace_buffer__free(buffer);
> >> -  return err;
> >> +  return auxtrace_queues__add_buffer(queues, session, idx, ,
> >> + buffer_ptr);
> >>  }
> >>  
> >>  static int auxtrace_queues__add_indexed_event(struct auxtrace_queues 
> >> *queues,
> >> -- 
> >> 1.9.1
> > 

Re: [PATCH 6/7] perf auxtrace: Make auxtrace_queues__add_buffer() allocate struct buffer

[Arnaldo Carvalho de Melo]

Em Wed, Mar 07, 2018 at 10:06:50AM +0200, Adrian Hunter escreveu:
> On 06/03/18 22:25, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Mar 06, 2018 at 11:13:17AM +0200, Adrian Hunter escreveu:
> >> In preparation for supporting AUX area sampling buffers,
> >> auxtrace_queues__add_buffer() needs to be more generic. To that end, move
> >> memory allocation for struct buffer into it.
> >>
> >> Signed-off-by: Adrian Hunter 
> >> ---
> >>  tools/perf/util/auxtrace.c | 54 
> >> +-
> >>  1 file changed, 24 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> >> index fb357a00dd86..e1aff91c54a8 100644
> >> --- a/tools/perf/util/auxtrace.c
> >> +++ b/tools/perf/util/auxtrace.c
> >> @@ -308,7 +308,11 @@ static int auxtrace_queues__add_buffer(struct 
> >> auxtrace_queues *queues,
> >>   struct auxtrace_buffer *buffer,
> >>   struct auxtrace_buffer **buffer_ptr)
> >>  {
> >> -  int err;
> >> +  int err = -ENOMEM;
> >> +
> >> +  buffer = memdup(buffer, sizeof(*buffer));
> > 
> > this is a bit strange, why not make buffer a local variable in this
> > function then?
> 
> Do you mean the following?
> 
>   struct auxtrace_buffer *new_buf;
> 
>   new_buf = memdup(buffer, sizeof(*buffer));

I hadn't noticed that you were using buffer as both r and l value :-\

If all you want is to receive that buffer, duplicate it and then use
just the duplicate, not needing any reference to the original buffer,
then your code is correct, it just looked strange from a quick look, so
nevermind, I'll continue processing this one and the others.

Thanks,

- Arnaldo
 
> > 
> >> +  if (!buffer)
> >> +  return -ENOMEM;
> >>  
> >>if (session->one_mmap) {
> >>buffer->data = buffer->data_offset - session->one_mmap_offset +
> >> @@ -316,24 +320,28 @@ static int auxtrace_queues__add_buffer(struct 
> >> auxtrace_queues *queues,
> >>} else if (perf_data__is_pipe(session->data)) {
> >>buffer->data = auxtrace_copy_data(buffer->size, session);
> >>if (!buffer->data)
> >> -  return -ENOMEM;
> >> +  goto out_free;
> >>buffer->data_needs_freeing = true;
> >>} else if (BITS_PER_LONG == 32 &&
> >>   buffer->size > BUFFER_LIMIT_FOR_32_BIT) {
> >>err = auxtrace_queues__split_buffer(queues, idx, buffer);
> >>if (err)
> >> -  return err;
> >> +  goto out_free;
> >>}
> >>  
> >>err = auxtrace_queues__queue_buffer(queues, idx, buffer);
> >>if (err)
> >> -  return err;
> >> +  goto out_free;
> >>  
> >>/* FIXME: Doesn't work for split buffer */
> >>if (buffer_ptr)
> >>*buffer_ptr = buffer;
> >>  
> >>return 0;
> >> +
> >> +out_free:
> >> +  auxtrace_buffer__free(buffer);
> >> +  return err;
> >>  }
> >>  
> >>  static bool filter_cpu(struct perf_session *session, int cpu)
> >> @@ -348,36 +356,22 @@ int auxtrace_queues__add_event(struct 
> >> auxtrace_queues *queues,
> >>   union perf_event *event, off_t data_offset,
> >>   struct auxtrace_buffer **buffer_ptr)
> >>  {
> >> -  struct auxtrace_buffer *buffer;
> >> -  unsigned int idx;
> >> -  int err;
> >> +  struct auxtrace_buffer buffer = {
> >> +  .pid = -1,
> >> +  .tid = event->auxtrace.tid,
> >> +  .cpu = event->auxtrace.cpu,
> >> +  .data_offset = data_offset,
> >> +  .offset = event->auxtrace.offset,
> >> +  .reference = event->auxtrace.reference,
> >> +  .size = event->auxtrace.size,
> >> +  };
> >> +  unsigned int idx = event->auxtrace.idx;
> >>  
> >>if (filter_cpu(session, event->auxtrace.cpu))
> >>return 0;
> >>  
> >> -  buffer = zalloc(sizeof(struct auxtrace_buffer));
> >> -  if (!buffer)
> >> -  return -ENOMEM;
> >> -
> >> -  buffer->pid = -1;
> >> -  buffer->tid = event->auxtrace.tid;
> >> -  buffer->cpu = event->auxtrace.cpu;
> >> -  buffer->data_offset = data_offset;
> >> -  buffer->offset = event->auxtrace.offset;
> >> -  buffer->reference = event->auxtrace.reference;
> >> -  buffer->size = event->auxtrace.size;
> >> -  idx = event->auxtrace.idx;
> >> -
> >> -  err = auxtrace_queues__add_buffer(queues, session, idx, buffer,
> >> -buffer_ptr);
> >> -  if (err)
> >> -  goto out_err;
> >> -
> >> -  return 0;
> >> -
> >> -out_err:
> >> -  auxtrace_buffer__free(buffer);
> >> -  return err;
> >> +  return auxtrace_queues__add_buffer(queues, session, idx, ,
> >> + buffer_ptr);
> >>  }
> >>  
> >>  static int auxtrace_queues__add_indexed_event(struct auxtrace_queues 
> >> *queues,
> >> -- 
> >> 1.9.1
> > 

Re: [PATCH 6/7] perf auxtrace: Make auxtrace_queues__add_buffer() allocate struct buffer

[Adrian Hunter]

On 06/03/18 22:25, Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 06, 2018 at 11:13:17AM +0200, Adrian Hunter escreveu:
>> In preparation for supporting AUX area sampling buffers,
>> auxtrace_queues__add_buffer() needs to be more generic. To that end, move
>> memory allocation for struct buffer into it.
>>
>> Signed-off-by: Adrian Hunter 
>> ---
>>  tools/perf/util/auxtrace.c | 54 
>> +-
>>  1 file changed, 24 insertions(+), 30 deletions(-)
>>
>> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
>> index fb357a00dd86..e1aff91c54a8 100644
>> --- a/tools/perf/util/auxtrace.c
>> +++ b/tools/perf/util/auxtrace.c
>> @@ -308,7 +308,11 @@ static int auxtrace_queues__add_buffer(struct 
>> auxtrace_queues *queues,
>> struct auxtrace_buffer *buffer,
>> struct auxtrace_buffer **buffer_ptr)
>>  {
>> -int err;
>> +int err = -ENOMEM;
>> +
>> +buffer = memdup(buffer, sizeof(*buffer));
> 
> this is a bit strange, why not make buffer a local variable in this
> function then?

Do you mean the following?

struct auxtrace_buffer *new_buf;

new_buf = memdup(buffer, sizeof(*buffer));

> 
>> +if (!buffer)
>> +return -ENOMEM;
>>  
>>  if (session->one_mmap) {
>>  buffer->data = buffer->data_offset - session->one_mmap_offset +
>> @@ -316,24 +320,28 @@ static int auxtrace_queues__add_buffer(struct 
>> auxtrace_queues *queues,
>>  } else if (perf_data__is_pipe(session->data)) {
>>  buffer->data = auxtrace_copy_data(buffer->size, session);
>>  if (!buffer->data)
>> -return -ENOMEM;
>> +goto out_free;
>>  buffer->data_needs_freeing = true;
>>  } else if (BITS_PER_LONG == 32 &&
>> buffer->size > BUFFER_LIMIT_FOR_32_BIT) {
>>  err = auxtrace_queues__split_buffer(queues, idx, buffer);
>>  if (err)
>> -return err;
>> +goto out_free;
>>  }
>>  
>>  err = auxtrace_queues__queue_buffer(queues, idx, buffer);
>>  if (err)
>> -return err;
>> +goto out_free;
>>  
>>  /* FIXME: Doesn't work for split buffer */
>>  if (buffer_ptr)
>>  *buffer_ptr = buffer;
>>  
>>  return 0;
>> +
>> +out_free:
>> +auxtrace_buffer__free(buffer);
>> +return err;
>>  }
>>  
>>  static bool filter_cpu(struct perf_session *session, int cpu)
>> @@ -348,36 +356,22 @@ int auxtrace_queues__add_event(struct auxtrace_queues 
>> *queues,
>> union perf_event *event, off_t data_offset,
>> struct auxtrace_buffer **buffer_ptr)
>>  {
>> -struct auxtrace_buffer *buffer;
>> -unsigned int idx;
>> -int err;
>> +struct auxtrace_buffer buffer = {
>> +.pid = -1,
>> +.tid = event->auxtrace.tid,
>> +.cpu = event->auxtrace.cpu,
>> +.data_offset = data_offset,
>> +.offset = event->auxtrace.offset,
>> +.reference = event->auxtrace.reference,
>> +.size = event->auxtrace.size,
>> +};
>> +unsigned int idx = event->auxtrace.idx;
>>  
>>  if (filter_cpu(session, event->auxtrace.cpu))
>>  return 0;
>>  
>> -buffer = zalloc(sizeof(struct auxtrace_buffer));
>> -if (!buffer)
>> -return -ENOMEM;
>> -
>> -buffer->pid = -1;
>> -buffer->tid = event->auxtrace.tid;
>> -buffer->cpu = event->auxtrace.cpu;
>> -buffer->data_offset = data_offset;
>> -buffer->offset = event->auxtrace.offset;
>> -buffer->reference = event->auxtrace.reference;
>> -buffer->size = event->auxtrace.size;
>> -idx = event->auxtrace.idx;
>> -
>> -err = auxtrace_queues__add_buffer(queues, session, idx, buffer,
>> -  buffer_ptr);
>> -if (err)
>> -goto out_err;
>> -
>> -return 0;
>> -
>> -out_err:
>> -auxtrace_buffer__free(buffer);
>> -return err;
>> +return auxtrace_queues__add_buffer(queues, session, idx, ,
>> +   buffer_ptr);
>>  }
>>  
>>  static int auxtrace_queues__add_indexed_event(struct auxtrace_queues 
>> *queues,
>> -- 
>> 1.9.1
> 

Re: [PATCH 6/7] perf auxtrace: Make auxtrace_queues__add_buffer() allocate struct buffer

[Adrian Hunter]

On 06/03/18 22:25, Arnaldo Carvalho de Melo wrote:
> Em Tue, Mar 06, 2018 at 11:13:17AM +0200, Adrian Hunter escreveu:
>> In preparation for supporting AUX area sampling buffers,
>> auxtrace_queues__add_buffer() needs to be more generic. To that end, move
>> memory allocation for struct buffer into it.
>>
>> Signed-off-by: Adrian Hunter 
>> ---
>>  tools/perf/util/auxtrace.c | 54 
>> +-
>>  1 file changed, 24 insertions(+), 30 deletions(-)
>>
>> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
>> index fb357a00dd86..e1aff91c54a8 100644
>> --- a/tools/perf/util/auxtrace.c
>> +++ b/tools/perf/util/auxtrace.c
>> @@ -308,7 +308,11 @@ static int auxtrace_queues__add_buffer(struct 
>> auxtrace_queues *queues,
>> struct auxtrace_buffer *buffer,
>> struct auxtrace_buffer **buffer_ptr)
>>  {
>> -int err;
>> +int err = -ENOMEM;
>> +
>> +buffer = memdup(buffer, sizeof(*buffer));
> 
> this is a bit strange, why not make buffer a local variable in this
> function then?

Do you mean the following?

struct auxtrace_buffer *new_buf;

new_buf = memdup(buffer, sizeof(*buffer));

> 
>> +if (!buffer)
>> +return -ENOMEM;
>>  
>>  if (session->one_mmap) {
>>  buffer->data = buffer->data_offset - session->one_mmap_offset +
>> @@ -316,24 +320,28 @@ static int auxtrace_queues__add_buffer(struct 
>> auxtrace_queues *queues,
>>  } else if (perf_data__is_pipe(session->data)) {
>>  buffer->data = auxtrace_copy_data(buffer->size, session);
>>  if (!buffer->data)
>> -return -ENOMEM;
>> +goto out_free;
>>  buffer->data_needs_freeing = true;
>>  } else if (BITS_PER_LONG == 32 &&
>> buffer->size > BUFFER_LIMIT_FOR_32_BIT) {
>>  err = auxtrace_queues__split_buffer(queues, idx, buffer);
>>  if (err)
>> -return err;
>> +goto out_free;
>>  }
>>  
>>  err = auxtrace_queues__queue_buffer(queues, idx, buffer);
>>  if (err)
>> -return err;
>> +goto out_free;
>>  
>>  /* FIXME: Doesn't work for split buffer */
>>  if (buffer_ptr)
>>  *buffer_ptr = buffer;
>>  
>>  return 0;
>> +
>> +out_free:
>> +auxtrace_buffer__free(buffer);
>> +return err;
>>  }
>>  
>>  static bool filter_cpu(struct perf_session *session, int cpu)
>> @@ -348,36 +356,22 @@ int auxtrace_queues__add_event(struct auxtrace_queues 
>> *queues,
>> union perf_event *event, off_t data_offset,
>> struct auxtrace_buffer **buffer_ptr)
>>  {
>> -struct auxtrace_buffer *buffer;
>> -unsigned int idx;
>> -int err;
>> +struct auxtrace_buffer buffer = {
>> +.pid = -1,
>> +.tid = event->auxtrace.tid,
>> +.cpu = event->auxtrace.cpu,
>> +.data_offset = data_offset,
>> +.offset = event->auxtrace.offset,
>> +.reference = event->auxtrace.reference,
>> +.size = event->auxtrace.size,
>> +};
>> +unsigned int idx = event->auxtrace.idx;
>>  
>>  if (filter_cpu(session, event->auxtrace.cpu))
>>  return 0;
>>  
>> -buffer = zalloc(sizeof(struct auxtrace_buffer));
>> -if (!buffer)
>> -return -ENOMEM;
>> -
>> -buffer->pid = -1;
>> -buffer->tid = event->auxtrace.tid;
>> -buffer->cpu = event->auxtrace.cpu;
>> -buffer->data_offset = data_offset;
>> -buffer->offset = event->auxtrace.offset;
>> -buffer->reference = event->auxtrace.reference;
>> -buffer->size = event->auxtrace.size;
>> -idx = event->auxtrace.idx;
>> -
>> -err = auxtrace_queues__add_buffer(queues, session, idx, buffer,
>> -  buffer_ptr);
>> -if (err)
>> -goto out_err;
>> -
>> -return 0;
>> -
>> -out_err:
>> -auxtrace_buffer__free(buffer);
>> -return err;
>> +return auxtrace_queues__add_buffer(queues, session, idx, ,
>> +   buffer_ptr);
>>  }
>>  
>>  static int auxtrace_queues__add_indexed_event(struct auxtrace_queues 
>> *queues,
>> -- 
>> 1.9.1
> 

Re: [PATCH 6/7] perf auxtrace: Make auxtrace_queues__add_buffer() allocate struct buffer

[Arnaldo Carvalho de Melo]

Em Tue, Mar 06, 2018 at 11:13:17AM +0200, Adrian Hunter escreveu:
> In preparation for supporting AUX area sampling buffers,
> auxtrace_queues__add_buffer() needs to be more generic. To that end, move
> memory allocation for struct buffer into it.
> 
> Signed-off-by: Adrian Hunter 
> ---
>  tools/perf/util/auxtrace.c | 54 
> +-
>  1 file changed, 24 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index fb357a00dd86..e1aff91c54a8 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -308,7 +308,11 @@ static int auxtrace_queues__add_buffer(struct 
> auxtrace_queues *queues,
>  struct auxtrace_buffer *buffer,
>  struct auxtrace_buffer **buffer_ptr)
>  {
> - int err;
> + int err = -ENOMEM;
> +
> + buffer = memdup(buffer, sizeof(*buffer));

this is a bit strange, why not make buffer a local variable in this
function then?

> + if (!buffer)
> + return -ENOMEM;
>  
>   if (session->one_mmap) {
>   buffer->data = buffer->data_offset - session->one_mmap_offset +
> @@ -316,24 +320,28 @@ static int auxtrace_queues__add_buffer(struct 
> auxtrace_queues *queues,
>   } else if (perf_data__is_pipe(session->data)) {
>   buffer->data = auxtrace_copy_data(buffer->size, session);
>   if (!buffer->data)
> - return -ENOMEM;
> + goto out_free;
>   buffer->data_needs_freeing = true;
>   } else if (BITS_PER_LONG == 32 &&
>  buffer->size > BUFFER_LIMIT_FOR_32_BIT) {
>   err = auxtrace_queues__split_buffer(queues, idx, buffer);
>   if (err)
> - return err;
> + goto out_free;
>   }
>  
>   err = auxtrace_queues__queue_buffer(queues, idx, buffer);
>   if (err)
> - return err;
> + goto out_free;
>  
>   /* FIXME: Doesn't work for split buffer */
>   if (buffer_ptr)
>   *buffer_ptr = buffer;
>  
>   return 0;
> +
> +out_free:
> + auxtrace_buffer__free(buffer);
> + return err;
>  }
>  
>  static bool filter_cpu(struct perf_session *session, int cpu)
> @@ -348,36 +356,22 @@ int auxtrace_queues__add_event(struct auxtrace_queues 
> *queues,
>  union perf_event *event, off_t data_offset,
>  struct auxtrace_buffer **buffer_ptr)
>  {
> - struct auxtrace_buffer *buffer;
> - unsigned int idx;
> - int err;
> + struct auxtrace_buffer buffer = {
> + .pid = -1,
> + .tid = event->auxtrace.tid,
> + .cpu = event->auxtrace.cpu,
> + .data_offset = data_offset,
> + .offset = event->auxtrace.offset,
> + .reference = event->auxtrace.reference,
> + .size = event->auxtrace.size,
> + };
> + unsigned int idx = event->auxtrace.idx;
>  
>   if (filter_cpu(session, event->auxtrace.cpu))
>   return 0;
>  
> - buffer = zalloc(sizeof(struct auxtrace_buffer));
> - if (!buffer)
> - return -ENOMEM;
> -
> - buffer->pid = -1;
> - buffer->tid = event->auxtrace.tid;
> - buffer->cpu = event->auxtrace.cpu;
> - buffer->data_offset = data_offset;
> - buffer->offset = event->auxtrace.offset;
> - buffer->reference = event->auxtrace.reference;
> - buffer->size = event->auxtrace.size;
> - idx = event->auxtrace.idx;
> -
> - err = auxtrace_queues__add_buffer(queues, session, idx, buffer,
> -   buffer_ptr);
> - if (err)
> - goto out_err;
> -
> - return 0;
> -
> -out_err:
> - auxtrace_buffer__free(buffer);
> - return err;
> + return auxtrace_queues__add_buffer(queues, session, idx, ,
> +buffer_ptr);
>  }
>  
>  static int auxtrace_queues__add_indexed_event(struct auxtrace_queues *queues,
> -- 
> 1.9.1

Re: [PATCH 6/7] perf auxtrace: Make auxtrace_queues__add_buffer() allocate struct buffer

[Arnaldo Carvalho de Melo]

Em Tue, Mar 06, 2018 at 11:13:17AM +0200, Adrian Hunter escreveu:
> In preparation for supporting AUX area sampling buffers,
> auxtrace_queues__add_buffer() needs to be more generic. To that end, move
> memory allocation for struct buffer into it.
> 
> Signed-off-by: Adrian Hunter 
> ---
>  tools/perf/util/auxtrace.c | 54 
> +-
>  1 file changed, 24 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/perf/util/auxtrace.c b/tools/perf/util/auxtrace.c
> index fb357a00dd86..e1aff91c54a8 100644
> --- a/tools/perf/util/auxtrace.c
> +++ b/tools/perf/util/auxtrace.c
> @@ -308,7 +308,11 @@ static int auxtrace_queues__add_buffer(struct 
> auxtrace_queues *queues,
>  struct auxtrace_buffer *buffer,
>  struct auxtrace_buffer **buffer_ptr)
>  {
> - int err;
> + int err = -ENOMEM;
> +
> + buffer = memdup(buffer, sizeof(*buffer));

this is a bit strange, why not make buffer a local variable in this
function then?

> + if (!buffer)
> + return -ENOMEM;
>  
>   if (session->one_mmap) {
>   buffer->data = buffer->data_offset - session->one_mmap_offset +
> @@ -316,24 +320,28 @@ static int auxtrace_queues__add_buffer(struct 
> auxtrace_queues *queues,
>   } else if (perf_data__is_pipe(session->data)) {
>   buffer->data = auxtrace_copy_data(buffer->size, session);
>   if (!buffer->data)
> - return -ENOMEM;
> + goto out_free;
>   buffer->data_needs_freeing = true;
>   } else if (BITS_PER_LONG == 32 &&
>  buffer->size > BUFFER_LIMIT_FOR_32_BIT) {
>   err = auxtrace_queues__split_buffer(queues, idx, buffer);
>   if (err)
> - return err;
> + goto out_free;
>   }
>  
>   err = auxtrace_queues__queue_buffer(queues, idx, buffer);
>   if (err)
> - return err;
> + goto out_free;
>  
>   /* FIXME: Doesn't work for split buffer */
>   if (buffer_ptr)
>   *buffer_ptr = buffer;
>  
>   return 0;
> +
> +out_free:
> + auxtrace_buffer__free(buffer);
> + return err;
>  }
>  
>  static bool filter_cpu(struct perf_session *session, int cpu)
> @@ -348,36 +356,22 @@ int auxtrace_queues__add_event(struct auxtrace_queues 
> *queues,
>  union perf_event *event, off_t data_offset,
>  struct auxtrace_buffer **buffer_ptr)
>  {
> - struct auxtrace_buffer *buffer;
> - unsigned int idx;
> - int err;
> + struct auxtrace_buffer buffer = {
> + .pid = -1,
> + .tid = event->auxtrace.tid,
> + .cpu = event->auxtrace.cpu,
> + .data_offset = data_offset,
> + .offset = event->auxtrace.offset,
> + .reference = event->auxtrace.reference,
> + .size = event->auxtrace.size,
> + };
> + unsigned int idx = event->auxtrace.idx;
>  
>   if (filter_cpu(session, event->auxtrace.cpu))
>   return 0;
>  
> - buffer = zalloc(sizeof(struct auxtrace_buffer));
> - if (!buffer)
> - return -ENOMEM;
> -
> - buffer->pid = -1;
> - buffer->tid = event->auxtrace.tid;
> - buffer->cpu = event->auxtrace.cpu;
> - buffer->data_offset = data_offset;
> - buffer->offset = event->auxtrace.offset;
> - buffer->reference = event->auxtrace.reference;
> - buffer->size = event->auxtrace.size;
> - idx = event->auxtrace.idx;
> -
> - err = auxtrace_queues__add_buffer(queues, session, idx, buffer,
> -   buffer_ptr);
> - if (err)
> - goto out_err;
> -
> - return 0;
> -
> -out_err:
> - auxtrace_buffer__free(buffer);
> - return err;
> + return auxtrace_queues__add_buffer(queues, session, idx, ,
> +buffer_ptr);
>  }
>  
>  static int auxtrace_queues__add_indexed_event(struct auxtrace_queues *queues,
> -- 
> 1.9.1