Re: [PATCH v3 3/4] perf record: Don't clear event's period if set by a term

From: Ian Rogers <irogers@google.com>
To: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andriin@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Andi Kleen <ak@linux.intel.com>,
	Athira Rajeev <atrajeev@linux.vnet.ibm.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	Stephane Eranian <eranian@google.com>
Subject: Re: [PATCH v3 3/4] perf record: Don't clear event's period if set by a term
Date: Mon, 14 Sep 2020 14:52:57 -0700	[thread overview]
Message-ID: <CAP-5=fUO_HFd2-z53u6GdRV=o7HsB4ThzWYJDGQG8OwGDeV+VA@mail.gmail.com> (raw)
In-Reply-To: <20200914215106.GF166601@kernel.org>

On Mon, Sep 14, 2020 at 2:51 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Mon, Sep 14, 2020 at 06:46:55PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Fri, Sep 11, 2020 at 07:56:54PM -0700, Ian Rogers escreveu:
> > > If events in a group explicitly set a frequency or period with leader
> > > sampling, don't disable the samples on those events.
> > >
> > > Prior to 5.8:
> > > perf record -e '{cycles/period=12345000/,instructions/period=6789000/}:S'
> > > would clear the attributes then apply the config terms. In commit
> > > 5f34278867b7 leader sampling configuration was moved to after applying the
> > > config terms, in the example, making the instructions' event have its period
> > > cleared.
> > > This change makes it so that sampling is only disabled if configuration
> > > terms aren't present.
> >
> > Adrian, Jiri, can you please take a look a this and provide Reviewed-by
> > or Acked-by tags?
>
> Without this patch we have:
>
> # perf record -e '{cycles/period=1/,instructions/period=2/}:S' sleep 1
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.051 MB perf.data (6 samples) ]
> #
> # perf evlist -v
> cycles/period=1/: size: 120, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|READ|ID, read_format: ID|GROUP, disabled: 1, mmap: 1, comm: 1, enable_on_exec: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, ksymbol: 1, bpf_event: 1
> instructions/period=2/: size: 120, config: 0x1, sample_type: IP|TID|TIME|READ|ID, read_format: ID|GROUP, sample_id_all: 1, exclude_guest: 1
> #
>
> So indeed the period=2 is being cleared for the second event in that
> group.
>
> - Arnaldo

Thanks Arnaldo and Adrian! Adrian's acked-by is here:
https://lore.kernel.org/lkml/77df85d3-a50c-d6aa-1d60-4fc9ea90dc44@intel.com/
Let me know if anything is missing.

Ian