Messages in this thread | | | Date | Mon, 11 Oct 2021 21:56:40 +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 07:40:33PM +0300, Bayduraev, Alexey V wrote: > > > On 11.10.2021 16:21, Jiri Olsa wrote: > > 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 > > Now I got it, thanks ;) > > With your suggestion, for multiple trace reader, we should always > remember to switch active_decomp when switching the reader object, > just passing the current reader pointer to the reader__read_event > function will not be enough. I thought it would be better to hide > such details in the reader__read_event function. > > Of course, I can move setting of active_decomp outside of > reader__read_event if this is better from your point of view.
at the moment it's not necessary to set/unset it for each event, let's do that when it's really needed in the following changes for threaded perf record with explanation
thanks, jirka
> > Regards, > Alexey > > > > > 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 > >>>>>>> > >>>>>> > >>>>> > >>>> > >> > > >
| |