From: James Clark <james.clark@arm.com>
To: Ben Gainey <Ben.Gainey@arm.com>
Cc: "acme@kernel.org" <acme@kernel.org>,
"alexander.shishkin@linux.intel.com"
<alexander.shishkin@linux.intel.com>,
"ak@linux.intel.com" <ak@linux.intel.com>,
"mingo@redhat.com" <mingo@redhat.com>,
"namhyung@kernel.org" <namhyung@kernel.org>,
Mark Rutland <Mark.Rutland@arm.com>,
"peterz@infradead.org" <peterz@infradead.org>,
"linux-perf-users@vger.kernel.org"
<linux-perf-users@vger.kernel.org>,
"adrian.hunter@intel.com" <adrian.hunter@intel.com>,
"will@kernel.org" <will@kernel.org>,
"irogers@google.com" <irogers@google.com>,
"jolsa@kernel.org" <jolsa@kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v2 2/4] perf: Allow adding fixed random jitter to the alternate sampling period
Date: Tue, 23 Apr 2024 10:55:32 +0100 [thread overview]
Message-ID: <e1bfb006-0226-4be4-9058-7c86da0e36e3@arm.com> (raw)
In-Reply-To: <50f8e635667f2c0b389f882bc3f881552ea77e68.camel@arm.com>
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 <ben.gainey@arm.com>
>>> ---
>>> 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 <linux/idr.h>
>>> #include <linux/file.h>
>>> #include <linux/poll.h>
>>> +#include <linux/random.h>
>>> #include <linux/slab.h>
>>> #include <linux/hash.h>
>>> #include <linux/tick.h>
>>> @@ -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.
next prev parent reply other threads:[~2024-04-23 9:55 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-22 10:49 [RFC PATCH v2 0/4] A mechanism for efficient support for per-function metrics Ben Gainey
2024-04-22 10:49 ` [RFC PATCH v2 1/4] perf: Allow periodic events to alternate between two sample periods Ben Gainey
2024-04-22 10:49 ` [RFC PATCH v2 2/4] perf: Allow adding fixed random jitter to the alternate sampling period Ben Gainey
2024-04-22 13:08 ` James Clark
2024-04-22 14:40 ` Ben Gainey
2024-04-23 9:55 ` James Clark [this message]
2024-04-22 10:49 ` [RFC PATCH v2 3/4] tools/perf: Modify event parser to support alt-period term Ben Gainey
2024-04-22 10:49 ` [RFC PATCH v2 4/4] tools/perf: Modify event parser to support alt-period-jitter term Ben Gainey
2024-04-23 15:42 ` [RFC PATCH v2 0/4] A mechanism for efficient support for per-function metrics Andi Kleen
2024-04-26 11:11 ` Ben Gainey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e1bfb006-0226-4be4-9058-7c86da0e36e3@arm.com \
--to=james.clark@arm.com \
--cc=Ben.Gainey@arm.com \
--cc=Mark.Rutland@arm.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).