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 53ED6C3DA59 for ; Mon, 22 Jul 2024 08:29:56 +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=+miZW+jDIJz6oUu4IYBQyU51RBxZgHppBXImsiiQXdI=; b=gcaxrZKRWzyjSdDAA4F1uDPNfJ MbsovgrzaFJhVpVoHu6GKGNkjieDYyfbP5mq532Hsp/xneDIalSmXRrDZAItHBmZNmn37bJpuH6M7 kMe7UXgpO+kkjudiLVKmfVoJvrdwBfLlLmj5J8EHpVto5UQLINHrOeF+Vvrn21cpSCDrNkwXk2an4 gX2759nwpY0/6hxW5V6O2QeAJWOk7jjZibCBY0U2t1saktQaybpJFo79cvmi8nJw3bk+KI++uidLj ZHMFuAYEQAzykfF7l8QuT1WvSdooZZ06VBFOT1uw0HLvBiRjuc9ibWVbOd/vNDmBKI99Li+/6ex1s a9bKUtwQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sVoQP-00000008tOg-0wph; Mon, 22 Jul 2024 08:29:41 +0000 Received: from mgamail.intel.com ([192.198.163.7]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sVoQ2-00000008tFJ-0N3H for linux-arm-kernel@lists.infradead.org; Mon, 22 Jul 2024 08:29:19 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1721636958; x=1753172958; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=WVUiUZZrhSyC5ogpqbKGLX1PHFwhFxJSq6enD4jXS94=; b=lkIUxrzVFiUMXPc771g1OvlNBdaMMHU4DC2kGZafbASG9F+3X7r6aoVJ JmxdfD5/Bnjd9g1+gMfCYXFIGjlAfpjD2t2dM7WxEMBs2/6eeaw2HdyOv TtUBp934TYItF8lYH9oHQD9fIziqdFh+lHpJGIuYaJ3wl528kPsThqqso 7MqK7ilaZoJfKHtFkw0V4l6RjCsxcKGX0KYNSkIXc6QA1hXLaZR9034MC FW99r1VwbqVv5SvDJRXt+LMMTpjm+HBnQwC3O7bfZL508AU9B5bO6QHfj 4xX5xkmjotr4QKP6nkkR6bVk1q+6tM2xQ/nHOtMeebTL9/sxpUuhCsxKe w==; X-CSE-ConnectionGUID: +doNSDl7Q6Sr79K6q85JcA== X-CSE-MsgGUID: T6PLfjyWQEWTi0mjEQCl0Q== X-IronPort-AV: E=McAfee;i="6700,10204,11140"; a="44618486" X-IronPort-AV: E=Sophos;i="6.09,227,1716274800"; d="scan'208";a="44618486" Received: from fmviesa005.fm.intel.com ([10.60.135.145]) by fmvoesa101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Jul 2024 01:28:55 -0700 X-CSE-ConnectionGUID: qxVdrlWkQOOjlTuJ3O6rdQ== X-CSE-MsgGUID: RxoZp5BWT42LIo+ODeZ7iQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.09,227,1716274800"; d="scan'208";a="56092103" Received: from ahunter6-mobl1.ger.corp.intel.com (HELO [10.0.2.15]) ([10.246.41.28]) by fmviesa005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Jul 2024 01:28:48 -0700 Message-ID: <05fa0449-4fd4-41ed-93e8-db825e48268f@intel.com> Date: Mon, 22 Jul 2024 11:28:44 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v6 00/27] Constify tool pointers To: Ian Rogers Cc: Peter Zijlstra , Ingo Molnar , Arnaldo Carvalho de Melo , Namhyung Kim , 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> 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-20240722_012918_184995_5CEA273E X-CRM114-Status: GOOD ( 37.16 ) 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 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; > > 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. Really, 'tool' needs to be defined as const in the first place. > 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() > 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(...); > 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.