Re: [PATCH v7 01/13] tools/libperf: introduce notion of static polled file descriptors

From: Jiri Olsa
Date: Mon Jun 15 2020 - 12:58:25 EST

On Mon, Jun 15, 2020 at 05:37:53PM +0300, Alexey Budankov wrote:
>
> On 15.06.2020 15:30, Jiri Olsa wrote:
> > On Mon, Jun 15, 2020 at 08:20:38AM +0300, Alexey Budankov wrote:
> >>
> >> On 08.06.2020 19:07, Jiri Olsa wrote:
> >>> On Mon, Jun 08, 2020 at 12:54:31PM +0300, Alexey Budankov wrote:
> >>>>
> >>>> On 08.06.2020 11:43, Jiri Olsa wrote:
> >>>>> On Mon, Jun 08, 2020 at 11:08:56AM +0300, Alexey Budankov wrote:
> >>>>>>
> >>>>>> On 05.06.2020 19:15, Alexey Budankov wrote:
> >>>>>>>
> >>>>>>> On 05.06.2020 14:38, Jiri Olsa wrote:
> >> <SNIP>
> >>>>>>> revents = fdarray_fixed_revents(array, pos);
> >>>>>>> fdarray__del(array, pos);
> >>>>>>
> >>>>>> So how is it about just adding _revents() and _del() for fixed fds with
> >>>>>> correction of retval to bool for fdarray__add()?
> >>>>>
> >>>>> I don't like the separation for fixed and non-fixed fds,
> >>>>> why can't we make generic?
> >>>>
> >>>> Usage models are different but they want still to be parts of the same class
> >>>> for atomic poll(). The distinction is filterable vs. not filterable.
> >>>> The distinction should be somehow provided in API. Options are:
> >>>> 1. expose separate API calls like __add_nonfilterable(), __del_nonfilterable();
> >>>> use nonfilterable quality in __filter() and __poll() and, perhaps, other internals;
> >>>> 2. extend fdarray__add(, nonfilterable) with the nonfilterable quality
> >>>> use the type in __filter() and __poll() and, perhaps, other internals;
> >>>> expose less API calls in comparison with option 1
> >>>>
> >>>> Exposure of pos for filterable fds should be converted to bool since currently
> >>>> the returned pos can become stale and there is no way in API to check its state.
> >>>> So it could look like this:
> >>>>
> >>>> fdkey = fdarray__add(array, fd, events, type)
> >>>> type: filterable, nonfilterable, somthing else
> >>>> revents = fdarray__get_revents(fdkey);
> >>>> fdarray__del(array, fdkey);
> >>>
> >>> I think there's solution without having filterable type,
> >>> I'm not sure why you think this is needed
> >>>
> >>> I'm busy with other things this week, but I think I can
> >>> come up with some patch early next week if needed
> >>
> >> Friendly reminder.
> >
> > hm? I believe we discussed this in here:
> > 20200609145611.GI1558310@krava/">https://lore.kernel.org/lkml/20200609145611.GI1558310@krava/
>
> Do you want it to be implemented like in the patch posted by the link?

no idea.. looking for good solution ;-)

how about switching completely to epoll? I tried and it
does not look that bad

there might be some loose ends (interface change), but
I think this would solve our problems with fdarray

I'll be able to get back to it by the end of the week,
but if you want to check/finish this patch earlier go ahead

jirka


---
tools/lib/perf/evlist.c | 134 +++++++++++++++++------
tools/lib/perf/include/internal/evlist.h | 9 +-
tools/perf/builtin-kvm.c | 8 +-
tools/perf/builtin-record.c | 14 ++-
4 files changed, 120 insertions(+), 45 deletions(-)

diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
index 6a875a0f01bb..8569cdd8bbd8 100644
--- a/tools/lib/perf/evlist.c
+++ b/tools/lib/perf/evlist.c
@@ -23,6 +23,7 @@
#include <perf/cpumap.h>
#include <perf/threadmap.h>
#include <api/fd/array.h>
+#include <sys/epoll.h>

void perf_evlist__init(struct perf_evlist *evlist)
{
@@ -32,7 +33,10 @@ void perf_evlist__init(struct perf_evlist *evlist)
INIT_HLIST_HEAD(&evlist->heads[i]);
INIT_LIST_HEAD(&evlist->entries);
evlist->nr_entries = 0;
- fdarray__init(&evlist->pollfd, 64);
+ INIT_LIST_HEAD(&evlist->poll_data);
+ evlist->poll_cnt = 0;
+ evlist->poll_act = 0;
+ evlist->poll_fd = -1;
}

static void __perf_evlist__propagate_maps(struct perf_evlist *evlist,
@@ -120,6 +124,23 @@ static void perf_evlist__purge(struct perf_evlist *evlist)
evlist->nr_entries = 0;
}

+struct poll_data {
+ int fd;
+ void *ptr;
+ struct list_head list;
+};
+
+static void perf_evlist__exit_pollfd(struct perf_evlist *evlist)
+{
+ struct poll_data *data, *tmp;
+
+ if (evlist->poll_fd != -1)
+ close(evlist->poll_fd);
+
+ list_for_each_entry_safe(data, tmp, &evlist->poll_data, list)
+ free(data);
+}
+
void perf_evlist__exit(struct perf_evlist *evlist)
{
perf_cpu_map__put(evlist->cpus);
@@ -128,7 +149,7 @@ void perf_evlist__exit(struct perf_evlist *evlist)
evlist->cpus = NULL;
evlist->all_cpus = NULL;
evlist->threads = NULL;
- fdarray__exit(&evlist->pollfd);
+ perf_evlist__exit_pollfd(evlist);
}

void perf_evlist__delete(struct perf_evlist *evlist)
@@ -285,56 +306,105 @@ int perf_evlist__id_add_fd(struct perf_evlist *evlist,

int perf_evlist__alloc_pollfd(struct perf_evlist *evlist)
{
- int nr_cpus = perf_cpu_map__nr(evlist->cpus);
- int nr_threads = perf_thread_map__nr(evlist->threads);
- int nfds = 0;
- struct perf_evsel *evsel;
-
- perf_evlist__for_each_entry(evlist, evsel) {
- if (evsel->system_wide)
- nfds += nr_cpus;
- else
- nfds += nr_cpus * nr_threads;
- }
+ int poll_fd;

- if (fdarray__available_entries(&evlist->pollfd) < nfds &&
- fdarray__grow(&evlist->pollfd, nfds) < 0)
- return -ENOMEM;
+ poll_fd = epoll_create1(EPOLL_CLOEXEC);
+ if (!poll_fd)
+ return -1;

+ evlist->poll_fd = poll_fd;
return 0;
}

-int perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd,
- void *ptr, short revent)
+static int __perf_evlist__add_pollfd(struct perf_evlist *evlist,
+ struct poll_data *data,
+ short revent)
{
- int pos = fdarray__add(&evlist->pollfd, fd, revent | POLLERR | POLLHUP);
+ struct epoll_event *events, ev = {
+ .data.ptr = data,
+ .events = revent | EPOLLERR | EPOLLHUP,
+ };
+ int err;
+
+ err = epoll_ctl(evlist->poll_fd, EPOLL_CTL_ADD, data->fd, &ev);
+ if (err)
+ return err;

- if (pos >= 0) {
- evlist->pollfd.priv[pos].ptr = ptr;
- fcntl(fd, F_SETFL, O_NONBLOCK);
+ events = realloc(evlist->poll_events, sizeof(ev) * evlist->poll_cnt);
+ if (events) {
+ evlist->poll_events = events;
+ evlist->poll_cnt++;
}

- return pos;
+ return events ? 0 : -ENOMEM;
}

-static void perf_evlist__munmap_filtered(struct fdarray *fda, int fd,
- void *arg __maybe_unused)
+int perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd,
+ void *ptr, short revent)
{
- struct perf_mmap *map = fda->priv[fd].ptr;
+ struct poll_data *data = zalloc(sizeof(*data));
+ int err;

- if (map)
- perf_mmap__put(map);
+ if (!data)
+ return -ENOMEM;
+
+ data->fd = fd;
+ data->ptr = ptr;
+
+ err = __perf_evlist__add_pollfd(evlist, data, revent);
+ if (!err)
+ list_add_tail(&data->list, &evlist->poll_data);
+
+ return err;
}

int perf_evlist__filter_pollfd(struct perf_evlist *evlist, short revents_and_mask)
{
- return fdarray__filter(&evlist->pollfd, revents_and_mask,
- perf_evlist__munmap_filtered, NULL);
+ struct epoll_event *events = evlist->poll_events;
+ int i, removed = 0;
+
+ for (i = 0; i < evlist->poll_act; i++) {
+ if (events[i].events & revents_and_mask) {
+ struct poll_data *data = events[i].data.ptr;
+
+ if (data->ptr)
+ perf_mmap__put(data->ptr);
+
+ epoll_ctl(evlist->poll_fd, EPOLL_CTL_DEL, data->fd, &events[i]);
+
+ list_del(&data->list);
+ free(data);
+ removed++;
+ }
+ }
+
+ return evlist->poll_cnt -= removed;
+}
+
+bool perf_evlist__pollfd_data(struct perf_evlist *evlist, int fd)
+{
+ int i;
+
+ if (evlist->poll_act < 0)
+ return false;
+
+ for (i = 0; i < evlist->poll_act; i++) {
+ struct poll_data *data = evlist->poll_events[i].data.ptr;
+
+ if (data->fd == fd)
+ return true;
+ }
+
+ return false;
}

int perf_evlist__poll(struct perf_evlist *evlist, int timeout)
{
- return fdarray__poll(&evlist->pollfd, timeout);
+ evlist->poll_act = epoll_wait(evlist->poll_fd,
+ evlist->poll_events,
+ evlist->poll_cnt,
+ timeout);
+ return evlist->poll_act;
}

static struct perf_mmap* perf_evlist__alloc_mmap(struct perf_evlist *evlist, bool overwrite)
@@ -593,7 +663,7 @@ int perf_evlist__mmap_ops(struct perf_evlist *evlist,
return -ENOMEM;
}

- if (evlist->pollfd.entries == NULL && perf_evlist__alloc_pollfd(evlist) < 0)
+ if (evlist->poll_fd == -1 && perf_evlist__alloc_pollfd(evlist) < 0)
return -ENOMEM;

if (perf_cpu_map__empty(cpus))
diff --git a/tools/lib/perf/include/internal/evlist.h b/tools/lib/perf/include/internal/evlist.h
index 74dc8c3f0b66..39b08a04b992 100644
--- a/tools/lib/perf/include/internal/evlist.h
+++ b/tools/lib/perf/include/internal/evlist.h
@@ -3,7 +3,6 @@
#define __LIBPERF_INTERNAL_EVLIST_H

#include <linux/list.h>
-#include <api/fd/array.h>
#include <internal/evsel.h>

#define PERF_EVLIST__HLIST_BITS 8
@@ -12,6 +11,7 @@
struct perf_cpu_map;
struct perf_thread_map;
struct perf_mmap_param;
+struct epoll_event;

struct perf_evlist {
struct list_head entries;
@@ -22,7 +22,11 @@ struct perf_evlist {
struct perf_thread_map *threads;
int nr_mmaps;
size_t mmap_len;
- struct fdarray pollfd;
+ int poll_fd;
+ int poll_cnt;
+ int poll_act;
+ struct epoll_event *poll_events;
+ struct list_head poll_data;
struct hlist_head heads[PERF_EVLIST__HLIST_SIZE];
struct perf_mmap *mmap;
struct perf_mmap *mmap_ovw;
@@ -124,4 +128,5 @@ int perf_evlist__id_add_fd(struct perf_evlist *evlist,
struct perf_evsel *evsel,
int cpu, int thread, int fd);

+bool perf_evlist__pollfd_data(struct perf_evlist *evlist, int fd);
#endif /* __LIBPERF_INTERNAL_EVLIST_H */
diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
index 95a77058023e..decc75745395 100644
--- a/tools/perf/builtin-kvm.c
+++ b/tools/perf/builtin-kvm.c
@@ -940,7 +940,7 @@ static int perf_kvm__handle_stdin(void)

static int kvm_events_live_report(struct perf_kvm_stat *kvm)
{
- int nr_stdin, ret, err = -EINVAL;
+ int ret, err = -EINVAL;
struct termios save;

/* live flag must be set first */
@@ -971,8 +971,7 @@ static int kvm_events_live_report(struct perf_kvm_stat *kvm)
if (evlist__add_pollfd(kvm->evlist, kvm->timerfd) < 0)
goto out;

- nr_stdin = evlist__add_pollfd(kvm->evlist, fileno(stdin));
- if (nr_stdin < 0)
+ if (evlist__add_pollfd(kvm->evlist, fileno(stdin)))
goto out;

if (fd_set_nonblock(fileno(stdin)) != 0)
@@ -982,7 +981,6 @@ static int kvm_events_live_report(struct perf_kvm_stat *kvm)
evlist__enable(kvm->evlist);

while (!done) {
- struct fdarray *fda = &kvm->evlist->core.pollfd;
int rc;

rc = perf_kvm__mmap_read(kvm);
@@ -993,7 +991,7 @@ static int kvm_events_live_report(struct perf_kvm_stat *kvm)
if (err)
goto out;

- if (fda->entries[nr_stdin].revents & POLLIN)
+ if (perf_evlist__pollfd_data(&kvm->evlist->core, fileno(stdin)))
done = perf_kvm__handle_stdin();

if (!rc && !done)
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index e108d90ae2ed..a49bf4186aab 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1576,12 +1576,6 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
status = -1;
goto out_delete_session;
}
- err = evlist__add_pollfd(rec->evlist, done_fd);
- if (err < 0) {
- pr_err("Failed to add wakeup eventfd to poll list\n");
- status = err;
- goto out_delete_session;
- }
#endif // HAVE_EVENTFD_SUPPORT

session->header.env.comp_type = PERF_COMP_ZSTD;
@@ -1624,6 +1618,14 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
}
session->header.env.comp_mmap_len = session->evlist->core.mmap_len;

+#ifdef HAVE_EVENTFD_SUPPORT
+ err = evlist__add_pollfd(rec->evlist, done_fd);
+ if (err < 0) {
+ pr_err("Failed to add wakeup eventfd to poll list\n");
+ goto out_child;
+ }
+#endif // HAVE_EVENTFD_SUPPORT
+
if (rec->opts.kcore) {
err = record__kcore_copy(&session->machines.host, data);
if (err) {
--
2.25.4