Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 11 Oct 2021 21:56:40 +0200
From:   Jiri Olsa <jolsa@redhat.com>
To:     "Bayduraev, Alexey V" <alexey.v.bayduraev@linux.intel.com>
Cc:     Arnaldo Carvalho de Melo <acme@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Andi Kleen <ak@linux.intel.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Alexander Antonov <alexander.antonov@linux.intel.com>,
        Alexei Budankov <abudankov@huawei.com>,
        Riccardo Mancini <rickyman7@gmail.com>
Subject: Re: [PATCH v3 6/8] perf session: Move event read code to separate
 function
Message-ID: <YWSW+PLDJJlkjlXz@krava>
References: <cover.1633596227.git.alexey.v.bayduraev@linux.intel.com>
 <6ab47325fa261deca4ca55ecacf1ca2437abcd78.1633596227.git.alexey.v.bayduraev@linux.intel.com>
 <YV/0ZZBu01V87A8e@krava>
 <aa62d0ed-abca-2123-c8bf-cd6bced2fe9c@linux.intel.com>
 <YWBX+p1vtLXXbpkF@krava>
 <5e5ecfcd-57f1-1a06-4ed6-6a1e6983d1f8@linux.intel.com>
 <f8d0accb-b569-3fcd-ffe5-99e2fab4c8b7@linux.intel.com>
 <YWQ6TyyYRfx9AXLH@krava>
 <d630cf0d-1bb5-0527-411a-c70a01e2ddea@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <d630cf0d-1bb5-0527-411a-c70a01e2ddea@linux.intel.com>
Precedence: bulk

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