Re: [PATCHv3] perf tools: Add struct ordered_events_buffer layer
On Thu, Sep 6, 2018 at 6:29 AM Jiri Olsa wrote: > > On Mon, Sep 03, 2018 at 07:37:56PM -0700, Stephane Eranian wrote: > > SNIP > > > > > I think the code is correct now for the issue related to uninitialized > > pointer. > > But there is still one problem I found stressing the code with > > max_alloc_size. > > The way the following is written: > > > >if (!list_empty(cache)) { > > new = list_entry(cache->next, struct ordered_event, list); > > list_del(>list); > > } else if (oe->buffer) { > > new = oe->buffer + 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); > > > > oe->buffer = malloc(size); > > if (!oe->buffer) { > > free_dup_event(oe, new_event); > > return NULL; > > } > > > > pr("alloc size %" PRIu64 "B (+%zu), max %" PRIu64 "B\n", > >oe->cur_alloc_size, size, oe->max_alloc_size); > > > > oe->cur_alloc_size += size; > > > > You can end up with oe->cur_alloc_size > oe->max_alloc_size in case > > the max limit is > > really low (< size_t size = sizeof (*oe->buffer) + MAX_SAMPLE_BUFFER * > > sizeof(*new); > > So I think to make sure you can never allocate more than the max, you > > have to do: > > > > size_t size = sizeof(*oe->buffer) MAX_SAMPLE_BUFFER * sizeof(*new); > >if (!list_empty(cache)) { > > new = list_entry(cache->next, struct ordered_event, list); > > list_del(>list); > > } else if (oe->buffer) { > > new = oe->buffer + oe->buffer_idx; > > if (++oe->buffer_idx == MAX_SAMPLE_BUFFER) > > oe->buffer = NULL; > > } else if ((oe->cur_alloc_size + size) < oe->max_alloc_size) { > > > > Then you will never allocate more than the max. > > I think with this change, we are okay. > > Tested-by: Stephane Eranian > > yep, makes sense.. something like below then > I'll post it on top of the previous patch > Yes. This works. Thanks. > thanks, > jirka > > > --- > diff --git a/tools/perf/util/ordered-events.c > b/tools/perf/util/ordered-events.c > index 87171e8fd70d..2d1d0f3c8f77 100644 > --- a/tools/perf/util/ordered-events.c > +++ b/tools/perf/util/ordered-events.c > @@ -101,6 +101,7 @@ static struct ordered_event *alloc_event(struct > ordered_events *oe, > struct list_head *cache = >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(>list); > @@ -140,10 +143,7 @@ static struct ordered_event *alloc_event(struct > ordered_events *oe, > new = >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);
Re: [PATCHv3] perf tools: Add struct ordered_events_buffer layer
On Thu, Sep 6, 2018 at 6:29 AM Jiri Olsa wrote: > > On Mon, Sep 03, 2018 at 07:37:56PM -0700, Stephane Eranian wrote: > > SNIP > > > > > I think the code is correct now for the issue related to uninitialized > > pointer. > > But there is still one problem I found stressing the code with > > max_alloc_size. > > The way the following is written: > > > >if (!list_empty(cache)) { > > new = list_entry(cache->next, struct ordered_event, list); > > list_del(>list); > > } else if (oe->buffer) { > > new = oe->buffer + 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); > > > > oe->buffer = malloc(size); > > if (!oe->buffer) { > > free_dup_event(oe, new_event); > > return NULL; > > } > > > > pr("alloc size %" PRIu64 "B (+%zu), max %" PRIu64 "B\n", > >oe->cur_alloc_size, size, oe->max_alloc_size); > > > > oe->cur_alloc_size += size; > > > > You can end up with oe->cur_alloc_size > oe->max_alloc_size in case > > the max limit is > > really low (< size_t size = sizeof (*oe->buffer) + MAX_SAMPLE_BUFFER * > > sizeof(*new); > > So I think to make sure you can never allocate more than the max, you > > have to do: > > > > size_t size = sizeof(*oe->buffer) MAX_SAMPLE_BUFFER * sizeof(*new); > >if (!list_empty(cache)) { > > new = list_entry(cache->next, struct ordered_event, list); > > list_del(>list); > > } else if (oe->buffer) { > > new = oe->buffer + oe->buffer_idx; > > if (++oe->buffer_idx == MAX_SAMPLE_BUFFER) > > oe->buffer = NULL; > > } else if ((oe->cur_alloc_size + size) < oe->max_alloc_size) { > > > > Then you will never allocate more than the max. > > I think with this change, we are okay. > > Tested-by: Stephane Eranian > > yep, makes sense.. something like below then > I'll post it on top of the previous patch > Yes. This works. Thanks. > thanks, > jirka > > > --- > diff --git a/tools/perf/util/ordered-events.c > b/tools/perf/util/ordered-events.c > index 87171e8fd70d..2d1d0f3c8f77 100644 > --- a/tools/perf/util/ordered-events.c > +++ b/tools/perf/util/ordered-events.c > @@ -101,6 +101,7 @@ static struct ordered_event *alloc_event(struct > ordered_events *oe, > struct list_head *cache = >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(>list); > @@ -140,10 +143,7 @@ static struct ordered_event *alloc_event(struct > ordered_events *oe, > new = >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);
Re: [PATCHv3] perf tools: Add struct ordered_events_buffer layer
On Mon, Sep 03, 2018 at 07:37:56PM -0700, Stephane Eranian wrote: SNIP > > I think the code is correct now for the issue related to uninitialized > pointer. > But there is still one problem I found stressing the code with max_alloc_size. > The way the following is written: > >if (!list_empty(cache)) { > new = list_entry(cache->next, struct ordered_event, list); > list_del(>list); > } else if (oe->buffer) { > new = oe->buffer + 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); > > oe->buffer = malloc(size); > if (!oe->buffer) { > free_dup_event(oe, new_event); > return NULL; > } > > pr("alloc size %" PRIu64 "B (+%zu), max %" PRIu64 "B\n", >oe->cur_alloc_size, size, oe->max_alloc_size); > > oe->cur_alloc_size += size; > > You can end up with oe->cur_alloc_size > oe->max_alloc_size in case > the max limit is > really low (< size_t size = sizeof (*oe->buffer) + MAX_SAMPLE_BUFFER * > sizeof(*new); > So I think to make sure you can never allocate more than the max, you > have to do: > > size_t size = sizeof(*oe->buffer) MAX_SAMPLE_BUFFER * sizeof(*new); >if (!list_empty(cache)) { > new = list_entry(cache->next, struct ordered_event, list); > list_del(>list); > } else if (oe->buffer) { > new = oe->buffer + oe->buffer_idx; > if (++oe->buffer_idx == MAX_SAMPLE_BUFFER) > oe->buffer = NULL; > } else if ((oe->cur_alloc_size + size) < oe->max_alloc_size) { > > Then you will never allocate more than the max. > I think with this change, we are okay. > Tested-by: Stephane Eranian yep, makes sense.. something like below then I'll post it on top of the previous patch thanks, jirka --- diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c index 87171e8fd70d..2d1d0f3c8f77 100644 --- a/tools/perf/util/ordered-events.c +++ b/tools/perf/util/ordered-events.c @@ -101,6 +101,7 @@ static struct ordered_event *alloc_event(struct ordered_events *oe, struct list_head *cache = >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(>list); @@ -140,10 +143,7 @@ static struct ordered_event *alloc_event(struct ordered_events *oe, new = >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);
Re: [PATCHv3] perf tools: Add struct ordered_events_buffer layer
On Mon, Sep 03, 2018 at 07:37:56PM -0700, Stephane Eranian wrote: SNIP > > I think the code is correct now for the issue related to uninitialized > pointer. > But there is still one problem I found stressing the code with max_alloc_size. > The way the following is written: > >if (!list_empty(cache)) { > new = list_entry(cache->next, struct ordered_event, list); > list_del(>list); > } else if (oe->buffer) { > new = oe->buffer + 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); > > oe->buffer = malloc(size); > if (!oe->buffer) { > free_dup_event(oe, new_event); > return NULL; > } > > pr("alloc size %" PRIu64 "B (+%zu), max %" PRIu64 "B\n", >oe->cur_alloc_size, size, oe->max_alloc_size); > > oe->cur_alloc_size += size; > > You can end up with oe->cur_alloc_size > oe->max_alloc_size in case > the max limit is > really low (< size_t size = sizeof (*oe->buffer) + MAX_SAMPLE_BUFFER * > sizeof(*new); > So I think to make sure you can never allocate more than the max, you > have to do: > > size_t size = sizeof(*oe->buffer) MAX_SAMPLE_BUFFER * sizeof(*new); >if (!list_empty(cache)) { > new = list_entry(cache->next, struct ordered_event, list); > list_del(>list); > } else if (oe->buffer) { > new = oe->buffer + oe->buffer_idx; > if (++oe->buffer_idx == MAX_SAMPLE_BUFFER) > oe->buffer = NULL; > } else if ((oe->cur_alloc_size + size) < oe->max_alloc_size) { > > Then you will never allocate more than the max. > I think with this change, we are okay. > Tested-by: Stephane Eranian yep, makes sense.. something like below then I'll post it on top of the previous patch thanks, jirka --- diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c index 87171e8fd70d..2d1d0f3c8f77 100644 --- a/tools/perf/util/ordered-events.c +++ b/tools/perf/util/ordered-events.c @@ -101,6 +101,7 @@ static struct ordered_event *alloc_event(struct ordered_events *oe, struct list_head *cache = >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(>list); @@ -140,10 +143,7 @@ static struct ordered_event *alloc_event(struct ordered_events *oe, new = >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);
Re: [PATCHv3] perf tools: Add struct ordered_events_buffer layer
Jiri, On Sun, Sep 2, 2018 at 7:47 AM Jiri Olsa wrote: > > On Mon, Aug 27, 2018 at 07:05:43PM +0200, Jiri Olsa wrote: > > On Mon, Aug 27, 2018 at 08:24:56AM -0700, Stephane Eranian wrote: > > > > SNIP > > > > > > - /* First entry is abused to maintain the to_free list. > > > > */ > > > > - oe->buffer_idx = 2; > > > > - new = oe->buffer + 1; > > > > + oe->buffer_idx = 1; > > > > + new = >buffer->event[0]; > > > > } else { > > > > pr("allocation limit reached %" PRIu64 "B\n", > > > > oe->max_alloc_size); > > > > > > > > > I am wondering about the usefulness of returning a new_event with > > > new_event->event = NULL > > > in this case. Don't you need new_event->event? If so, then you need > > > return NULL. > > > > yep, that's a bug.. with new being NULL in here, > > we'd get a crash anyway.. so 'return NULL;' it is > > > > SNIP > > > > > > +* yet, we need to free only allocated ones ... > > > > +*/ > > > > + list_del(>buffer->list); > > > > + ordered_events_buffer__free(oe->buffer, oe->buffer_idx, oe); > > > > + > > > > + /* ... and continue with the rest */ > > > > + list_for_each_entry_safe(buffer, tmp, >to_free, list) { > > > > + list_del(>list); > > > > + ordered_events_buffer__free(buffer, MAX_SAMPLE_BUFFER, > > > > oe); > > > > > > > > > Here you are saying that if it is on the to_free list and not the > > > current buffer, then necessarily > > > all the entries have been used and it is safe to use > > > MAX_SAMPLE_BUFFER. Is that right? > > > > yes, at this point they either holds an event or NULL > > so it's free to call __free_dup_event on it > > > > thanks, v3 attached > > > > added also Namhyung's ack, as the 'return NULL' change wasn't > > related to the v2 changes > > I think the code is correct now for the issue related to uninitialized pointer. But there is still one problem I found stressing the code with max_alloc_size. The way the following is written: if (!list_empty(cache)) { new = list_entry(cache->next, struct ordered_event, list); list_del(>list); } else if (oe->buffer) { new = oe->buffer + 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); oe->buffer = malloc(size); if (!oe->buffer) { free_dup_event(oe, new_event); return NULL; } pr("alloc size %" PRIu64 "B (+%zu), max %" PRIu64 "B\n", oe->cur_alloc_size, size, oe->max_alloc_size); oe->cur_alloc_size += size; You can end up with oe->cur_alloc_size > oe->max_alloc_size in case the max limit is really low (< size_t size = sizeof (*oe->buffer) + MAX_SAMPLE_BUFFER * sizeof(*new); So I think to make sure you can never allocate more than the max, you have to do: size_t size = sizeof(*oe->buffer) MAX_SAMPLE_BUFFER * sizeof(*new); if (!list_empty(cache)) { new = list_entry(cache->next, struct ordered_event, list); list_del(>list); } else if (oe->buffer) { new = oe->buffer + oe->buffer_idx; if (++oe->buffer_idx == MAX_SAMPLE_BUFFER) oe->buffer = NULL; } else if ((oe->cur_alloc_size + size) < oe->max_alloc_size) { Then you will never allocate more than the max. I think with this change, we are okay. Tested-by: Stephane Eranian > jirka > > Stephane, > any comments to v3 version? > > thanks, > jirka > > > > > > > --- > > 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=153376761329335=2 > > > > Reported-by: Stephane Eranian > > Acked-by: Namhyung Kim > > Link: http://lkml.kernel.org/n/tip-qrkcqm5m1sugy4q83pfn5...@git.kernel.org > > Signed-off-by: Jiri Olsa > > --- > >
Re: [PATCHv3] perf tools: Add struct ordered_events_buffer layer
Jiri, On Sun, Sep 2, 2018 at 7:47 AM Jiri Olsa wrote: > > On Mon, Aug 27, 2018 at 07:05:43PM +0200, Jiri Olsa wrote: > > On Mon, Aug 27, 2018 at 08:24:56AM -0700, Stephane Eranian wrote: > > > > SNIP > > > > > > - /* First entry is abused to maintain the to_free list. > > > > */ > > > > - oe->buffer_idx = 2; > > > > - new = oe->buffer + 1; > > > > + oe->buffer_idx = 1; > > > > + new = >buffer->event[0]; > > > > } else { > > > > pr("allocation limit reached %" PRIu64 "B\n", > > > > oe->max_alloc_size); > > > > > > > > > I am wondering about the usefulness of returning a new_event with > > > new_event->event = NULL > > > in this case. Don't you need new_event->event? If so, then you need > > > return NULL. > > > > yep, that's a bug.. with new being NULL in here, > > we'd get a crash anyway.. so 'return NULL;' it is > > > > SNIP > > > > > > +* yet, we need to free only allocated ones ... > > > > +*/ > > > > + list_del(>buffer->list); > > > > + ordered_events_buffer__free(oe->buffer, oe->buffer_idx, oe); > > > > + > > > > + /* ... and continue with the rest */ > > > > + list_for_each_entry_safe(buffer, tmp, >to_free, list) { > > > > + list_del(>list); > > > > + ordered_events_buffer__free(buffer, MAX_SAMPLE_BUFFER, > > > > oe); > > > > > > > > > Here you are saying that if it is on the to_free list and not the > > > current buffer, then necessarily > > > all the entries have been used and it is safe to use > > > MAX_SAMPLE_BUFFER. Is that right? > > > > yes, at this point they either holds an event or NULL > > so it's free to call __free_dup_event on it > > > > thanks, v3 attached > > > > added also Namhyung's ack, as the 'return NULL' change wasn't > > related to the v2 changes > > I think the code is correct now for the issue related to uninitialized pointer. But there is still one problem I found stressing the code with max_alloc_size. The way the following is written: if (!list_empty(cache)) { new = list_entry(cache->next, struct ordered_event, list); list_del(>list); } else if (oe->buffer) { new = oe->buffer + 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); oe->buffer = malloc(size); if (!oe->buffer) { free_dup_event(oe, new_event); return NULL; } pr("alloc size %" PRIu64 "B (+%zu), max %" PRIu64 "B\n", oe->cur_alloc_size, size, oe->max_alloc_size); oe->cur_alloc_size += size; You can end up with oe->cur_alloc_size > oe->max_alloc_size in case the max limit is really low (< size_t size = sizeof (*oe->buffer) + MAX_SAMPLE_BUFFER * sizeof(*new); So I think to make sure you can never allocate more than the max, you have to do: size_t size = sizeof(*oe->buffer) MAX_SAMPLE_BUFFER * sizeof(*new); if (!list_empty(cache)) { new = list_entry(cache->next, struct ordered_event, list); list_del(>list); } else if (oe->buffer) { new = oe->buffer + oe->buffer_idx; if (++oe->buffer_idx == MAX_SAMPLE_BUFFER) oe->buffer = NULL; } else if ((oe->cur_alloc_size + size) < oe->max_alloc_size) { Then you will never allocate more than the max. I think with this change, we are okay. Tested-by: Stephane Eranian > jirka > > Stephane, > any comments to v3 version? > > thanks, > jirka > > > > > > > --- > > 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=153376761329335=2 > > > > Reported-by: Stephane Eranian > > Acked-by: Namhyung Kim > > Link: http://lkml.kernel.org/n/tip-qrkcqm5m1sugy4q83pfn5...@git.kernel.org > > Signed-off-by: Jiri Olsa > > --- > >
Re: [PATCHv3] perf tools: Add struct ordered_events_buffer layer
On Mon, Aug 27, 2018 at 07:05:43PM +0200, Jiri Olsa wrote: > On Mon, Aug 27, 2018 at 08:24:56AM -0700, Stephane Eranian wrote: > > SNIP > > > > - /* First entry is abused to maintain the to_free list. */ > > > - oe->buffer_idx = 2; > > > - new = oe->buffer + 1; > > > + oe->buffer_idx = 1; > > > + new = >buffer->event[0]; > > > } else { > > > pr("allocation limit reached %" PRIu64 "B\n", > > > oe->max_alloc_size); > > > > > > I am wondering about the usefulness of returning a new_event with > > new_event->event = NULL > > in this case. Don't you need new_event->event? If so, then you need return > > NULL. > > yep, that's a bug.. with new being NULL in here, > we'd get a crash anyway.. so 'return NULL;' it is > > SNIP > > > > +* yet, we need to free only allocated ones ... > > > +*/ > > > + list_del(>buffer->list); > > > + ordered_events_buffer__free(oe->buffer, oe->buffer_idx, oe); > > > + > > > + /* ... and continue with the rest */ > > > + list_for_each_entry_safe(buffer, tmp, >to_free, list) { > > > + list_del(>list); > > > + ordered_events_buffer__free(buffer, MAX_SAMPLE_BUFFER, > > > oe); > > > > > > Here you are saying that if it is on the to_free list and not the > > current buffer, then necessarily > > all the entries have been used and it is safe to use > > MAX_SAMPLE_BUFFER. Is that right? > > yes, at this point they either holds an event or NULL > so it's free to call __free_dup_event on it > > thanks, v3 attached > > added also Namhyung's ack, as the 'return NULL' change wasn't > related to the v2 changes > > jirka Stephane, any comments to v3 version? thanks, jirka > > > --- > 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=153376761329335=2 > > Reported-by: Stephane Eranian > Acked-by: Namhyung Kim > Link: http://lkml.kernel.org/n/tip-qrkcqm5m1sugy4q83pfn5...@git.kernel.org > Signed-off-by: Jiri Olsa > --- > tools/perf/util/ordered-events.c | 83 +++- > tools/perf/util/ordered-events.h | 37 -- > 2 files changed, 91 insertions(+), 29 deletions(-) > > diff --git a/tools/perf/util/ordered-events.c > b/tools/perf/util/ordered-events.c > index bad9e0296e9a..87171e8fd70d 100644 > --- a/tools/perf/util/ordered-events.c > +++ b/tools/perf/util/ordered-events.c > @@ -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
Re: [PATCHv3] perf tools: Add struct ordered_events_buffer layer
On Mon, Aug 27, 2018 at 07:05:43PM +0200, Jiri Olsa wrote: > On Mon, Aug 27, 2018 at 08:24:56AM -0700, Stephane Eranian wrote: > > SNIP > > > > - /* First entry is abused to maintain the to_free list. */ > > > - oe->buffer_idx = 2; > > > - new = oe->buffer + 1; > > > + oe->buffer_idx = 1; > > > + new = >buffer->event[0]; > > > } else { > > > pr("allocation limit reached %" PRIu64 "B\n", > > > oe->max_alloc_size); > > > > > > I am wondering about the usefulness of returning a new_event with > > new_event->event = NULL > > in this case. Don't you need new_event->event? If so, then you need return > > NULL. > > yep, that's a bug.. with new being NULL in here, > we'd get a crash anyway.. so 'return NULL;' it is > > SNIP > > > > +* yet, we need to free only allocated ones ... > > > +*/ > > > + list_del(>buffer->list); > > > + ordered_events_buffer__free(oe->buffer, oe->buffer_idx, oe); > > > + > > > + /* ... and continue with the rest */ > > > + list_for_each_entry_safe(buffer, tmp, >to_free, list) { > > > + list_del(>list); > > > + ordered_events_buffer__free(buffer, MAX_SAMPLE_BUFFER, > > > oe); > > > > > > Here you are saying that if it is on the to_free list and not the > > current buffer, then necessarily > > all the entries have been used and it is safe to use > > MAX_SAMPLE_BUFFER. Is that right? > > yes, at this point they either holds an event or NULL > so it's free to call __free_dup_event on it > > thanks, v3 attached > > added also Namhyung's ack, as the 'return NULL' change wasn't > related to the v2 changes > > jirka Stephane, any comments to v3 version? thanks, jirka > > > --- > 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=153376761329335=2 > > Reported-by: Stephane Eranian > Acked-by: Namhyung Kim > Link: http://lkml.kernel.org/n/tip-qrkcqm5m1sugy4q83pfn5...@git.kernel.org > Signed-off-by: Jiri Olsa > --- > tools/perf/util/ordered-events.c | 83 +++- > tools/perf/util/ordered-events.h | 37 -- > 2 files changed, 91 insertions(+), 29 deletions(-) > > diff --git a/tools/perf/util/ordered-events.c > b/tools/perf/util/ordered-events.c > index bad9e0296e9a..87171e8fd70d 100644 > --- a/tools/perf/util/ordered-events.c > +++ b/tools/perf/util/ordered-events.c > @@ -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
[PATCHv3] perf tools: Add struct ordered_events_buffer layer
On Mon, Aug 27, 2018 at 08:24:56AM -0700, Stephane Eranian wrote: SNIP > > - /* First entry is abused to maintain the to_free list. */ > > - oe->buffer_idx = 2; > > - new = oe->buffer + 1; > > + oe->buffer_idx = 1; > > + new = >buffer->event[0]; > > } else { > > pr("allocation limit reached %" PRIu64 "B\n", > > oe->max_alloc_size); > > > I am wondering about the usefulness of returning a new_event with > new_event->event = NULL > in this case. Don't you need new_event->event? If so, then you need return > NULL. yep, that's a bug.. with new being NULL in here, we'd get a crash anyway.. so 'return NULL;' it is SNIP > > +* yet, we need to free only allocated ones ... > > +*/ > > + list_del(>buffer->list); > > + ordered_events_buffer__free(oe->buffer, oe->buffer_idx, oe); > > + > > + /* ... and continue with the rest */ > > + list_for_each_entry_safe(buffer, tmp, >to_free, list) { > > + list_del(>list); > > + ordered_events_buffer__free(buffer, MAX_SAMPLE_BUFFER, oe); > > > Here you are saying that if it is on the to_free list and not the > current buffer, then necessarily > all the entries have been used and it is safe to use > MAX_SAMPLE_BUFFER. Is that right? yes, at this point they either holds an event or NULL so it's free to call __free_dup_event on it thanks, v3 attached added also Namhyung's ack, as the 'return NULL' change wasn't related to the v2 changes jirka --- 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=153376761329335=2 Reported-by: Stephane Eranian Acked-by: Namhyung Kim Link: http://lkml.kernel.org/n/tip-qrkcqm5m1sugy4q83pfn5...@git.kernel.org Signed-off-by: Jiri Olsa --- tools/perf/util/ordered-events.c | 83 +++- tools/perf/util/ordered-events.h | 37 -- 2 files changed, 91 insertions(+), 29 deletions(-) diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c index bad9e0296e9a..87171e8fd70d 100644 --- a/tools/perf/util/ordered-events.c +++ b/tools/perf/util/ordered-events.c @@ -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)) {
[PATCHv3] perf tools: Add struct ordered_events_buffer layer
On Mon, Aug 27, 2018 at 08:24:56AM -0700, Stephane Eranian wrote: SNIP > > - /* First entry is abused to maintain the to_free list. */ > > - oe->buffer_idx = 2; > > - new = oe->buffer + 1; > > + oe->buffer_idx = 1; > > + new = >buffer->event[0]; > > } else { > > pr("allocation limit reached %" PRIu64 "B\n", > > oe->max_alloc_size); > > > I am wondering about the usefulness of returning a new_event with > new_event->event = NULL > in this case. Don't you need new_event->event? If so, then you need return > NULL. yep, that's a bug.. with new being NULL in here, we'd get a crash anyway.. so 'return NULL;' it is SNIP > > +* yet, we need to free only allocated ones ... > > +*/ > > + list_del(>buffer->list); > > + ordered_events_buffer__free(oe->buffer, oe->buffer_idx, oe); > > + > > + /* ... and continue with the rest */ > > + list_for_each_entry_safe(buffer, tmp, >to_free, list) { > > + list_del(>list); > > + ordered_events_buffer__free(buffer, MAX_SAMPLE_BUFFER, oe); > > > Here you are saying that if it is on the to_free list and not the > current buffer, then necessarily > all the entries have been used and it is safe to use > MAX_SAMPLE_BUFFER. Is that right? yes, at this point they either holds an event or NULL so it's free to call __free_dup_event on it thanks, v3 attached added also Namhyung's ack, as the 'return NULL' change wasn't related to the v2 changes jirka --- 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=153376761329335=2 Reported-by: Stephane Eranian Acked-by: Namhyung Kim Link: http://lkml.kernel.org/n/tip-qrkcqm5m1sugy4q83pfn5...@git.kernel.org Signed-off-by: Jiri Olsa --- tools/perf/util/ordered-events.c | 83 +++- tools/perf/util/ordered-events.h | 37 -- 2 files changed, 91 insertions(+), 29 deletions(-) diff --git a/tools/perf/util/ordered-events.c b/tools/perf/util/ordered-events.c index bad9e0296e9a..87171e8fd70d 100644 --- a/tools/perf/util/ordered-events.c +++ b/tools/perf/util/ordered-events.c @@ -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)) {