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 1ADA5C16B13 for ; Tue, 23 Apr 2024 09:56:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id: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=e03jIsCmpSLaBd9FXkcbmthc9+UL9OKYORRap5p+J6U=; b=UelnCA+Xx3041r ITrBYGfM6HXRBd7a6CVtlwWI+U/jYQULf1uJX1+3fHNysseRSIy4go7TkCTJIYY64zO6axhjivQT5 tj7rwhEZqq0wWvvQMrAdc1KlpMQd53Zbqvdc4fyGdmKl7igiahQ3cbrw6zjEvhBLVK/btCob7Ep+S epU4cASd8MdCj3Ni92+RqYHaS/RP5m8Rr7m/itlAoEDqEQUqEUrwHher/AEb0lOdMPWplMSArNfET 1s9HPhHmOn/HjhgJbF2NTS6bIefnKpvf4pANkKpKTnkN+eawuHnrrJkn9JY83CZnNJBgqXFoHfcxs vmgDs4gvVDz0MH+1KteA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rzCsb-0000000GkTw-1lmU; Tue, 23 Apr 2024 09:56:01 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rzCsX-0000000GkR6-3zjv for linux-arm-kernel@lists.infradead.org; Tue, 23 Apr 2024 09:56:00 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C5366339; Tue, 23 Apr 2024 02:56:21 -0700 (PDT) Received: from [192.168.1.216] (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id F0E203F7BD; Tue, 23 Apr 2024 02:55:50 -0700 (PDT) Message-ID: Date: Tue, 23 Apr 2024 10:55:32 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH v2 2/4] perf: Allow adding fixed random jitter to the alternate sampling period To: Ben Gainey Cc: "acme@kernel.org" , "alexander.shishkin@linux.intel.com" , "ak@linux.intel.com" , "mingo@redhat.com" , "namhyung@kernel.org" , Mark Rutland , "peterz@infradead.org" , "linux-perf-users@vger.kernel.org" , "adrian.hunter@intel.com" , "will@kernel.org" , "irogers@google.com" , "jolsa@kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" References: <20240422104929.264241-1-ben.gainey@arm.com> <20240422104929.264241-3-ben.gainey@arm.com> <1dd3692d-dd2c-428f-a7f7-e263d1d5e5c8@arm.com> <50f8e635667f2c0b389f882bc3f881552ea77e68.camel@arm.com> Content-Language: en-US From: James Clark In-Reply-To: <50f8e635667f2c0b389f882bc3f881552ea77e68.camel@arm.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240423_025558_341919_C0125E8F X-CRM114-Status: GOOD ( 25.63 ) 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: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 22/04/2024 15:40, Ben Gainey wrote: > On Mon, 2024-04-22 at 14:08 +0100, James Clark wrote: >> >> >> On 22/04/2024 11:49, Ben Gainey wrote: >>> This change modifies the core perf overflow handler, adding some >>> small >>> random jitter to each sample period whenever an event switches >>> between the >>> two alternate sample periods. A new flag is added to >>> perf_event_attr to >>> opt into this behaviour. >>> >>> This change follows the discussion in [1], where it is recognized >>> that it >>> may be possible for certain patterns of execution to end up with >>> biased >>> results. >>> >>> [1] https://lore.kernel.org/linux-perf- >>> users/Zc24eLqZycmIg3d2@tassilo/ >>> >>> Signed-off-by: Ben Gainey >>> --- >>> include/uapi/linux/perf_event.h | 3 ++- >>> kernel/events/core.c | 11 ++++++++++- >>> 2 files changed, 12 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/uapi/linux/perf_event.h >>> b/include/uapi/linux/perf_event.h >>> index 5c1701d091cf..dd3697a4b300 100644 >>> --- a/include/uapi/linux/perf_event.h >>> +++ b/include/uapi/linux/perf_event.h >>> @@ -461,7 +461,8 @@ struct perf_event_attr { >>> inherit_thread : 1, /* children only inherit if cloned with >>> CLONE_THREAD */ >>> remove_on_exec : 1, /* event is removed from task on exec */ >>> sigtrap : 1, /* send synchronous SIGTRAP on event */ >>> - __reserved_1 : 26; >>> + jitter_alternate_period : 1, /* add a limited amount of jitter on >>> each alternate period */ >>> + __reserved_1 : 25; >>> >>> union { >>> __u32 wakeup_events; /* wakeup every n events */ >>> diff --git a/kernel/events/core.c b/kernel/events/core.c >>> index 07f1f931e18e..079ae520e836 100644 >>> --- a/kernel/events/core.c >>> +++ b/kernel/events/core.c >>> @@ -15,6 +15,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> #include >>> #include >>> #include >>> @@ -9546,6 +9547,8 @@ static inline bool sample_is_allowed(struct >>> perf_event *event, struct pt_regs *r >>> return true; >>> } >>> >>> +# define MAX_ALT_SAMPLE_PERIOD_JITTER 16 >>> + >> >> Is 16 enough to make a difference with very large alternate periods? >> I'm >> wondering if it's worth making it customisable and instead of adding >> the >> boolean option add a 16 bit jitter field. Or the option could still >> be a >> boolean but the jitter value is some ratio of the alt sample period, >> like: >> >> get_random_u32_below(max(16, alternative_sample_period >> 4)) >> > > I don't really have a strong opinion; in all my time I've never seen an > Arm PMU produce a precise and constant period anyway, so this may be > more useful in the case the architecture is able to support precise > sampling. In any case it's is likely to be specific to a particular > workload / configuration anyway. > > The main downside I can see for making it configurable is that the > compiler cannot then optimise the get_random call as well as for a > constant, which may be undesirable on this code path. > > Hmmm I see, I didn't expect get_random_u32_below() to have such different implementations depending on whether it was a constant or not. You don't have to remove the constant though, it could be: get_random_u32() & (jitter_power_of_2_max - 1) If you're really worried about optimising this path, then generating the jitter with some rotate/xor/mask operation is probably much faster. I don't think the requirements are for it to actually be "random", but just something to perturb it, even a badly distributed random number would be fine. But also yes I don't have a particularly strong opinion either. Just that if someone does have a justification for a configurable value in the future, depending on how that's implemented it could make the new jitter boolean redundant which would be annoying. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel