Re: [PATCH v3 6/8] perf session: Move event read code to separate function

From: Jiri Olsa
Date: Mon Oct 11 2021 - 15:56:48 EST


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
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
> >
>