perf tools: Add struct ordered_events_buffer layer
Jiri Olsa
reviewerPATCH 1/2 perf tools: Add struct ordered_events_buffer layer
When ordering events, we use preallocated buffers to store separated events. Those buffers currently don't have their own struct, but since they are basically array of 'struct ordered_event' objects, we use the first event to hold buffers data - list head, that holds all buffers together:
struct ordered_events {
...
struct ordered_event *buffer;
...
};
struct ordered_event {
u64 timestamp;
u64 file_offset;
union perf_event *event;
struct list_head list;
};This is quite convoluted and error prone as demonstrated by free-ing issue discovered and fixed by Stephane in here [1].
This patch adds the 'struct ordered_events_buffer' object, that holds the buffer data and frees it up properly.
[1] - https://marc.info/?l=linux-kernel&m=153376761329335&w=2
--- tools/perf/util/ordered-events.c | 83 +++++++++++++++++++++++++++----- tools/perf/util/ordered-events.h | 37 ++++++++------ 2 files changed, 91 insertions(+), 29 deletions(-)
tools/perf/util/ordered-events.c +70 -13
@@ -80,14 +80,20 @@ static union perf_event *dup_event(struct ordered_events *oe,return oe->copy_on_queue ?__dup_event (oe, event) : event; } -static void free_dup_event (struct ordered_events *oe,union perf_event *event) +static void __free_dup_event (struct ordered_events *oe,union perf_event *event) { -if (event && oe->copy_on_queue) { +if (event) { oe->cur_alloc_size -= event->header.size;free (event); } } +static void free_dup_event (struct ordered_events *oe,union perf_event *event) +{ +if (oe->copy_on_queue) +__free_dup_event (oe, event); +} +#define MAX_SAMPLE_BUFFER (64 * 1024 / sizeof(struct ordered_event))static struct ordered_event *alloc_event (struct ordered_events *oe,union perf_event *event)
@@ -100,15 +106,43 @@ static struct ordered_event *alloc_event(struct ordered_events *oe,if (!new_event)return NULL ; +/* +* We maintain following scheme of buffers for ordered +* event allocation: +* +* to_free list -> buffer1 (64K) +* buffer2 (64K) +* ... +* +* Each buffer keeps an array of ordered events objects: +* buffer -> event[0] +* event[1] +* ... +* +* Each allocated ordered event is linked to one of +* following lists: +* - time ordered list 'events' +* - list of currently removed events 'cache' +* +* Allocation of the ordered event uses following order +* to get the memory: +* - use recently removed object from 'cache' list +* - use available object in current allocation buffer +* - allocate new buffer if the current buffer is full +* +* Removal of ordered event object moves it from events to +* the cache list. +*/ if (!list_empty (cache)) { new = list_entry(cache->next, struct ordered_event, list);list_del (&new->list); }else if (oe->buffer) { - new = oe->buffer + oe->buffer_idx; + new = &oe->buffer->event[oe->buffer_idx];if (++oe->buffer_idx ==MAX_SAMPLE_BUFFER ) oe->buffer =NULL ; }else if (oe->cur_alloc_size < oe->max_alloc_size) { -size_t size =MAX_SAMPLE_BUFFER *sizeof (*new); +size_t size =sizeof (*oe->buffer) + +MAX_SAMPLE_BUFFER *sizeof (*new); oe->buffer =malloc (size);if (!oe->buffer) {
@@ -122,11 +156,11 @@ static struct ordered_event *alloc_event(struct ordered_events *oe, oe->cur_alloc_size += size;list_add (&oe->buffer->list, &oe->to_free); -/* First entry is abused to maintain the to_free list. */ - oe->buffer_idx =2 ; - new = oe->buffer +1 ; + oe->buffer_idx =1 ; + new = &oe->buffer->event[0 ]; }else {pr ("allocation limit reached %" PRIu64"B\n" , oe->max_alloc_size); +return NULL ; } new->event = new_event;
@@ -300,15 +334,38 @@ void ordered_events__init(struct ordered_events *oe, ordered_events__deliver_t d oe->deliver = deliver; } +static void +ordered_events_buffer__free (struct ordered_events_buffer *buffer, +unsigned int max,struct ordered_events *oe) +{ +if (oe->copy_on_queue) { +unsigned int i; + +for (i =0 ; i < max; i++) +__free_dup_event (oe, buffer->event[i].event); + } + +free (buffer); +} +void ordered_events__free (struct ordered_events *oe) { -while (!list_empty (&oe->to_free)) { -struct ordered_event *event; +struct ordered_events_buffer *buffer, *tmp; - event = list_entry(oe->to_free.next, struct ordered_event, list); -list_del (&event->list); -free_dup_event (oe, event->event); -free (event); +if (list_empty (&oe->to_free)) +return ; + +/* +* Current buffer might not have all the events allocated +* yet, we need to free only allocated ones ... +*/ +list_del (&oe->buffer->list); +ordered_events_buffer__free (oe->buffer, oe->buffer_idx, oe); + +/* ... and continue with the rest */ +list_for_each_entry_safe (buffer, tmp, &oe->to_free, list) { +list_del (&buffer->list); +ordered_events_buffer__free (buffer,MAX_SAMPLE_BUFFER , oe); } }
tools/perf/util/ordered-events.h +21 -16
@@ -25,23 +25,28 @@ struct ordered_events;typedef int (*ordered_events__deliver_t )(struct ordered_events *oe,struct ordered_event *event); +struct ordered_events_buffer { +struct list_head list; +struct ordered_event event[0 ]; +}; +struct ordered_events { -u64 last_flush; -u64 next_flush; -u64 max_timestamp; -u64 max_alloc_size; -u64 cur_alloc_size; -struct list_head events; -struct list_head cache; -struct list_head to_free; -struct ordered_event *buffer; -struct ordered_event *last; -ordered_events__deliver_t deliver; -int buffer_idx; -unsigned int nr_events; -enum oe_flush last_flush_type; -u32 nr_unordered_events; -bool copy_on_queue; +u64 last_flush; +u64 next_flush; +u64 max_timestamp; +u64 max_alloc_size; +u64 cur_alloc_size; +struct list_head events; +struct list_head cache; +struct list_head to_free; +struct ordered_events_buffer *buffer; +struct ordered_event *last; +ordered_events__deliver_t deliver; +int buffer_idx; +unsigned int nr_events; +enum oe_flush last_flush_type; +u32 nr_unordered_events; +bool copy_on_queue; }; int ordered_events__queue(struct ordered_events *oe, union perf_event *event,
Signature · 1 line
2.17.1
Jiri Olsa
reviewerPATCH 2/2 perf tools: Prevent crossing ordered events max_alloc_size
Stephane reported possible issue in the ordered events code, which could lead to allocating more memory than guarded by max_alloc_size.
He also suggested the fix to properly check that the new size is below the max_alloc_size limit.
--- tools/perf/util/ordered-events.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
tools/perf/util/ordered-events.c +4 -4
@@ -101,6 +101,7 @@ static struct ordered_event *alloc_event(struct ordered_events *oe,struct list_head *cache = &oe->cache;struct ordered_event *new =NULL ;union perf_event *new_event; +size_t size; new_event =dup_event (oe, event);if (!new_event)
@@ -133,6 +134,8 @@ static struct ordered_event *alloc_event(struct ordered_events *oe,* Removal of ordered event object moves it from events to * the cache list. */ + size =sizeof (*oe->buffer) +MAX_SAMPLE_BUFFER *sizeof (*new); +if (!list_empty (cache)) { new = list_entry(cache->next, struct ordered_event, list);list_del (&new->list);
@@ -140,10 +143,7 @@ static struct ordered_event *alloc_event(struct ordered_events *oe, new = &oe->buffer->event[oe->buffer_idx];if (++oe->buffer_idx ==MAX_SAMPLE_BUFFER ) oe->buffer =NULL ; - }else if (oe->cur_alloc_size < oe->max_alloc_size) { -size_t size =sizeof (*oe->buffer) + -MAX_SAMPLE_BUFFER *sizeof (*new); - + }else if ((oe->cur_alloc_size + size) < oe->max_alloc_size) { oe->buffer =malloc (size);if (!oe->buffer) {free_dup_event (oe, new_event);
Signature · 1 line
2.17.1
tip-bot for Jiri Olsa
merge bot[tip:perf/core] perf ordered_events: Prevent crossing max_alloc_size
perf ordered_events: Prevent crossing max_alloc_size
Commit-ID: 53da12e013498c4eca592939bb18a5dbd1d228c9 Gitweb: https://git.kernel.org/tip/53da12e013498c4eca592939bb18a5dbd1d228c9 Author: Jiri Olsa <[email protected]> AuthorDate: Fri, 7 Sep 2018 12:24:55 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Wed, 19 Sep 2018 10:25:08 -0300
perf ordered_events: Prevent crossing max_alloc_size
Stephane reported a possible issue in the ordered events code, which could lead to allocating more memory than guarded by max_alloc_size.
He also suggested the fix to properly check that the new size is below the max_alloc_size limit.
--- tools/perf/util/ordered-events.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
tools/perf/util/ordered-events.c +4 -4
@@ -101,6 +101,7 @@ static struct ordered_event *alloc_event(struct ordered_events *oe,struct list_head *cache = &oe->cache;struct ordered_event *new =NULL ;union perf_event *new_event; +size_t size; new_event =dup_event (oe, event);if (!new_event)
@@ -133,6 +134,8 @@ static struct ordered_event *alloc_event(struct ordered_events *oe,* Removal of ordered event object moves it from events to * the cache list. */ + size =sizeof (*oe->buffer) +MAX_SAMPLE_BUFFER *sizeof (*new); +if (!list_empty (cache)) { new = list_entry(cache->next, struct ordered_event, list);list_del (&new->list);
@@ -140,10 +143,7 @@ static struct ordered_event *alloc_event(struct ordered_events *oe, new = &oe->buffer->event[oe->buffer_idx];if (++oe->buffer_idx ==MAX_SAMPLE_BUFFER ) oe->buffer =NULL ; - }else if (oe->cur_alloc_size < oe->max_alloc_size) { -size_t size =sizeof (*oe->buffer) + -MAX_SAMPLE_BUFFER *sizeof (*new); - + }else if ((oe->cur_alloc_size + size) < oe->max_alloc_size) { oe->buffer =malloc (size);if (!oe->buffer) {free_dup_event (oe, new_event);
tip-bot for Jiri Olsa
merge bot[tip:perf/core] perf ordered_events: Add 'struct ordered_events_buffer' layer
In reply to Jiri Olsaperf ordered_events: Add 'struct ordered_events_buffer' layer
Commit-ID: d5ceb62b36545b597e89adb5eb778c3e15431c10 Gitweb: https://git.kernel.org/tip/d5ceb62b36545b597e89adb5eb778c3e15431c10 Author: Jiri Olsa <[email protected]> AuthorDate: Fri, 7 Sep 2018 12:24:54 +0200 Committer: Arnaldo Carvalho de Melo <[email protected]> CommitDate: Wed, 19 Sep 2018 10:24:57 -0300
perf ordered_events: Add 'struct ordered_events_buffer' layer
When ordering events, we use preallocated buffers to store separate events. Those buffers currently don't have their own struct, but since they are basically an array of 'struct ordered_event' objects, we use the first event to hold buffers data - list head, that holds all buffers together:
struct ordered_events {
...
struct ordered_event *buffer;
...
};
struct ordered_event {
u64 timestamp;
u64 file_offset;
union perf_event *event;
struct list_head list;
};This is quite convoluted and error prone as demonstrated by free-ing issue discovered and fixed by Stephane in here [1].
This patch adds the 'struct ordered_events_buffer' object, that holds the buffer data and frees it up properly.
[1] - https://marc.info/?l=linux-kernel&m=153376761329335&w=2
--- tools/perf/util/ordered-events.c | 83 +++++++++++++++++++++++++++++++++------- tools/perf/util/ordered-events.h | 37 ++++++++++-------- 2 files changed, 91 insertions(+), 29 deletions(-)
tools/perf/util/ordered-events.c +70 -13
@@ -80,14 +80,20 @@ static union perf_event *dup_event(struct ordered_events *oe,return oe->copy_on_queue ?__dup_event (oe, event) : event; } -static void free_dup_event (struct ordered_events *oe,union perf_event *event) +static void __free_dup_event (struct ordered_events *oe,union perf_event *event) { -if (event && oe->copy_on_queue) { +if (event) { oe->cur_alloc_size -= event->header.size;free (event); } } +static void free_dup_event (struct ordered_events *oe,union perf_event *event) +{ +if (oe->copy_on_queue) +__free_dup_event (oe, event); +} +#define MAX_SAMPLE_BUFFER (64 * 1024 / sizeof(struct ordered_event))static struct ordered_event *alloc_event (struct ordered_events *oe,union perf_event *event)
@@ -100,15 +106,43 @@ static struct ordered_event *alloc_event(struct ordered_events *oe,if (!new_event)return NULL ; +/* +* We maintain the following scheme of buffers for ordered +* event allocation: +* +* to_free list -> buffer1 (64K) +* buffer2 (64K) +* ... +* +* Each buffer keeps an array of ordered events objects: +* buffer -> event[0] +* event[1] +* ... +* +* Each allocated ordered event is linked to one of +* following lists: +* - time ordered list 'events' +* - list of currently removed events 'cache' +* +* Allocation of the ordered event uses the following order +* to get the memory: +* - use recently removed object from 'cache' list +* - use available object in current allocation buffer +* - allocate new buffer if the current buffer is full +* +* Removal of ordered event object moves it from events to +* the cache list. +*/ if (!list_empty (cache)) { new = list_entry(cache->next, struct ordered_event, list);list_del (&new->list); }else if (oe->buffer) { - new = oe->buffer + oe->buffer_idx; + new = &oe->buffer->event[oe->buffer_idx];if (++oe->buffer_idx ==MAX_SAMPLE_BUFFER ) oe->buffer =NULL ; }else if (oe->cur_alloc_size < oe->max_alloc_size) { -size_t size =MAX_SAMPLE_BUFFER *sizeof (*new); +size_t size =sizeof (*oe->buffer) + +MAX_SAMPLE_BUFFER *sizeof (*new); oe->buffer =malloc (size);if (!oe->buffer) {
@@ -122,11 +156,11 @@ static struct ordered_event *alloc_event(struct ordered_events *oe, oe->cur_alloc_size += size;list_add (&oe->buffer->list, &oe->to_free); -/* First entry is abused to maintain the to_free list. */ - oe->buffer_idx =2 ; - new = oe->buffer +1 ; + oe->buffer_idx =1 ; + new = &oe->buffer->event[0 ]; }else {pr ("allocation limit reached %" PRIu64"B\n" , oe->max_alloc_size); +return NULL ; } new->event = new_event;
@@ -300,15 +334,38 @@ void ordered_events__init(struct ordered_events *oe, ordered_events__deliver_t d oe->deliver = deliver; } +static void +ordered_events_buffer__free (struct ordered_events_buffer *buffer, +unsigned int max,struct ordered_events *oe) +{ +if (oe->copy_on_queue) { +unsigned int i; + +for (i =0 ; i < max; i++) +__free_dup_event (oe, buffer->event[i].event); + } + +free (buffer); +} +void ordered_events__free (struct ordered_events *oe) { -while (!list_empty (&oe->to_free)) { -struct ordered_event *event; +struct ordered_events_buffer *buffer, *tmp; - event = list_entry(oe->to_free.next, struct ordered_event, list); -list_del (&event->list); -free_dup_event (oe, event->event); -free (event); +if (list_empty (&oe->to_free)) +return ; + +/* +* Current buffer might not have all the events allocated +* yet, we need to free only allocated ones ... +*/ +list_del (&oe->buffer->list); +ordered_events_buffer__free (oe->buffer, oe->buffer_idx, oe); + +/* ... and continue with the rest */ +list_for_each_entry_safe (buffer, tmp, &oe->to_free, list) { +list_del (&buffer->list); +ordered_events_buffer__free (buffer,MAX_SAMPLE_BUFFER , oe); } }
tools/perf/util/ordered-events.h +21 -16
@@ -25,23 +25,28 @@ struct ordered_events;typedef int (*ordered_events__deliver_t )(struct ordered_events *oe,struct ordered_event *event); +struct ordered_events_buffer { +struct list_head list; +struct ordered_event event[0 ]; +}; +struct ordered_events { -u64 last_flush; -u64 next_flush; -u64 max_timestamp; -u64 max_alloc_size; -u64 cur_alloc_size; -struct list_head events; -struct list_head cache; -struct list_head to_free; -struct ordered_event *buffer; -struct ordered_event *last; -ordered_events__deliver_t deliver; -int buffer_idx; -unsigned int nr_events; -enum oe_flush last_flush_type; -u32 nr_unordered_events; -bool copy_on_queue; +u64 last_flush; +u64 next_flush; +u64 max_timestamp; +u64 max_alloc_size; +u64 cur_alloc_size; +struct list_head events; +struct list_head cache; +struct list_head to_free; +struct ordered_events_buffer *buffer; +struct ordered_event *last; +ordered_events__deliver_t deliver; +int buffer_idx; +unsigned int nr_events; +enum oe_flush last_flush_type; +u32 nr_unordered_events; +bool copy_on_queue; }; int ordered_events__queue(struct ordered_events *oe, union perf_event *event,