All of lore.kernel.org
 help / color / mirror / Atom feed
From: Rob Herring <robh@kernel.org>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Ian Rogers <irogers@google.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>,
	Zachary.Leaf@arm.com, Raphael Gault <raphael.gault@arm.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Namhyung Kim <namhyung@kernel.org>,
	Itaru Kitayama <itaru.kitayama@gmail.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 04/10] libperf: Add evsel mmap support
Date: Wed, 31 Mar 2021 17:06:46 -0500	[thread overview]
Message-ID: <CAL_Jsq+c2suLcGUuXZ-a4UG-Bte-+uVfYjB-+BYXDbAYEf2Jyg@mail.gmail.com> (raw)
In-Reply-To: <YEuzFHyjubILDA5h@krava>

On Fri, Mar 12, 2021 at 12:29 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Mar 12, 2021 at 07:34:39AM -0700, Rob Herring wrote:
> > On Fri, Mar 12, 2021 at 6:59 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Wed, Mar 10, 2021 at 05:08:31PM -0700, Rob Herring wrote:
> > >
> > > SNIP
> > >
> > > > +
> > > >  static int
> > > >  sys_perf_event_open(struct perf_event_attr *attr,
> > > >                   pid_t pid, int cpu, int group_fd,
> > > > @@ -137,6 +147,8 @@ void perf_evsel__free_fd(struct perf_evsel *evsel)
> > > >  {
> > > >       xyarray__delete(evsel->fd);
> > > >       evsel->fd = NULL;
> > > > +     xyarray__delete(evsel->mmap);
> > > > +     evsel->mmap = NULL;
> > > >  }
> > > >
> > > >  void perf_evsel__close(struct perf_evsel *evsel)
> > > > @@ -156,6 +168,45 @@ void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu)
> > > >       perf_evsel__close_fd_cpu(evsel, cpu);
> > > >  }
> > > >
> > > > +int perf_evsel__mmap(struct perf_evsel *evsel, int pages)
> > > > +{
> > > > +     int ret, cpu, thread;
> > > > +     struct perf_mmap_param mp = {
> > > > +             .prot = PROT_READ | PROT_WRITE,
> > > > +             .mask = (pages * page_size) - 1,
> > > > +     };
> > >
> > > I don't mind using evsel->fd for dimensions below,
> > > but we need to check in here that it's defined,
> > > that perf_evsel__open was called
> >
> > Right, so I'll add this here:
> >
> > if (evsel->fd == NULL)
> >     return -EINVAL;

Actually, pretty much none of the API checks this.
perf_evsel__run_ioctl(), perf_evsel__enable(), perf_evsel__disable(),
perf_evsel__read() will all just deference evsel->fd. So fix all of
these or follow existing behavior?


> > Note that struct evsel has dimensions in it, but they are only set in
> > the evlist code. I couldn't tell if that was by design or mistake.
>
> we do? we have the cpus and threads in perf_evsel no?

Right, perf_evsel has cpus and threads pointers which in turn have the
sizes, but those pointers are not set within the evsel code.

> also you'd need perf_evsel not evsel, right?
>
> >
> > BTW, I just noticed perf_evsel__open is leaking memory on most of its
> > error paths.
>
> nice.. let's fix it ;-)

NM, I missed that they are static...

Rob

WARNING: multiple messages have this Message-ID
From: Rob Herring <robh@kernel.org>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	 Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	 Arnaldo Carvalho de Melo <acme@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Ian Rogers <irogers@google.com>,
	 Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	 Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>,
	Zachary.Leaf@arm.com,  Raphael Gault <raphael.gault@arm.com>,
	Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	 Namhyung Kim <namhyung@kernel.org>,
	Itaru Kitayama <itaru.kitayama@gmail.com>,
	 linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 04/10] libperf: Add evsel mmap support
Date: Wed, 31 Mar 2021 17:06:46 -0500	[thread overview]
Message-ID: <CAL_Jsq+c2suLcGUuXZ-a4UG-Bte-+uVfYjB-+BYXDbAYEf2Jyg@mail.gmail.com> (raw)
In-Reply-To: <YEuzFHyjubILDA5h@krava>

On Fri, Mar 12, 2021 at 12:29 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Fri, Mar 12, 2021 at 07:34:39AM -0700, Rob Herring wrote:
> > On Fri, Mar 12, 2021 at 6:59 AM Jiri Olsa <jolsa@redhat.com> wrote:
> > >
> > > On Wed, Mar 10, 2021 at 05:08:31PM -0700, Rob Herring wrote:
> > >
> > > SNIP
> > >
> > > > +
> > > >  static int
> > > >  sys_perf_event_open(struct perf_event_attr *attr,
> > > >                   pid_t pid, int cpu, int group_fd,
> > > > @@ -137,6 +147,8 @@ void perf_evsel__free_fd(struct perf_evsel *evsel)
> > > >  {
> > > >       xyarray__delete(evsel->fd);
> > > >       evsel->fd = NULL;
> > > > +     xyarray__delete(evsel->mmap);
> > > > +     evsel->mmap = NULL;
> > > >  }
> > > >
> > > >  void perf_evsel__close(struct perf_evsel *evsel)
> > > > @@ -156,6 +168,45 @@ void perf_evsel__close_cpu(struct perf_evsel *evsel, int cpu)
> > > >       perf_evsel__close_fd_cpu(evsel, cpu);
> > > >  }
> > > >
> > > > +int perf_evsel__mmap(struct perf_evsel *evsel, int pages)
> > > > +{
> > > > +     int ret, cpu, thread;
> > > > +     struct perf_mmap_param mp = {
> > > > +             .prot = PROT_READ | PROT_WRITE,
> > > > +             .mask = (pages * page_size) - 1,
> > > > +     };
> > >
> > > I don't mind using evsel->fd for dimensions below,
> > > but we need to check in here that it's defined,
> > > that perf_evsel__open was called
> >
> > Right, so I'll add this here:
> >
> > if (evsel->fd == NULL)
> >     return -EINVAL;

Actually, pretty much none of the API checks this.
perf_evsel__run_ioctl(), perf_evsel__enable(), perf_evsel__disable(),
perf_evsel__read() will all just deference evsel->fd. So fix all of
these or follow existing behavior?


> > Note that struct evsel has dimensions in it, but they are only set in
> > the evlist code. I couldn't tell if that was by design or mistake.
>
> we do? we have the cpus and threads in perf_evsel no?

Right, perf_evsel has cpus and threads pointers which in turn have the
sizes, but those pointers are not set within the evsel code.

> also you'd need perf_evsel not evsel, right?
>
> >
> > BTW, I just noticed perf_evsel__open is leaking memory on most of its
> > error paths.
>
> nice.. let's fix it ;-)

NM, I missed that they are static...

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2021-03-31 22:07 UTC|newest]

Thread overview: 70+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-11  0:08 [PATCH v6 00/10] libperf and arm64 userspace counter access support Rob Herring
2021-03-11  0:08 ` Rob Herring
2021-03-11  0:08 ` [PATCH v6 01/10] arm64: pmu: Add function implementation to update event index in userpage Rob Herring
2021-03-11  0:08   ` Rob Herring
2021-03-30 15:30   ` Will Deacon
2021-03-30 15:30     ` Will Deacon
2021-03-11  0:08 ` [PATCH v6 02/10] arm64: perf: Enable PMU counter direct access for perf event Rob Herring
2021-03-11  0:08   ` Rob Herring
2021-03-30 11:30   ` Zachary Leaf
2021-03-30 11:30     ` Zachary Leaf
2021-03-30 15:31   ` Will Deacon
2021-03-30 15:31     ` Will Deacon
2021-03-30 17:09     ` Rob Herring
2021-03-30 17:09       ` Rob Herring
2021-03-30 21:08       ` Rob Herring
2021-03-30 21:08         ` Rob Herring
2021-03-31 15:38         ` Will Deacon
2021-03-31 15:38           ` Will Deacon
2021-03-31 17:52           ` Rob Herring
2021-03-31 17:52             ` Rob Herring
2021-04-01  9:04             ` Will Deacon
2021-04-01  9:04               ` Will Deacon
2021-03-31 16:00       ` Will Deacon
2021-03-31 16:00         ` Will Deacon
2021-04-01 19:45         ` Rob Herring
2021-04-01 19:45           ` Rob Herring
2021-04-07 12:44           ` Will Deacon
2021-04-07 12:44             ` Will Deacon
2021-04-08 11:08             ` Mark Rutland
2021-04-08 11:08               ` Mark Rutland
2021-04-08 18:38               ` Rob Herring
2021-04-08 18:38                 ` Rob Herring
2021-04-19 16:14                 ` Will Deacon
2021-04-19 16:14                   ` Will Deacon
2021-04-19 19:00                   ` Rob Herring
2021-04-19 19:00                     ` Rob Herring
2021-03-11  0:08 ` [PATCH v6 03/10] tools/include: Add an initial math64.h Rob Herring
2021-03-11  0:08   ` Rob Herring
2021-03-11  0:08 ` [PATCH v6 04/10] libperf: Add evsel mmap support Rob Herring
2021-03-11  0:08   ` Rob Herring
2021-03-12 13:58   ` Jiri Olsa
2021-03-12 13:58     ` Jiri Olsa
2021-03-12 14:34     ` Rob Herring
2021-03-12 14:34       ` Rob Herring
2021-03-12 18:29       ` Jiri Olsa
2021-03-12 18:29         ` Jiri Olsa
2021-03-31 22:06         ` Rob Herring [this message]
2021-03-31 22:06           ` Rob Herring
2021-03-11  0:08 ` [PATCH v6 05/10] libperf: tests: Add support for verbose printing Rob Herring
2021-03-11  0:08   ` Rob Herring
2021-03-11  0:08 ` [PATCH v6 06/10] libperf: Add support for user space counter access Rob Herring
2021-03-11  0:08   ` Rob Herring
2021-05-04 21:40   ` Ian Rogers
2021-05-04 21:40     ` Ian Rogers
2021-05-05  2:12     ` Rob Herring
2021-05-05  2:12       ` Rob Herring
2021-03-11  0:08 ` [PATCH v6 07/10] libperf: Add arm64 support to perf_mmap__read_self() Rob Herring
2021-03-11  0:08   ` Rob Herring
2021-03-11  0:08 ` [PATCH v6 08/10] perf: arm64: Add test for userspace counter access on heterogeneous systems Rob Herring
2021-03-11  0:08   ` Rob Herring
2021-03-15 16:09   ` Masayoshi Mizuma
2021-03-15 16:09     ` Masayoshi Mizuma
2021-03-11  0:08 ` [PATCH v6 09/10] perf: arm64: Add tests for 32-bit and 64-bit counter size userspace access Rob Herring
2021-03-11  0:08   ` Rob Herring
2021-03-11  0:08 ` [PATCH v6 10/10] Documentation: arm64: Document PMU counters access from userspace Rob Herring
2021-03-11  0:08   ` Rob Herring
2021-03-31 16:00   ` Will Deacon
2021-03-31 16:00     ` Will Deacon
2021-03-30 11:31 ` [PATCH v6 00/10] libperf and arm64 userspace counter access support Zachary Leaf
2021-03-30 11:31   ` Zachary Leaf

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=CAL_Jsq+c2suLcGUuXZ-a4UG-Bte-+uVfYjB-+BYXDbAYEf2Jyg@mail.gmail.com \
    --to=robh@kernel.org \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=Zachary.Leaf@arm.com \
    --cc=acme@kernel.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=catalin.marinas@arm.com \
    --cc=honnappa.nagarahalli@arm.com \
    --cc=irogers@google.com \
    --cc=itaru.kitayama@gmail.com \
    --cc=jolsa@redhat.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=peterz@infradead.org \
    --cc=raphael.gault@arm.com \
    --cc=will@kernel.org \
    --subject='Re: [PATCH v6 04/10] libperf: Add evsel mmap support' \
    /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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.