From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BE7E5C433EF for ; Mon, 11 Oct 2021 16:45:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 95C8160F3A for ; Mon, 11 Oct 2021 16:45:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231739AbhJKQrB (ORCPT ); Mon, 11 Oct 2021 12:47:01 -0400 Received: from mga05.intel.com ([192.55.52.43]:38929 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229816AbhJKQrA (ORCPT ); Mon, 11 Oct 2021 12:47:00 -0400 X-IronPort-AV: E=McAfee;i="6200,9189,10134"; a="313112875" X-IronPort-AV: E=Sophos;i="5.85,364,1624345200"; d="scan'208";a="313112875" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Oct 2021 09:40:42 -0700 X-IronPort-AV: E=Sophos;i="5.85,364,1624345200"; d="scan'208";a="490546679" Received: from abaydur-mobl1.ccr.corp.intel.com (HELO [10.249.229.105]) ([10.249.229.105]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 11 Oct 2021 09:40:36 -0700 Subject: Re: [PATCH v3 6/8] perf session: Move event read code to separate function To: Jiri Olsa Cc: Arnaldo Carvalho de Melo , Namhyung Kim , Alexander Shishkin , Peter Zijlstra , Ingo Molnar , linux-kernel , Andi Kleen , Adrian Hunter , Alexander Antonov , Alexei Budankov , Riccardo Mancini References: <6ab47325fa261deca4ca55ecacf1ca2437abcd78.1633596227.git.alexey.v.bayduraev@linux.intel.com> <5e5ecfcd-57f1-1a06-4ed6-6a1e6983d1f8@linux.intel.com> From: "Bayduraev, Alexey V" Organization: Intel Corporation Message-ID: Date: Mon, 11 Oct 2021 19:40:33 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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 >>>>>>> >>>>>> >>>>> >>>> >> >