On Fri, 6 Jul 2018 15:46:54 +0300
"Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx> wrote:
> > static size_t get_records(struct kshark_context *kshark_ctx,
> > - struct rec_list ***rec_list)
> > + struct rec_list ***rec_list, enum rec_type type)
> > {
> > + struct event_filter *adv_filter = NULL;
> > struct pevent_record *rec;
> > struct rec_list **temp_next;
> > struct rec_list **cpu_list;
> > @@ -547,12 +560,50 @@ static size_t get_records(struct kshark_
> >
> > rec = tracecmd_read_cpu_first(kshark_ctx->handle, cpu);
> > while (rec) {
> > - *temp_next = temp_rec = malloc(sizeof(*temp_rec));
> > + *temp_next = temp_rec = calloc(1, sizeof(*temp_rec));
> > if (!temp_rec)
> > goto fail;
> >
> > - temp_rec->rec = rec;
> > temp_rec->next = NULL;
> > +
> > + switch (type) {
> > + case REC_RECORD:
> > + temp_rec->rec = rec;
> > + break;
> > + case REC_ENTRY: {
> > + struct kshark_task_list *task;
> > + struct kshark_entry *entry;
> > + int ret;
> > +
> > + if (!adv_filter)
> > + adv_filter = kshark_ctx->advanced_event_filter;
>
> I defined "adv_filter" just as a short version of
> "kshark_ctx->advanced_event_filter", because of the 80 columns limit on
> the length of lines. You can move this outside of the while() loop;
I didn't want to put it outside the loop, because I didn't want it to
be required for REC_RECORD. But I think outside the loop, I can just do:
if (type == REC_ENTRY)
adv_filter = ....
>
> > + entry = &temp_rec->entry;
> > + kshark_set_entry_values(kshark_ctx, rec, entry);
>
> Maybe the call of kshark_add_task() can be moved to
> kshark_load_data_entries()
> Just to be consistent with kshark_load_data_records()
The problem is that "rec" is freed. That was required as part of the
speed up.
>
> > + task = kshark_add_task(kshark_ctx, entry->pid);
> > + if (!task) {
> > + free_record(rec);
> > + goto fail;
> > + }
> > +
> > + /* Apply event filtering. */
> > + ret = FILTER_NONE;
>
> Opps, I made a bug here. It has to be
> ret = FILTER_MATCH;
Can you send a patch. I don't want this to be a fix. That should be a
separate patch.
>
> > + if (adv_filter->filters)
> > + ret = pevent_filter_match(adv_filter, rec);
> > +
> > + if (!kshark_show_event(kshark_ctx, entry->event_id) ||
> > + ret != FILTER_MATCH) {
> > + unset_event_filter_flag(kshark_ctx, entry);
> > + }
> > +
> > + /* Apply task filtering. */
> > + if (!kshark_show_task(kshark_ctx, entry->pid)) {
> > + entry->visible &= ~kshark_ctx->filter_mask;
> > + }
> > + free_record(rec);
> > + break;
> > + } /* REC_ENTRY */
> > + }
> > +
> > temp_next = &temp_rec->next;
> >
> > ++count;
> > @@ -566,13 +617,15 @@ static size_t get_records(struct kshark_
> > return total;
> >
> > fail:
> > - free_rec_list(cpu_list, n_cpus);
> > + free_rec_list(cpu_list, n_cpus, type);
> > return -ENOMEM;
> > }
> >
>
> [..]
>
> >
> > Index: trace-cmd.git/kernel-shark-qt/src/libkshark.h
> > ===================================================================
> > --- trace-cmd.git.orig/kernel-shark-qt/src/libkshark.h
> > +++ trace-cmd.git/kernel-shark-qt/src/libkshark.h
> > @@ -33,6 +33,9 @@ extern "C" {
> > * info etc.) is available on-demand via the offset into the trace file.
> > */
> > struct kshark_entry {
> > + /** Pointer to the next (in time) kshark_entry on the same CPU core. */
> > + struct kshark_entry *next; /* MUST BE FIRST ENTRY */
> > +
> Correct, thanks!
Yep, this is a common trick I use.
-- Steve
>
> Yordan
>
> > /**
> > * A bit mask controlling the visibility of the entry. A value of OxFF
> > * would mean that the entry is visible everywhere. Use
> > @@ -60,9 +63,6 @@ struct kshark_entry {
> > * started.
> > */
> > uint64_t ts;
> > -
> > - /** Pointer to the next (in time) kshark_entry on the same CPU core. */
> > - struct kshark_entry *next;
> > };
> >
> > /** Size of the task's hash table. */
> >
|