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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B8A2DC3DA49 for ; Tue, 23 Jul 2024 09:39:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:In-Reply-To:From:References:Cc:To:Subject:MIME-Version:Date: Message-ID:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=Xs8MM/mikOHErW8aLuYSD6S0FiS5u8Sr7IlyY3jh58Q=; b=bvNRxNcmHWPY8uBQ/DBFgC/fkO RiH1wnZkNVDr2a11zm9Wq5uvRE3eSawGzRgzbskz5vl5m2fGRrQFmCKQpGHNsnp6btileq207HoVb fYUy8xmYlWic/eXGlcacqafgKTP0mfYaeFhduaucDSnVye52aARWn2g2xjGduyQoPoTtFurvC3+NX nTleTJP07zILHOsi+h8CZsvzVFQeHDKagSMbA/pzS/FlZtLHM6JU057yCxQDR0zVXaerBHvK3NNQF jOsUuSm2v7z0Na2T6xrPauPtLKmyv/oOGB+jLBOKZ2I95+TS+v4FFhdORiB5rk2uCaJpGzoO0VeBV NKBmZzLA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sWBz5-0000000C381-43te; Tue, 23 Jul 2024 09:39:04 +0000 Received: from mgamail.intel.com ([192.198.163.16]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sWByh-0000000C31m-0lzN for linux-arm-kernel@lists.infradead.org; Tue, 23 Jul 2024 09:38:41 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1721727519; x=1753263519; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=tTaCjEsKqRpzcHF+EyOIq9OzbUjorqzZxzkrSqU2dz8=; b=iis1rHCKqlLeJ3W7mEfxgD1ZbehJEBtINop47zxvHTpxhZN+RJnTIwXG RD8IpKA2SFzijuipawCieS/3o2jROtoSFZGH7dpVfK/bArYMLryvpU3ww qwMGYsPfHcW3g8oons+VjTWXOltlKZJy4h0+Vz+fOjvgmcn3OIN5FHOXr GcPPbcK8WWB3N89k9gqE8GkreDOmX/CW0GxTcIwzTek8e0BdFLns5TVwj 1rcfI8T/9YSCNpRbiBqEayUzCL733N3WHjJXjH9WTwO0Kc0IAcN2dC/aI wkuHdf68/N+uTImex4b/hKjCA+0OuYJiQEKg3+eaX1lNtTeaYQ0NFJP8v A==; X-CSE-ConnectionGUID: 5nW6LBazT3aim+3YY8MjbA== X-CSE-MsgGUID: NAO1H8AnSyKLJNzFn2BnxA== X-IronPort-AV: E=McAfee;i="6700,10204,11141"; a="12664238" X-IronPort-AV: E=Sophos;i="6.09,230,1716274800"; d="scan'208";a="12664238" Received: from fmviesa004.fm.intel.com ([10.60.135.144]) by fmvoesa110.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Jul 2024 02:38:37 -0700 X-CSE-ConnectionGUID: R8uSE1D3SFSnfG/uRIwByQ== X-CSE-MsgGUID: PiTE7INaRpCepkEeiegFFA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.09,230,1716274800"; d="scan'208";a="56732595" Received: from ahunter6-mobl1.ger.corp.intel.com (HELO [10.0.2.15]) ([10.94.249.84]) by fmviesa004-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Jul 2024 02:38:30 -0700 Message-ID: <5444c426-a4be-4b85-b9d4-65aac78115be@intel.com> Date: Tue, 23 Jul 2024 12:38:24 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v6 00/27] Constify tool pointers To: Ian Rogers , Namhyung Kim Cc: Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Mark Rutland , Alexander Shishkin , Jiri Olsa , Kan Liang , John Garry , Will Deacon , James Clark , Mike Leach , Leo Yan , Suzuki K Poulose , Yicong Yang , Jonathan Cameron , Nick Terrell , Nick Desaulniers , Oliver Upton , Anshuman Khandual , Song Liu , Ilkka Koskinen , Athira Rajeev , Huacai Chen , Yanteng Si , Sun Haiyong , linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, linux-arm-kernel@lists.infradead.org References: <20240718010023.1495687-1-irogers@google.com> <738b5c89-acb2-46a5-92a1-c36bd90abc30@intel.com> <05fa0449-4fd4-41ed-93e8-db825e48268f@intel.com> Content-Language: en-US From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240723_023839_275112_A8913437 X-CRM114-Status: GOOD ( 50.23 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 22/07/24 21:04, Ian Rogers wrote: > On Mon, Jul 22, 2024 at 10:50 AM Ian Rogers wrote: >> >> On Mon, Jul 22, 2024 at 10:45 AM Namhyung Kim wrote: >>> >>> Hi, >>> >>> On Mon, Jul 22, 2024 at 9:06 AM Ian Rogers wrote: >>>> >>>> On Mon, Jul 22, 2024 at 1:29 AM Adrian Hunter wrote: >>>>> >>>>> On 19/07/24 19:26, Ian Rogers wrote: >>>>>> On Fri, Jul 19, 2024 at 1:51 AM Adrian Hunter wrote: >>>>>>> >>>>>>> On 18/07/24 03:59, Ian Rogers wrote: >>>>>>>> struct perf_tool provides a set of function pointers that are called >>>>>>>> through when processing perf data. To make filling the pointers less >>>>>>>> cumbersome, if they are NULL perf_tools__fill_defaults will add >>>>>>>> default do nothing implementations. >>>>>>>> >>>>>>>> This change refactors struct perf_tool to have an init function that >>>>>>>> provides the default implementation. The special use of NULL and >>>>>>>> perf_tools__fill_defaults are removed. As a consequence the tool >>>>>>>> pointers can then all be made const, which better reflects the >>>>>>>> behavior a particular perf command would expect of the tool and to >>>>>>>> some extent can reduce the cognitive load on someone working on a >>>>>>>> command. >>>>>>>> >>>>>>>> v6: Rebase adding Adrian's reviewed-by/tested-by and Leo's tested-by. >>>>>>> >>>>>>> The tags were really meant only for patch 1, the email that was replied to. >>>>>>> >>>>>>> But now for patches 2 and 3: >>>>>>> >>>>>>> Reviewed-by: Adrian Hunter >>>>>> >>>>>> Sorry for that, you'd mentioned that pt and bts testing which is >>>>>> impacted by more than just patch 1. >>>>>> >>>>>>> Looking at patches 4 to 25, they do not seem to offer any benefit. >>>>>>> >>>>>>> Instead of patch 26, presumably perf_tool__fill_defaults() could >>>>>>> be moved to __perf_session__new(), which perhaps would allow >>>>>>> patch 27 as it is. >>>>>> >>>>>> What I'm trying to do in the series is make it so that the tool isn't >>>>>> mutated during its use by session. Ideally we'd be passing a const >>>>>> tool to session_new, that's not possible because there's a hack to fix >>>>>> ordered events and pipe mode in session__new: >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/session.c?h=perf-tools-next#n275 >>>>>> Imo, it isn't great to pass a tool to session__new where you say you >>>>>> want ordered events and then session just goes to change that for you. >>>>>> Altering that behavior was beyond the scope of this clean up, so tool >>>>>> is only const after session__new. >>>>> >>>>> Seems like a separate issue. Since the session is created >>>>> by __perf_session__new(), session->tool will always be a pointer >>>>> to a const tool once there is: >>>>> >>>>> diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h >>>>> index 7f69baeae7fb..7c8dd6956330 100644 >>>>> --- a/tools/perf/util/session.h >>>>> +++ b/tools/perf/util/session.h >>>>> @@ -43,7 +43,7 @@ struct perf_session { >>>>> u64 one_mmap_offset; >>>>> struct ordered_events ordered_events; >>>>> struct perf_data *data; >>>>> - struct perf_tool *tool; >>>>> + const struct perf_tool *tool; >>>>> u64 bytes_transferred; >>>>> u64 bytes_compressed; >>>>> struct zstd_data zstd_data; >>>> >>>> That's the case after these changes, but not before. >>>> >>>>>> >>>>>> The reason for doing this is to make it so that when I have a tool I >>>>>> can reason that nobody is doing things to change it under my feet. >>>>> >>>>> It still can be changed by the caller of __perf_session__new(), since >>>>> the tool itself is not const. >>>>> >>>>> Anything using container_of() like: >>>>> >>>>> static int process_sample_event(const struct perf_tool *tool, >>>>> union perf_event *event, >>>>> struct perf_sample *sample, >>>>> struct evsel *evsel, >>>>> struct machine *machine) >>>>> { >>>>> struct perf_script *scr = container_of(tool, struct perf_script, tool); >>>>> >>>>> can then change scr->tool without even having to cast away const. >>>> >>>> Agreed, but such things happen in builtin_cmd where the tool is >>>> defined and presumably they know what they are doing. My objection is >>>> to code in util mutating the tool as I want the tool to have >>>> predictable behavior. As callers that take a tool can call fill in >>>> defaults (not all) then the tool has to be mutable and I don't want >>>> this to be the case. >>>> >>>>> Really, 'tool' needs to be defined as const in the first place. >>>> >>>> I'd like this. The problem is initializing all the function pointers >>>> and making such initialization robust to extra functions being added >>>> to the tool API. It can be done in a long winded way but I couldn't >>>> devise macro magic to do it. The other problem is around variables >>>> like ordered_events that can't currently be const. The patches move us >>>> closer to this being a possibility. >>>> >>>>>> My >>>>>> builtin_cmd is in charge of what the tool is rather than some code >>>>>> buried in util that thought it was going to do me a favor. The code is >>>>>> a refactor and so the benefit is intended to be for the developer and >>>>>> how they reason about the use of tool. >>>>> >>>>> It creates another question though: since there is a lot of code >>>>> before perf_tool__init() is called, does the caller mistakenly >>>>> change tool before calling perf_tool__init() >>>> >>>> If they do this their function pointers will be clobbered and their >>>> code won't behave as expected, which I'd expect to be easy to observe. >>>> In C++ if you were to initialize memory and then use the memory for a >>>> placement new to create an object which would call the constructor, >>>> the expected behavior would be that the initialized memory's values >>>> would get overridden. I see the use of _init and _exit in the code as >>>> being our poor man replacements of constructors and destructors. >>>> >>>>>> how they reason about the use of tool. We generally use _init >>>>>> functions rather than having _fill_defaults, so there is a consistency >>>>>> argument. >>>>> >>>>> The caller does not need the "defaults", so why would it set them up. >>>>> The session could just as easily do: >>>>> >>>>> if (tool->cb) >>>>> tool->cb(...); >>>>> else >>>>> cb_stub(...); >>>> >>>> Multiplied by every stub, we'd probably need a helper function, how to >>>> handle argument passing. There's nothing wrong with this as an idea >>>> but I think of this code as trying to create a visitor pattern and >>>> this is a visitor pattern with a hard time for the caller. >>>> >>>>>> I don't expect any impact in terms of performance... Moving >>>>>> perf_tool__fill_defaults to __perf_session__new had issues with the >>>>>> existing code where NULL would be written over a function pointer >>>>>> expecting the later fill_defaults to fix it up, doesn't address coding >>>>>> consistency where _init is the norm, and adds another reason the tool >>>>>> passed to session__new can't be const. >>>>> >>>>> perf_tool__init() is not a steeping stone to making 'tool' a >>>>> const in the first place. >>>> >>>> It is because the patch series gets rid of fill in defaults which is >>>> why we have a mutable tool passed around. I don't think this is up for >>>> debate as the patch series clearly goes from a non-const >>>> tool to a const tool at the end. Changing perf_tool__init to make all >>>> the function pointers NULL and then making every caller have to do a: >>>> >>>> if (tool->cb) >>>> tool->cb(...); >>>> else >>>> cb_stub(...); >>>> >>>> I think it is a less elegant solution at the end, it is also a large >>>> and more invasive change. The various refactorings to make tool const, >>>> changing the aux use of tool, etc. wouldn't be impacted by such a >>>> change but I think it is out of scope for this patch series. >>> >>> I don't think it's a large and invasive change. The tools are mostly >>> zero-initialized so we don't need to reset to NULL. And tool->cb is >>> called mostly from two functions: machines__deliver_event() and >>> perf_session__process_user_event(). Can we change them to check >>> NULL and get rid of perf_tool__fill_defaults() to keep it const? >> >> As I said above, I don't think that is good style and is out of scope >> here. It clearly can be done as follow up, but I don't see how that >> fixes the style issue. > > Just to be clear on what the style issue is. We already have code: > https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-record.c?h=perf-tools-next#n1461 > ``` > if (rec->buildid_all && !rec->timestamp_boundary) > rec->tool.sample = NULL; > ``` > that relies on the special behavior of NULL in a function pointer > being changed at dispatch time - a simple reading of that code would > be anyone calling the function pointer would get a segv. I'm trying to > make it so that NULL isn't magic in the context of tool and you can > simply look at the tool to understand what its behavior is, much as a > virtual method table would work if we could do proper object-oriented > development. In C, NULL or zero is often used as a special value to mean no-value. Optional callbacks that are NULL is also not remarkable.