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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <YWSW+PLDJJlkjlXz@krava>
Date:   Mon, 11 Oct 2021 21:56:40 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     "Bayduraev, Alexey V" <alexey.v.bayduraev@...ux.intel.com>
Cc:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Andi Kleen <ak@...ux.intel.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Alexander Antonov <alexander.antonov@...ux.intel.com>,
        Alexei Budankov <abudankov@...wei.com>,
        Riccardo Mancini <rickyman7@...il.com>
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
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
> > 
>