Messages in this thread | | | Date | Mon, 11 Oct 2021 15:21:19 +0200 | From | Jiri Olsa <> | Subject | Re: [PATCH v3 6/8] perf session: Move event read code to separate function |
| |
On Mon, Oct 11, 2021 at 12:53:30PM +0300, Bayduraev, Alexey V wrote: > > > On 11.10.2021 12:08, Bayduraev, Alexey V wrote: > > > > On 08.10.2021 17:38, Jiri Olsa wrote: > >> On Fri, Oct 08, 2021 at 11:42:18AM +0300, Bayduraev, Alexey V wrote: > >>> > >>> > >>> On 08.10.2021 10:33, Jiri Olsa wrote: > >>>> On Thu, Oct 07, 2021 at 01:25:41PM +0300, Alexey Bayduraev wrote: > >>>> > >>>> SNIP > >>>> > >>>>> static int > >>>>> -reader__process_events(struct reader *rd, struct perf_session *session, > >>>>> - struct ui_progress *prog) > >>>>> +reader__read_event(struct reader *rd, struct perf_session *session, > >>>>> + struct ui_progress *prog) > > > > SNIP > > > >>>> > >>>> active_decomp should be set/unset within reader__process_events, > >>>> not just for single event read, right? > >>> > >>> No, it should be set before perf_session__process_event/process_decomp_events > >>> and unset after these calls. So active_decomp setting/unsetting is moved in > >>> this patch to the reader__read_event function. This is necessary for multiple > >>> trace reader because it could call reader__read_event in round-robin manner. > >> > >> hum, is that code already in? I can't see this happening in current code > > > > Probably I don't understand the question. In [PATCH v3 2/8] I introduced > > active_decomp pointer in perf_session. It is initialized by a pointer to the > > decompressor object in perf_session. In reader__process_events it is set to > > the reader decompressor object. And it is reset to the session decompressor > > object at exit. In this case we do not need to reset it after each > > perf_session__process_event because this code reads events in loop with > > constant reader object. Maybe setting of active_decomp should be at the > > entrance to the reader__process_events, not before reader__process_events, > > in [PATCH v3 2/8]. All this code is new. > > We set active_decomp for perf_session__process_event (rd->process() in our > case) and for __perf_session__process_decomp_events, active_decomp is not > necessary for other parts of reader__process_events.
so what I see in the code is:
__perf_session__process_events { struct reader rd;
reader__process_events(rd) { reader__read_event(rd) { -> session->active_decomp = &rd->decomp_data; rd->process(... -> session->active_decomp = &session->decomp_data; }
} }
we set session->active_decomp for each event that we process and I don't understand why we can't do that just once in __perf_session__process_events, so it'd be like:
__perf_session__process_events { struct reader rd;
-> session->active_decomp = &rd->decomp_data;
reader__process_events(rd) { reader__read_event(rd) { rd->process(... }
}
-> session->active_decomp = &session->decomp_data; }
or within reader__process_events if it's more convenient
jirka
> > Regards, > Alexey > > > > > In this patch I separates single event reading and moves setting/resetting > > of active_decomp before/after perf_session__process_event because this is > > necessary for multiple trace reader. > > > > Regards, > > Alexey > > > >> > >> jirka > >> > >>> > >>> Regards, > >>> Alexey > >>> > >>>> > >>>> jirka > >>>> > >>>>> return err; > >>>>> } > >>>>> > >>>>> -- > >>>>> 2.19.0 > >>>>> > >>>> > >>> > >> >
| |