[v7,01/13] tools/libperf: introduce notion of static polled file descriptors
diff mbox series

Message ID 3c92a0ad-d7d3-4e78-f0b8-1d3a7122c69e@linux.intel.com
State New
Headers show
Series
  • perf: support enable and disable commands in stat and record modes
Related show

Commit Message

Alexey Budankov June 3, 2020, 3:52 p.m. UTC
Implement adding of file descriptors by fdarray__add_stat() to
fix-sized (currently 1) stat_entries array located at struct fdarray.
Append added file descriptors to the array used by poll() syscall
during fdarray__poll() call. Copy poll() result of the added
descriptors from the array back to the storage for analysis.

Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
---
 tools/lib/api/fd/array.c                 | 42 +++++++++++++++++++++++-
 tools/lib/api/fd/array.h                 |  7 ++++
 tools/lib/perf/evlist.c                  | 11 +++++++
 tools/lib/perf/include/internal/evlist.h |  2 ++
 4 files changed, 61 insertions(+), 1 deletion(-)

Comments

Jiri Olsa June 5, 2020, 10:50 a.m. UTC | #1
On Wed, Jun 03, 2020 at 06:52:59PM +0300, Alexey Budankov wrote:
> 
> Implement adding of file descriptors by fdarray__add_stat() to
> fix-sized (currently 1) stat_entries array located at struct fdarray.
> Append added file descriptors to the array used by poll() syscall
> during fdarray__poll() call. Copy poll() result of the added
> descriptors from the array back to the storage for analysis.
> 
> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> ---
>  tools/lib/api/fd/array.c                 | 42 +++++++++++++++++++++++-
>  tools/lib/api/fd/array.h                 |  7 ++++
>  tools/lib/perf/evlist.c                  | 11 +++++++
>  tools/lib/perf/include/internal/evlist.h |  2 ++
>  4 files changed, 61 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> index 58d44d5eee31..b0027f2169c7 100644
> --- a/tools/lib/api/fd/array.c
> +++ b/tools/lib/api/fd/array.c
> @@ -11,10 +11,16 @@
>  
>  void fdarray__init(struct fdarray *fda, int nr_autogrow)
>  {
> +	int i;
> +
>  	fda->entries	 = NULL;
>  	fda->priv	 = NULL;
>  	fda->nr		 = fda->nr_alloc = 0;
>  	fda->nr_autogrow = nr_autogrow;
> +
> +	fda->nr_stat = 0;
> +	for (i = 0; i < FDARRAY__STAT_ENTRIES_MAX; i++)
> +		fda->stat_entries[i].fd = -1;
>  }
>  
>  int fdarray__grow(struct fdarray *fda, int nr)
> @@ -83,6 +89,20 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
>  	return pos;
>  }
>  
> +int fdarray__add_stat(struct fdarray *fda, int fd, short revents)
> +{
> +	int pos = fda->nr_stat;
> +
> +	if (pos >= FDARRAY__STAT_ENTRIES_MAX)
> +		return -1;
> +
> +	fda->stat_entries[pos].fd = fd;
> +	fda->stat_entries[pos].events = revents;
> +	fda->nr_stat++;
> +
> +	return pos;
> +}
> +
>  int fdarray__filter(struct fdarray *fda, short revents,
>  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
>  		    void *arg)
> @@ -113,7 +133,27 @@ int fdarray__filter(struct fdarray *fda, short revents,
>  
>  int fdarray__poll(struct fdarray *fda, int timeout)
>  {
> -	return poll(fda->entries, fda->nr, timeout);
> +	int nr, i, pos, res;
> +
> +	nr = fda->nr;
> +
> +	for (i = 0; i < fda->nr_stat; i++) {
> +		if (fda->stat_entries[i].fd != -1) {
> +			pos = fdarray__add(fda, fda->stat_entries[i].fd,
> +					   fda->stat_entries[i].events);

so every call to fdarray__poll will add whatever is
in stat_entries to entries? how is it removed?

I think you should either follow what Adrian said
and put 'static' descriptors early and check for
filter number to match it as an 'quick fix'

or we should fix it for real and make it generic

so currently the interface is like this:

  pos1 = fdarray__add(a, fd1 ... );
  pos2 = fdarray__add(a, fd2 ... );
  pos3 = fdarray__add(a, fd2 ... );

  fdarray__poll(a);

  num = fdarray__filter(a, revents, destructor, arg);

when fdarray__filter removes some of the fds the 'pos1,pos2,pos3'
indexes are not relevant anymore

how about we make the 'pos indexes' being stable by allocating
separate object for each added descriptor and each poll call
would create pollfd array from current objects, and entries
would keep pointer to its pollfd entry

  struct fdentry *entry {
       int              fd;
       int              events;
       struct pollfd   *pollfd;
  }

  entry1 = fdarray__add(a, fd1 ...);
  entry2 = fdarray__add(a, fd2 ...);
  entry3 = fdarray__add(a, fd3 ...);

  fdarray__poll(a);

  struct pollfd *fdarray__entry_pollfd(a, entry1);

or smoething like that ;-)

jirka
Jiri Olsa June 5, 2020, 11:38 a.m. UTC | #2
On Fri, Jun 05, 2020 at 12:50:54PM +0200, Jiri Olsa wrote:
> On Wed, Jun 03, 2020 at 06:52:59PM +0300, Alexey Budankov wrote:
> > 
> > Implement adding of file descriptors by fdarray__add_stat() to
> > fix-sized (currently 1) stat_entries array located at struct fdarray.
> > Append added file descriptors to the array used by poll() syscall
> > during fdarray__poll() call. Copy poll() result of the added
> > descriptors from the array back to the storage for analysis.
> > 
> > Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> > ---
> >  tools/lib/api/fd/array.c                 | 42 +++++++++++++++++++++++-
> >  tools/lib/api/fd/array.h                 |  7 ++++
> >  tools/lib/perf/evlist.c                  | 11 +++++++
> >  tools/lib/perf/include/internal/evlist.h |  2 ++
> >  4 files changed, 61 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> > index 58d44d5eee31..b0027f2169c7 100644
> > --- a/tools/lib/api/fd/array.c
> > +++ b/tools/lib/api/fd/array.c
> > @@ -11,10 +11,16 @@
> >  
> >  void fdarray__init(struct fdarray *fda, int nr_autogrow)
> >  {
> > +	int i;
> > +
> >  	fda->entries	 = NULL;
> >  	fda->priv	 = NULL;
> >  	fda->nr		 = fda->nr_alloc = 0;
> >  	fda->nr_autogrow = nr_autogrow;
> > +
> > +	fda->nr_stat = 0;
> > +	for (i = 0; i < FDARRAY__STAT_ENTRIES_MAX; i++)
> > +		fda->stat_entries[i].fd = -1;
> >  }
> >  
> >  int fdarray__grow(struct fdarray *fda, int nr)
> > @@ -83,6 +89,20 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
> >  	return pos;
> >  }
> >  
> > +int fdarray__add_stat(struct fdarray *fda, int fd, short revents)
> > +{
> > +	int pos = fda->nr_stat;
> > +
> > +	if (pos >= FDARRAY__STAT_ENTRIES_MAX)
> > +		return -1;
> > +
> > +	fda->stat_entries[pos].fd = fd;
> > +	fda->stat_entries[pos].events = revents;
> > +	fda->nr_stat++;
> > +
> > +	return pos;
> > +}
> > +
> >  int fdarray__filter(struct fdarray *fda, short revents,
> >  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
> >  		    void *arg)
> > @@ -113,7 +133,27 @@ int fdarray__filter(struct fdarray *fda, short revents,
> >  
> >  int fdarray__poll(struct fdarray *fda, int timeout)
> >  {
> > -	return poll(fda->entries, fda->nr, timeout);
> > +	int nr, i, pos, res;
> > +
> > +	nr = fda->nr;
> > +
> > +	for (i = 0; i < fda->nr_stat; i++) {
> > +		if (fda->stat_entries[i].fd != -1) {
> > +			pos = fdarray__add(fda, fda->stat_entries[i].fd,
> > +					   fda->stat_entries[i].events);
> 
> so every call to fdarray__poll will add whatever is
> in stat_entries to entries? how is it removed?
> 
> I think you should either follow what Adrian said
> and put 'static' descriptors early and check for
> filter number to match it as an 'quick fix'
> 
> or we should fix it for real and make it generic
> 
> so currently the interface is like this:
> 
>   pos1 = fdarray__add(a, fd1 ... );
>   pos2 = fdarray__add(a, fd2 ... );
>   pos3 = fdarray__add(a, fd2 ... );
> 
>   fdarray__poll(a);
> 
>   num = fdarray__filter(a, revents, destructor, arg);
> 
> when fdarray__filter removes some of the fds the 'pos1,pos2,pos3'
> indexes are not relevant anymore
> 
> how about we make the 'pos indexes' being stable by allocating
> separate object for each added descriptor and each poll call
> would create pollfd array from current objects, and entries
> would keep pointer to its pollfd entry
> 
>   struct fdentry *entry {
>        int              fd;
>        int              events;
>        struct pollfd   *pollfd;
>   }
> 
>   entry1 = fdarray__add(a, fd1 ...);
>   entry2 = fdarray__add(a, fd2 ...);
>   entry3 = fdarray__add(a, fd3 ...);
> 
>   fdarray__poll(a);
> 
>   struct pollfd *fdarray__entry_pollfd(a, entry1);
> 
> or smoething like that ;-)

maybe something like below (only compile tested)

jirka


---
diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
index 58d44d5eee31..f1effed3dde1 100644
--- a/tools/lib/api/fd/array.c
+++ b/tools/lib/api/fd/array.c
@@ -22,8 +22,8 @@ int fdarray__grow(struct fdarray *fda, int nr)
 	void *priv;
 	int nr_alloc = fda->nr_alloc + nr;
 	size_t psize = sizeof(fda->priv[0]) * nr_alloc;
-	size_t size  = sizeof(struct pollfd) * nr_alloc;
-	struct pollfd *entries = realloc(fda->entries, size);
+	size_t size  = sizeof(struct fdentry *) * nr_alloc;
+	struct fdentry **entries = realloc(fda->entries, size);
 
 	if (entries == NULL)
 		return -ENOMEM;
@@ -58,7 +58,12 @@ struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow)
 
 void fdarray__exit(struct fdarray *fda)
 {
+	int i;
+
+	for (i = 0; i < fda->nr; i++)
+		free(fda->entries[i]);
 	free(fda->entries);
+	free(fda->pollfd);
 	free(fda->priv);
 	fdarray__init(fda, 0);
 }
@@ -69,18 +74,25 @@ void fdarray__delete(struct fdarray *fda)
 	free(fda);
 }
 
-int fdarray__add(struct fdarray *fda, int fd, short revents)
+struct fdentry *fdarray__add(struct fdarray *fda, int fd, short revents)
 {
-	int pos = fda->nr;
+	struct fdentry *entry;
 
 	if (fda->nr == fda->nr_alloc &&
 	    fdarray__grow(fda, fda->nr_autogrow) < 0)
-		return -ENOMEM;
+		return NULL;
+
+	entry = malloc(sizeof(*entry));
+	if (!entry)
+		return NULL;
+
+	entry->fd = fd;
+	entry->revents = revents;
+	entry->pollfd = NULL;
 
-	fda->entries[fda->nr].fd     = fd;
-	fda->entries[fda->nr].events = revents;
+	fda->entries[fda->nr] = entry;
 	fda->nr++;
-	return pos;
+	return entry;
 }
 
 int fdarray__filter(struct fdarray *fda, short revents,
@@ -93,7 +105,7 @@ int fdarray__filter(struct fdarray *fda, short revents,
 		return 0;
 
 	for (fd = 0; fd < fda->nr; ++fd) {
-		if (fda->entries[fd].revents & revents) {
+		if (fda->entries[fd]->revents & revents) {
 			if (entry_destructor)
 				entry_destructor(fda, fd, arg);
 
@@ -113,7 +125,22 @@ int fdarray__filter(struct fdarray *fda, short revents,
 
 int fdarray__poll(struct fdarray *fda, int timeout)
 {
-	return poll(fda->entries, fda->nr, timeout);
+	struct pollfd *pollfd = fda->pollfd;
+	int i;
+
+	pollfd = realloc(pollfd, sizeof(*pollfd) * fda->nr);
+	if (!pollfd)
+		return -ENOMEM;
+
+	fda->pollfd = pollfd;
+
+	for (i = 0; i < fda->nr; i++) {
+		pollfd[i].fd = fda->entries[i]->fd;
+		pollfd[i].revents = fda->entries[i]->revents;
+		fda->entries[i]->pollfd = &pollfd[i];
+	}
+
+	return poll(pollfd, fda->nr, timeout);
 }
 
 int fdarray__fprintf(struct fdarray *fda, FILE *fp)
@@ -121,7 +148,12 @@ int fdarray__fprintf(struct fdarray *fda, FILE *fp)
 	int fd, printed = fprintf(fp, "%d [ ", fda->nr);
 
 	for (fd = 0; fd < fda->nr; ++fd)
-		printed += fprintf(fp, "%s%d", fd ? ", " : "", fda->entries[fd].fd);
+		printed += fprintf(fp, "%s%d", fd ? ", " : "", fda->entries[fd]->fd);
 
 	return printed + fprintf(fp, " ]");
 }
+
+int fdentry__events(struct fdentry *entry)
+{
+	return entry->pollfd->revents;
+}
diff --git a/tools/lib/api/fd/array.h b/tools/lib/api/fd/array.h
index b39557d1a88f..5231ce047f2e 100644
--- a/tools/lib/api/fd/array.h
+++ b/tools/lib/api/fd/array.h
@@ -6,6 +6,12 @@
 
 struct pollfd;
 
+struct fdentry {
+	int		 fd;
+	int		 revents;
+	struct pollfd	*pollfd;
+};
+
 /**
  * struct fdarray: Array of file descriptors
  *
@@ -20,7 +26,10 @@ struct fdarray {
 	int	       nr;
 	int	       nr_alloc;
 	int	       nr_autogrow;
-	struct pollfd *entries;
+
+	struct fdentry	**entries;
+	struct pollfd	 *pollfd;
+
 	union {
 		int    idx;
 		void   *ptr;
@@ -33,7 +42,7 @@ void fdarray__exit(struct fdarray *fda);
 struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow);
 void fdarray__delete(struct fdarray *fda);
 
-int fdarray__add(struct fdarray *fda, int fd, short revents);
+struct fdentry *fdarray__add(struct fdarray *fda, int fd, short revents);
 int fdarray__poll(struct fdarray *fda, int timeout);
 int fdarray__filter(struct fdarray *fda, short revents,
 		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
@@ -41,6 +50,8 @@ int fdarray__filter(struct fdarray *fda, short revents,
 int fdarray__grow(struct fdarray *fda, int extra);
 int fdarray__fprintf(struct fdarray *fda, FILE *fp);
 
+int fdentry__events(struct fdentry *entry);
+
 static inline int fdarray__available_entries(struct fdarray *fda)
 {
 	return fda->nr_alloc - fda->nr;
Alexey Budankov June 5, 2020, 11:50 a.m. UTC | #3
On 05.06.2020 13:50, Jiri Olsa wrote:
> On Wed, Jun 03, 2020 at 06:52:59PM +0300, Alexey Budankov wrote:
>>
>> Implement adding of file descriptors by fdarray__add_stat() to
>> fix-sized (currently 1) stat_entries array located at struct fdarray.
>> Append added file descriptors to the array used by poll() syscall
>> during fdarray__poll() call. Copy poll() result of the added
>> descriptors from the array back to the storage for analysis.
>>
>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>> ---
>>  tools/lib/api/fd/array.c                 | 42 +++++++++++++++++++++++-
>>  tools/lib/api/fd/array.h                 |  7 ++++
>>  tools/lib/perf/evlist.c                  | 11 +++++++
>>  tools/lib/perf/include/internal/evlist.h |  2 ++
>>  4 files changed, 61 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
>> index 58d44d5eee31..b0027f2169c7 100644
>> --- a/tools/lib/api/fd/array.c
>> +++ b/tools/lib/api/fd/array.c
>> @@ -11,10 +11,16 @@
>>  
>>  void fdarray__init(struct fdarray *fda, int nr_autogrow)
>>  {
>> +	int i;
>> +
>>  	fda->entries	 = NULL;
>>  	fda->priv	 = NULL;
>>  	fda->nr		 = fda->nr_alloc = 0;
>>  	fda->nr_autogrow = nr_autogrow;
>> +
>> +	fda->nr_stat = 0;
>> +	for (i = 0; i < FDARRAY__STAT_ENTRIES_MAX; i++)
>> +		fda->stat_entries[i].fd = -1;
>>  }
>>  
>>  int fdarray__grow(struct fdarray *fda, int nr)
>> @@ -83,6 +89,20 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
>>  	return pos;
>>  }
>>  
>> +int fdarray__add_stat(struct fdarray *fda, int fd, short revents)
>> +{
>> +	int pos = fda->nr_stat;
>> +
>> +	if (pos >= FDARRAY__STAT_ENTRIES_MAX)
>> +		return -1;
>> +
>> +	fda->stat_entries[pos].fd = fd;
>> +	fda->stat_entries[pos].events = revents;
>> +	fda->nr_stat++;
>> +
>> +	return pos;
>> +}
>> +
>>  int fdarray__filter(struct fdarray *fda, short revents,
>>  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
>>  		    void *arg)
>> @@ -113,7 +133,27 @@ int fdarray__filter(struct fdarray *fda, short revents,
>>  
>>  int fdarray__poll(struct fdarray *fda, int timeout)
>>  {
>> -	return poll(fda->entries, fda->nr, timeout);
>> +	int nr, i, pos, res;
>> +
>> +	nr = fda->nr;
>> +
>> +	for (i = 0; i < fda->nr_stat; i++) {
>> +		if (fda->stat_entries[i].fd != -1) {
>> +			pos = fdarray__add(fda, fda->stat_entries[i].fd,
>> +					   fda->stat_entries[i].events);
> 
> so every call to fdarray__poll will add whatever is
> in stat_entries to entries? how is it removed?

Whatever stat fd which is not -1 is added. If static fd is -1 then it is skipped for
poll() call. Complete stat slot (fd == -1) isn't expected to be reused but reuse could
be supported by simple change at fdarray__add_stat()
and probably bring required generality.

> 
> I think you should either follow what Adrian said
> and put 'static' descriptors early and check for
> filter number to match it as an 'quick fix'
> 
> or we should fix it for real and make it generic

It would complicate without a reason. If it really matters I would add possibility of
realloc of stat entries in fdarray__add_stat() and that would make stat entries usage
more similar to filterable ones without dramatic change and risk of regressions.

~Alexey

> 
> so currently the interface is like this:
> 
>   pos1 = fdarray__add(a, fd1 ... );
>   pos2 = fdarray__add(a, fd2 ... );
>   pos3 = fdarray__add(a, fd2 ... );
> 
>   fdarray__poll(a);
> 
>   num = fdarray__filter(a, revents, destructor, arg);
> 
> when fdarray__filter removes some of the fds the 'pos1,pos2,pos3'
> indexes are not relevant anymore
> 
> how about we make the 'pos indexes' being stable by allocating
> separate object for each added descriptor and each poll call
> would create pollfd array from current objects, and entries
> would keep pointer to its pollfd entry
> 
>   struct fdentry *entry {
>        int              fd;
>        int              events;
>        struct pollfd   *pollfd;
>   }
> 
>   entry1 = fdarray__add(a, fd1 ...);
>   entry2 = fdarray__add(a, fd2 ...);
>   entry3 = fdarray__add(a, fd3 ...);
> 
>   fdarray__poll(a);
> 
>   struct pollfd *fdarray__entry_pollfd(a, entry1);
> 
> or smoething like that ;-)
> 
> jirka
>
Alexey Budankov June 5, 2020, 4:15 p.m. UTC | #4
On 05.06.2020 14:38, Jiri Olsa wrote:
> On Fri, Jun 05, 2020 at 12:50:54PM +0200, Jiri Olsa wrote:
>> On Wed, Jun 03, 2020 at 06:52:59PM +0300, Alexey Budankov wrote:
>>>
>>> Implement adding of file descriptors by fdarray__add_stat() to
>>> fix-sized (currently 1) stat_entries array located at struct fdarray.
>>> Append added file descriptors to the array used by poll() syscall
>>> during fdarray__poll() call. Copy poll() result of the added
>>> descriptors from the array back to the storage for analysis.
>>>
>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>> ---
>>>  tools/lib/api/fd/array.c                 | 42 +++++++++++++++++++++++-
>>>  tools/lib/api/fd/array.h                 |  7 ++++
>>>  tools/lib/perf/evlist.c                  | 11 +++++++
>>>  tools/lib/perf/include/internal/evlist.h |  2 ++
>>>  4 files changed, 61 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
>>> index 58d44d5eee31..b0027f2169c7 100644
>>> --- a/tools/lib/api/fd/array.c
>>> +++ b/tools/lib/api/fd/array.c
>>> @@ -11,10 +11,16 @@
>>>  
>>>  void fdarray__init(struct fdarray *fda, int nr_autogrow)
>>>  {
>>> +	int i;
>>> +
>>>  	fda->entries	 = NULL;
>>>  	fda->priv	 = NULL;
>>>  	fda->nr		 = fda->nr_alloc = 0;
>>>  	fda->nr_autogrow = nr_autogrow;
>>> +
>>> +	fda->nr_stat = 0;
>>> +	for (i = 0; i < FDARRAY__STAT_ENTRIES_MAX; i++)
>>> +		fda->stat_entries[i].fd = -1;
>>>  }
>>>  
>>>  int fdarray__grow(struct fdarray *fda, int nr)
>>> @@ -83,6 +89,20 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
>>>  	return pos;
>>>  }
>>>  
>>> +int fdarray__add_stat(struct fdarray *fda, int fd, short revents)
>>> +{
>>> +	int pos = fda->nr_stat;
>>> +
>>> +	if (pos >= FDARRAY__STAT_ENTRIES_MAX)
>>> +		return -1;
>>> +
>>> +	fda->stat_entries[pos].fd = fd;
>>> +	fda->stat_entries[pos].events = revents;
>>> +	fda->nr_stat++;
>>> +
>>> +	return pos;
>>> +}
>>> +
>>>  int fdarray__filter(struct fdarray *fda, short revents,
>>>  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
>>>  		    void *arg)
>>> @@ -113,7 +133,27 @@ int fdarray__filter(struct fdarray *fda, short revents,
>>>  
>>>  int fdarray__poll(struct fdarray *fda, int timeout)
>>>  {
>>> -	return poll(fda->entries, fda->nr, timeout);
>>> +	int nr, i, pos, res;
>>> +
>>> +	nr = fda->nr;
>>> +
>>> +	for (i = 0; i < fda->nr_stat; i++) {
>>> +		if (fda->stat_entries[i].fd != -1) {
>>> +			pos = fdarray__add(fda, fda->stat_entries[i].fd,
>>> +					   fda->stat_entries[i].events);
>>
>> so every call to fdarray__poll will add whatever is
>> in stat_entries to entries? how is it removed?
>>
>> I think you should either follow what Adrian said
>> and put 'static' descriptors early and check for
>> filter number to match it as an 'quick fix'
>>
>> or we should fix it for real and make it generic
>>
>> so currently the interface is like this:
>>
>>   pos1 = fdarray__add(a, fd1 ... );
>>   pos2 = fdarray__add(a, fd2 ... );
>>   pos3 = fdarray__add(a, fd2 ... );
>>
>>   fdarray__poll(a);
>>
>>   num = fdarray__filter(a, revents, destructor, arg);
>>
>> when fdarray__filter removes some of the fds the 'pos1,pos2,pos3'
>> indexes are not relevant anymore

and that is why the return value of fdarray__add() should be converted
to bool (added/not added). Currently the return value is used as bool
only allover the calling code.

fdarray__add_fixed() brings the notion of fd with fixed pos which is
valid after fdarray__add_fixed() call so the pos could be used to access
pos fd poll status after poll() call.

pos = fdarray__add_fixed(array, fd);
fdarray_poll(array);
revents = fdarray_fixed_revents(array, pos);
fdarray__del(array, pos);

or fdarray__add() could be extended with fixed attribute to avoid separate call:
int fdarray__add(struct fdarray *fda, int fd, short revents, bool fixed)

~Alexey

>>
>> how about we make the 'pos indexes' being stable by allocating
>> separate object for each added descriptor and each poll call
>> would create pollfd array from current objects, and entries
>> would keep pointer to its pollfd entry
>>
>>   struct fdentry *entry {
>>        int              fd;
>>        int              events;
>>        struct pollfd   *pollfd;
>>   }
>>
>>   entry1 = fdarray__add(a, fd1 ...);
>>   entry2 = fdarray__add(a, fd2 ...);
>>   entry3 = fdarray__add(a, fd3 ...);
>>
>>   fdarray__poll(a);
>>
>>   struct pollfd *fdarray__entry_pollfd(a, entry1);
>>
>> or smoething like that ;-)
> 
> maybe something like below (only compile tested)
> 
> jirka
> 
> 
> ---
> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> index 58d44d5eee31..f1effed3dde1 100644
> --- a/tools/lib/api/fd/array.c
> +++ b/tools/lib/api/fd/array.c
> @@ -22,8 +22,8 @@ int fdarray__grow(struct fdarray *fda, int nr)
>  	void *priv;
>  	int nr_alloc = fda->nr_alloc + nr;
>  	size_t psize = sizeof(fda->priv[0]) * nr_alloc;
> -	size_t size  = sizeof(struct pollfd) * nr_alloc;
> -	struct pollfd *entries = realloc(fda->entries, size);
> +	size_t size  = sizeof(struct fdentry *) * nr_alloc;
> +	struct fdentry **entries = realloc(fda->entries, size);
>  
>  	if (entries == NULL)
>  		return -ENOMEM;
> @@ -58,7 +58,12 @@ struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow)
>  
>  void fdarray__exit(struct fdarray *fda)
>  {
> +	int i;
> +
> +	for (i = 0; i < fda->nr; i++)
> +		free(fda->entries[i]);
>  	free(fda->entries);
> +	free(fda->pollfd);
>  	free(fda->priv);
>  	fdarray__init(fda, 0);
>  }
> @@ -69,18 +74,25 @@ void fdarray__delete(struct fdarray *fda)
>  	free(fda);
>  }
>  
> -int fdarray__add(struct fdarray *fda, int fd, short revents)
> +struct fdentry *fdarray__add(struct fdarray *fda, int fd, short revents)
>  {
> -	int pos = fda->nr;
> +	struct fdentry *entry;
>  
>  	if (fda->nr == fda->nr_alloc &&
>  	    fdarray__grow(fda, fda->nr_autogrow) < 0)
> -		return -ENOMEM;
> +		return NULL;
> +
> +	entry = malloc(sizeof(*entry));
> +	if (!entry)
> +		return NULL;
> +
> +	entry->fd = fd;
> +	entry->revents = revents;
> +	entry->pollfd = NULL;
>  
> -	fda->entries[fda->nr].fd     = fd;
> -	fda->entries[fda->nr].events = revents;
> +	fda->entries[fda->nr] = entry;
>  	fda->nr++;
> -	return pos;
> +	return entry;
>  }
>  
>  int fdarray__filter(struct fdarray *fda, short revents,
> @@ -93,7 +105,7 @@ int fdarray__filter(struct fdarray *fda, short revents,
>  		return 0;
>  
>  	for (fd = 0; fd < fda->nr; ++fd) {
> -		if (fda->entries[fd].revents & revents) {
> +		if (fda->entries[fd]->revents & revents) {
>  			if (entry_destructor)
>  				entry_destructor(fda, fd, arg);
>  
> @@ -113,7 +125,22 @@ int fdarray__filter(struct fdarray *fda, short revents,
>  
>  int fdarray__poll(struct fdarray *fda, int timeout)
>  {
> -	return poll(fda->entries, fda->nr, timeout);
> +	struct pollfd *pollfd = fda->pollfd;
> +	int i;
> +
> +	pollfd = realloc(pollfd, sizeof(*pollfd) * fda->nr);
> +	if (!pollfd)
> +		return -ENOMEM;
> +
> +	fda->pollfd = pollfd;
> +
> +	for (i = 0; i < fda->nr; i++) {
> +		pollfd[i].fd = fda->entries[i]->fd;
> +		pollfd[i].revents = fda->entries[i]->revents;
> +		fda->entries[i]->pollfd = &pollfd[i];
> +	}
> +
> +	return poll(pollfd, fda->nr, timeout);
>  }
>  
>  int fdarray__fprintf(struct fdarray *fda, FILE *fp)
> @@ -121,7 +148,12 @@ int fdarray__fprintf(struct fdarray *fda, FILE *fp)
>  	int fd, printed = fprintf(fp, "%d [ ", fda->nr);
>  
>  	for (fd = 0; fd < fda->nr; ++fd)
> -		printed += fprintf(fp, "%s%d", fd ? ", " : "", fda->entries[fd].fd);
> +		printed += fprintf(fp, "%s%d", fd ? ", " : "", fda->entries[fd]->fd);
>  
>  	return printed + fprintf(fp, " ]");
>  }
> +
> +int fdentry__events(struct fdentry *entry)
> +{
> +	return entry->pollfd->revents;
> +}
> diff --git a/tools/lib/api/fd/array.h b/tools/lib/api/fd/array.h
> index b39557d1a88f..5231ce047f2e 100644
> --- a/tools/lib/api/fd/array.h
> +++ b/tools/lib/api/fd/array.h
> @@ -6,6 +6,12 @@
>  
>  struct pollfd;
>  
> +struct fdentry {
> +	int		 fd;
> +	int		 revents;
> +	struct pollfd	*pollfd;
> +};
> +
>  /**
>   * struct fdarray: Array of file descriptors
>   *
> @@ -20,7 +26,10 @@ struct fdarray {
>  	int	       nr;
>  	int	       nr_alloc;
>  	int	       nr_autogrow;
> -	struct pollfd *entries;
> +
> +	struct fdentry	**entries;
> +	struct pollfd	 *pollfd;
> +
>  	union {
>  		int    idx;
>  		void   *ptr;
> @@ -33,7 +42,7 @@ void fdarray__exit(struct fdarray *fda);
>  struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow);
>  void fdarray__delete(struct fdarray *fda);
>  
> -int fdarray__add(struct fdarray *fda, int fd, short revents);
> +struct fdentry *fdarray__add(struct fdarray *fda, int fd, short revents);
>  int fdarray__poll(struct fdarray *fda, int timeout);
>  int fdarray__filter(struct fdarray *fda, short revents,
>  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
> @@ -41,6 +50,8 @@ int fdarray__filter(struct fdarray *fda, short revents,
>  int fdarray__grow(struct fdarray *fda, int extra);
>  int fdarray__fprintf(struct fdarray *fda, FILE *fp);
>  
> +int fdentry__events(struct fdentry *entry);
> +
>  static inline int fdarray__available_entries(struct fdarray *fda)
>  {
>  	return fda->nr_alloc - fda->nr;
>
Alexey Budankov June 8, 2020, 8:08 a.m. UTC | #5
On 05.06.2020 19:15, Alexey Budankov wrote:
> 
> On 05.06.2020 14:38, Jiri Olsa wrote:
>> On Fri, Jun 05, 2020 at 12:50:54PM +0200, Jiri Olsa wrote:
>>> On Wed, Jun 03, 2020 at 06:52:59PM +0300, Alexey Budankov wrote:
>>>>
>>>> Implement adding of file descriptors by fdarray__add_stat() to
>>>> fix-sized (currently 1) stat_entries array located at struct fdarray.
>>>> Append added file descriptors to the array used by poll() syscall
>>>> during fdarray__poll() call. Copy poll() result of the added
>>>> descriptors from the array back to the storage for analysis.
>>>>
>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>> ---
>>>>  tools/lib/api/fd/array.c                 | 42 +++++++++++++++++++++++-
>>>>  tools/lib/api/fd/array.h                 |  7 ++++
>>>>  tools/lib/perf/evlist.c                  | 11 +++++++
>>>>  tools/lib/perf/include/internal/evlist.h |  2 ++
>>>>  4 files changed, 61 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
>>>> index 58d44d5eee31..b0027f2169c7 100644
>>>> --- a/tools/lib/api/fd/array.c
>>>> +++ b/tools/lib/api/fd/array.c
>>>> @@ -11,10 +11,16 @@
>>>>  
>>>>  void fdarray__init(struct fdarray *fda, int nr_autogrow)
>>>>  {
>>>> +	int i;
>>>> +
>>>>  	fda->entries	 = NULL;
>>>>  	fda->priv	 = NULL;
>>>>  	fda->nr		 = fda->nr_alloc = 0;
>>>>  	fda->nr_autogrow = nr_autogrow;
>>>> +
>>>> +	fda->nr_stat = 0;
>>>> +	for (i = 0; i < FDARRAY__STAT_ENTRIES_MAX; i++)
>>>> +		fda->stat_entries[i].fd = -1;
>>>>  }
>>>>  
>>>>  int fdarray__grow(struct fdarray *fda, int nr)
>>>> @@ -83,6 +89,20 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
>>>>  	return pos;
>>>>  }
>>>>  
>>>> +int fdarray__add_stat(struct fdarray *fda, int fd, short revents)
>>>> +{
>>>> +	int pos = fda->nr_stat;
>>>> +
>>>> +	if (pos >= FDARRAY__STAT_ENTRIES_MAX)
>>>> +		return -1;
>>>> +
>>>> +	fda->stat_entries[pos].fd = fd;
>>>> +	fda->stat_entries[pos].events = revents;
>>>> +	fda->nr_stat++;
>>>> +
>>>> +	return pos;
>>>> +}
>>>> +
>>>>  int fdarray__filter(struct fdarray *fda, short revents,
>>>>  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
>>>>  		    void *arg)
>>>> @@ -113,7 +133,27 @@ int fdarray__filter(struct fdarray *fda, short revents,
>>>>  
>>>>  int fdarray__poll(struct fdarray *fda, int timeout)
>>>>  {
>>>> -	return poll(fda->entries, fda->nr, timeout);
>>>> +	int nr, i, pos, res;
>>>> +
>>>> +	nr = fda->nr;
>>>> +
>>>> +	for (i = 0; i < fda->nr_stat; i++) {
>>>> +		if (fda->stat_entries[i].fd != -1) {
>>>> +			pos = fdarray__add(fda, fda->stat_entries[i].fd,
>>>> +					   fda->stat_entries[i].events);
>>>
>>> so every call to fdarray__poll will add whatever is
>>> in stat_entries to entries? how is it removed?
>>>
>>> I think you should either follow what Adrian said
>>> and put 'static' descriptors early and check for
>>> filter number to match it as an 'quick fix'
>>>
>>> or we should fix it for real and make it generic
>>>
>>> so currently the interface is like this:
>>>
>>>   pos1 = fdarray__add(a, fd1 ... );
>>>   pos2 = fdarray__add(a, fd2 ... );
>>>   pos3 = fdarray__add(a, fd2 ... );
>>>
>>>   fdarray__poll(a);
>>>
>>>   num = fdarray__filter(a, revents, destructor, arg);
>>>
>>> when fdarray__filter removes some of the fds the 'pos1,pos2,pos3'
>>> indexes are not relevant anymore
> 
> and that is why the return value of fdarray__add() should be converted
> to bool (added/not added). Currently the return value is used as bool
> only allover the calling code.
> 
> fdarray__add_fixed() brings the notion of fd with fixed pos which is
> valid after fdarray__add_fixed() call so the pos could be used to access
> pos fd poll status after poll() call.
> 
> pos = fdarray__add_fixed(array, fd);
> fdarray_poll(array);
> 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()?

~Alexey
Jiri Olsa June 8, 2020, 8:43 a.m. UTC | #6
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:
> >> On Fri, Jun 05, 2020 at 12:50:54PM +0200, Jiri Olsa wrote:
> >>> On Wed, Jun 03, 2020 at 06:52:59PM +0300, Alexey Budankov wrote:
> >>>>
> >>>> Implement adding of file descriptors by fdarray__add_stat() to
> >>>> fix-sized (currently 1) stat_entries array located at struct fdarray.
> >>>> Append added file descriptors to the array used by poll() syscall
> >>>> during fdarray__poll() call. Copy poll() result of the added
> >>>> descriptors from the array back to the storage for analysis.
> >>>>
> >>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> >>>> ---
> >>>>  tools/lib/api/fd/array.c                 | 42 +++++++++++++++++++++++-
> >>>>  tools/lib/api/fd/array.h                 |  7 ++++
> >>>>  tools/lib/perf/evlist.c                  | 11 +++++++
> >>>>  tools/lib/perf/include/internal/evlist.h |  2 ++
> >>>>  4 files changed, 61 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> >>>> index 58d44d5eee31..b0027f2169c7 100644
> >>>> --- a/tools/lib/api/fd/array.c
> >>>> +++ b/tools/lib/api/fd/array.c
> >>>> @@ -11,10 +11,16 @@
> >>>>  
> >>>>  void fdarray__init(struct fdarray *fda, int nr_autogrow)
> >>>>  {
> >>>> +	int i;
> >>>> +
> >>>>  	fda->entries	 = NULL;
> >>>>  	fda->priv	 = NULL;
> >>>>  	fda->nr		 = fda->nr_alloc = 0;
> >>>>  	fda->nr_autogrow = nr_autogrow;
> >>>> +
> >>>> +	fda->nr_stat = 0;
> >>>> +	for (i = 0; i < FDARRAY__STAT_ENTRIES_MAX; i++)
> >>>> +		fda->stat_entries[i].fd = -1;
> >>>>  }
> >>>>  
> >>>>  int fdarray__grow(struct fdarray *fda, int nr)
> >>>> @@ -83,6 +89,20 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
> >>>>  	return pos;
> >>>>  }
> >>>>  
> >>>> +int fdarray__add_stat(struct fdarray *fda, int fd, short revents)
> >>>> +{
> >>>> +	int pos = fda->nr_stat;
> >>>> +
> >>>> +	if (pos >= FDARRAY__STAT_ENTRIES_MAX)
> >>>> +		return -1;
> >>>> +
> >>>> +	fda->stat_entries[pos].fd = fd;
> >>>> +	fda->stat_entries[pos].events = revents;
> >>>> +	fda->nr_stat++;
> >>>> +
> >>>> +	return pos;
> >>>> +}
> >>>> +
> >>>>  int fdarray__filter(struct fdarray *fda, short revents,
> >>>>  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
> >>>>  		    void *arg)
> >>>> @@ -113,7 +133,27 @@ int fdarray__filter(struct fdarray *fda, short revents,
> >>>>  
> >>>>  int fdarray__poll(struct fdarray *fda, int timeout)
> >>>>  {
> >>>> -	return poll(fda->entries, fda->nr, timeout);
> >>>> +	int nr, i, pos, res;
> >>>> +
> >>>> +	nr = fda->nr;
> >>>> +
> >>>> +	for (i = 0; i < fda->nr_stat; i++) {
> >>>> +		if (fda->stat_entries[i].fd != -1) {
> >>>> +			pos = fdarray__add(fda, fda->stat_entries[i].fd,
> >>>> +					   fda->stat_entries[i].events);
> >>>
> >>> so every call to fdarray__poll will add whatever is
> >>> in stat_entries to entries? how is it removed?
> >>>
> >>> I think you should either follow what Adrian said
> >>> and put 'static' descriptors early and check for
> >>> filter number to match it as an 'quick fix'
> >>>
> >>> or we should fix it for real and make it generic
> >>>
> >>> so currently the interface is like this:
> >>>
> >>>   pos1 = fdarray__add(a, fd1 ... );
> >>>   pos2 = fdarray__add(a, fd2 ... );
> >>>   pos3 = fdarray__add(a, fd2 ... );
> >>>
> >>>   fdarray__poll(a);
> >>>
> >>>   num = fdarray__filter(a, revents, destructor, arg);
> >>>
> >>> when fdarray__filter removes some of the fds the 'pos1,pos2,pos3'
> >>> indexes are not relevant anymore
> > 
> > and that is why the return value of fdarray__add() should be converted
> > to bool (added/not added). Currently the return value is used as bool
> > only allover the calling code.
> > 
> > fdarray__add_fixed() brings the notion of fd with fixed pos which is
> > valid after fdarray__add_fixed() call so the pos could be used to access
> > pos fd poll status after poll() call.
> > 
> > pos = fdarray__add_fixed(array, fd);
> > fdarray_poll(array);
> > 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?

jirka
Alexey Budankov June 8, 2020, 9:54 a.m. UTC | #7
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:
>>>> On Fri, Jun 05, 2020 at 12:50:54PM +0200, Jiri Olsa wrote:
>>>>> On Wed, Jun 03, 2020 at 06:52:59PM +0300, Alexey Budankov wrote:
>>>>>>
>>>>>> Implement adding of file descriptors by fdarray__add_stat() to
>>>>>> fix-sized (currently 1) stat_entries array located at struct fdarray.
>>>>>> Append added file descriptors to the array used by poll() syscall
>>>>>> during fdarray__poll() call. Copy poll() result of the added
>>>>>> descriptors from the array back to the storage for analysis.
>>>>>>
>>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>>>> ---
>>>>>>  tools/lib/api/fd/array.c                 | 42 +++++++++++++++++++++++-
>>>>>>  tools/lib/api/fd/array.h                 |  7 ++++
>>>>>>  tools/lib/perf/evlist.c                  | 11 +++++++
>>>>>>  tools/lib/perf/include/internal/evlist.h |  2 ++
>>>>>>  4 files changed, 61 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
>>>>>> index 58d44d5eee31..b0027f2169c7 100644
>>>>>> --- a/tools/lib/api/fd/array.c
>>>>>> +++ b/tools/lib/api/fd/array.c
>>>>>> @@ -11,10 +11,16 @@
>>>>>>  
>>>>>>  void fdarray__init(struct fdarray *fda, int nr_autogrow)
>>>>>>  {
>>>>>> +	int i;
>>>>>> +
>>>>>>  	fda->entries	 = NULL;
>>>>>>  	fda->priv	 = NULL;
>>>>>>  	fda->nr		 = fda->nr_alloc = 0;
>>>>>>  	fda->nr_autogrow = nr_autogrow;
>>>>>> +
>>>>>> +	fda->nr_stat = 0;
>>>>>> +	for (i = 0; i < FDARRAY__STAT_ENTRIES_MAX; i++)
>>>>>> +		fda->stat_entries[i].fd = -1;
>>>>>>  }
>>>>>>  
>>>>>>  int fdarray__grow(struct fdarray *fda, int nr)
>>>>>> @@ -83,6 +89,20 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
>>>>>>  	return pos;
>>>>>>  }
>>>>>>  
>>>>>> +int fdarray__add_stat(struct fdarray *fda, int fd, short revents)
>>>>>> +{
>>>>>> +	int pos = fda->nr_stat;
>>>>>> +
>>>>>> +	if (pos >= FDARRAY__STAT_ENTRIES_MAX)
>>>>>> +		return -1;
>>>>>> +
>>>>>> +	fda->stat_entries[pos].fd = fd;
>>>>>> +	fda->stat_entries[pos].events = revents;
>>>>>> +	fda->nr_stat++;
>>>>>> +
>>>>>> +	return pos;
>>>>>> +}
>>>>>> +
>>>>>>  int fdarray__filter(struct fdarray *fda, short revents,
>>>>>>  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
>>>>>>  		    void *arg)
>>>>>> @@ -113,7 +133,27 @@ int fdarray__filter(struct fdarray *fda, short revents,
>>>>>>  
>>>>>>  int fdarray__poll(struct fdarray *fda, int timeout)
>>>>>>  {
>>>>>> -	return poll(fda->entries, fda->nr, timeout);
>>>>>> +	int nr, i, pos, res;
>>>>>> +
>>>>>> +	nr = fda->nr;
>>>>>> +
>>>>>> +	for (i = 0; i < fda->nr_stat; i++) {
>>>>>> +		if (fda->stat_entries[i].fd != -1) {
>>>>>> +			pos = fdarray__add(fda, fda->stat_entries[i].fd,
>>>>>> +					   fda->stat_entries[i].events);
>>>>>
>>>>> so every call to fdarray__poll will add whatever is
>>>>> in stat_entries to entries? how is it removed?
>>>>>
>>>>> I think you should either follow what Adrian said
>>>>> and put 'static' descriptors early and check for
>>>>> filter number to match it as an 'quick fix'
>>>>>
>>>>> or we should fix it for real and make it generic
>>>>>
>>>>> so currently the interface is like this:
>>>>>
>>>>>   pos1 = fdarray__add(a, fd1 ... );
>>>>>   pos2 = fdarray__add(a, fd2 ... );
>>>>>   pos3 = fdarray__add(a, fd2 ... );
>>>>>
>>>>>   fdarray__poll(a);
>>>>>
>>>>>   num = fdarray__filter(a, revents, destructor, arg);
>>>>>
>>>>> when fdarray__filter removes some of the fds the 'pos1,pos2,pos3'
>>>>> indexes are not relevant anymore
>>>
>>> and that is why the return value of fdarray__add() should be converted
>>> to bool (added/not added). Currently the return value is used as bool
>>> only allover the calling code.
>>>
>>> fdarray__add_fixed() brings the notion of fd with fixed pos which is
>>> valid after fdarray__add_fixed() call so the pos could be used to access
>>> pos fd poll status after poll() call.
>>>
>>> pos = fdarray__add_fixed(array, fd);
>>> fdarray_poll(array);
>>> 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);

~Alexey
Alexey Budankov June 8, 2020, 3:05 p.m. UTC | #8
On 08.06.2020 12:54, 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:
>>>>> On Fri, Jun 05, 2020 at 12:50:54PM +0200, Jiri Olsa wrote:
>>>>>> On Wed, Jun 03, 2020 at 06:52:59PM +0300, Alexey Budankov wrote:
>>>>>>>
>>>>>>> Implement adding of file descriptors by fdarray__add_stat() to
>>>>>>> fix-sized (currently 1) stat_entries array located at struct fdarray.
>>>>>>> Append added file descriptors to the array used by poll() syscall
>>>>>>> during fdarray__poll() call. Copy poll() result of the added
>>>>>>> descriptors from the array back to the storage for analysis.
>>>>>>>
>>>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>>>>> ---
<SNIP>
>>>>>>> +					   fda->stat_entries[i].events);
>>>>>>
>>>>>> so every call to fdarray__poll will add whatever is
>>>>>> in stat_entries to entries? how is it removed?
>>>>>>
>>>>>> I think you should either follow what Adrian said
>>>>>> and put 'static' descriptors early and check for
>>>>>> filter number to match it as an 'quick fix'
>>>>>>
>>>>>> or we should fix it for real and make it generic
>>>>>>
>>>>>> so currently the interface is like this:
>>>>>>
>>>>>>   pos1 = fdarray__add(a, fd1 ... );
>>>>>>   pos2 = fdarray__add(a, fd2 ... );
>>>>>>   pos3 = fdarray__add(a, fd2 ... );
>>>>>>
>>>>>>   fdarray__poll(a);
>>>>>>
>>>>>>   num = fdarray__filter(a, revents, destructor, arg);
>>>>>>
>>>>>> when fdarray__filter removes some of the fds the 'pos1,pos2,pos3'
>>>>>> indexes are not relevant anymore
>>>>
>>>> and that is why the return value of fdarray__add() should be converted
>>>> to bool (added/not added). Currently the return value is used as bool
>>>> only allover the calling code.
>>>>
>>>> fdarray__add_fixed() brings the notion of fd with fixed pos which is
>>>> valid after fdarray__add_fixed() call so the pos could be used to access
>>>> pos fd poll status after poll() call.
>>>>
>>>> pos = fdarray__add_fixed(array, fd);
>>>> fdarray_poll(array);
>>>> 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);

Are there any thoughts regarding this?

~Alexey
Jiri Olsa June 8, 2020, 4:07 p.m. UTC | #9
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:
> >>>> On Fri, Jun 05, 2020 at 12:50:54PM +0200, Jiri Olsa wrote:
> >>>>> On Wed, Jun 03, 2020 at 06:52:59PM +0300, Alexey Budankov wrote:
> >>>>>>
> >>>>>> Implement adding of file descriptors by fdarray__add_stat() to
> >>>>>> fix-sized (currently 1) stat_entries array located at struct fdarray.
> >>>>>> Append added file descriptors to the array used by poll() syscall
> >>>>>> during fdarray__poll() call. Copy poll() result of the added
> >>>>>> descriptors from the array back to the storage for analysis.
> >>>>>>
> >>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
> >>>>>> ---
> >>>>>>  tools/lib/api/fd/array.c                 | 42 +++++++++++++++++++++++-
> >>>>>>  tools/lib/api/fd/array.h                 |  7 ++++
> >>>>>>  tools/lib/perf/evlist.c                  | 11 +++++++
> >>>>>>  tools/lib/perf/include/internal/evlist.h |  2 ++
> >>>>>>  4 files changed, 61 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> >>>>>> index 58d44d5eee31..b0027f2169c7 100644
> >>>>>> --- a/tools/lib/api/fd/array.c
> >>>>>> +++ b/tools/lib/api/fd/array.c
> >>>>>> @@ -11,10 +11,16 @@
> >>>>>>  
> >>>>>>  void fdarray__init(struct fdarray *fda, int nr_autogrow)
> >>>>>>  {
> >>>>>> +	int i;
> >>>>>> +
> >>>>>>  	fda->entries	 = NULL;
> >>>>>>  	fda->priv	 = NULL;
> >>>>>>  	fda->nr		 = fda->nr_alloc = 0;
> >>>>>>  	fda->nr_autogrow = nr_autogrow;
> >>>>>> +
> >>>>>> +	fda->nr_stat = 0;
> >>>>>> +	for (i = 0; i < FDARRAY__STAT_ENTRIES_MAX; i++)
> >>>>>> +		fda->stat_entries[i].fd = -1;
> >>>>>>  }
> >>>>>>  
> >>>>>>  int fdarray__grow(struct fdarray *fda, int nr)
> >>>>>> @@ -83,6 +89,20 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
> >>>>>>  	return pos;
> >>>>>>  }
> >>>>>>  
> >>>>>> +int fdarray__add_stat(struct fdarray *fda, int fd, short revents)
> >>>>>> +{
> >>>>>> +	int pos = fda->nr_stat;
> >>>>>> +
> >>>>>> +	if (pos >= FDARRAY__STAT_ENTRIES_MAX)
> >>>>>> +		return -1;
> >>>>>> +
> >>>>>> +	fda->stat_entries[pos].fd = fd;
> >>>>>> +	fda->stat_entries[pos].events = revents;
> >>>>>> +	fda->nr_stat++;
> >>>>>> +
> >>>>>> +	return pos;
> >>>>>> +}
> >>>>>> +
> >>>>>>  int fdarray__filter(struct fdarray *fda, short revents,
> >>>>>>  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
> >>>>>>  		    void *arg)
> >>>>>> @@ -113,7 +133,27 @@ int fdarray__filter(struct fdarray *fda, short revents,
> >>>>>>  
> >>>>>>  int fdarray__poll(struct fdarray *fda, int timeout)
> >>>>>>  {
> >>>>>> -	return poll(fda->entries, fda->nr, timeout);
> >>>>>> +	int nr, i, pos, res;
> >>>>>> +
> >>>>>> +	nr = fda->nr;
> >>>>>> +
> >>>>>> +	for (i = 0; i < fda->nr_stat; i++) {
> >>>>>> +		if (fda->stat_entries[i].fd != -1) {
> >>>>>> +			pos = fdarray__add(fda, fda->stat_entries[i].fd,
> >>>>>> +					   fda->stat_entries[i].events);
> >>>>>
> >>>>> so every call to fdarray__poll will add whatever is
> >>>>> in stat_entries to entries? how is it removed?
> >>>>>
> >>>>> I think you should either follow what Adrian said
> >>>>> and put 'static' descriptors early and check for
> >>>>> filter number to match it as an 'quick fix'
> >>>>>
> >>>>> or we should fix it for real and make it generic
> >>>>>
> >>>>> so currently the interface is like this:
> >>>>>
> >>>>>   pos1 = fdarray__add(a, fd1 ... );
> >>>>>   pos2 = fdarray__add(a, fd2 ... );
> >>>>>   pos3 = fdarray__add(a, fd2 ... );
> >>>>>
> >>>>>   fdarray__poll(a);
> >>>>>
> >>>>>   num = fdarray__filter(a, revents, destructor, arg);
> >>>>>
> >>>>> when fdarray__filter removes some of the fds the 'pos1,pos2,pos3'
> >>>>> indexes are not relevant anymore
> >>>
> >>> and that is why the return value of fdarray__add() should be converted
> >>> to bool (added/not added). Currently the return value is used as bool
> >>> only allover the calling code.
> >>>
> >>> fdarray__add_fixed() brings the notion of fd with fixed pos which is
> >>> valid after fdarray__add_fixed() call so the pos could be used to access
> >>> pos fd poll status after poll() call.
> >>>
> >>> pos = fdarray__add_fixed(array, fd);
> >>> fdarray_poll(array);
> >>> 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

jirka
Alexey Budankov June 8, 2020, 4:43 p.m. UTC | #10
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:
>>>>>> On Fri, Jun 05, 2020 at 12:50:54PM +0200, Jiri Olsa wrote:
>>>>>>> On Wed, Jun 03, 2020 at 06:52:59PM +0300, Alexey Budankov wrote:
>>>>>>>>
>>>>>>>> Implement adding of file descriptors by fdarray__add_stat() to
>>>>>>>> fix-sized (currently 1) stat_entries array located at struct fdarray.
>>>>>>>> Append added file descriptors to the array used by poll() syscall
>>>>>>>> during fdarray__poll() call. Copy poll() result of the added
>>>>>>>> descriptors from the array back to the storage for analysis.
>>>>>>>>
>>>>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>>>>>> ---
>>>>>>>>  tools/lib/api/fd/array.c                 | 42 +++++++++++++++++++++++-
>>>>>>>>  tools/lib/api/fd/array.h                 |  7 ++++
>>>>>>>>  tools/lib/perf/evlist.c                  | 11 +++++++
>>>>>>>>  tools/lib/perf/include/internal/evlist.h |  2 ++
>>>>>>>>  4 files changed, 61 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
>>>>>>>> index 58d44d5eee31..b0027f2169c7 100644
>>>>>>>> --- a/tools/lib/api/fd/array.c
>>>>>>>> +++ b/tools/lib/api/fd/array.c
>>>>>>>> @@ -11,10 +11,16 @@
>>>>>>>>  
>>>>>>>>  void fdarray__init(struct fdarray *fda, int nr_autogrow)
>>>>>>>>  {
>>>>>>>> +	int i;
>>>>>>>> +
>>>>>>>>  	fda->entries	 = NULL;
>>>>>>>>  	fda->priv	 = NULL;
>>>>>>>>  	fda->nr		 = fda->nr_alloc = 0;
>>>>>>>>  	fda->nr_autogrow = nr_autogrow;
>>>>>>>> +
>>>>>>>> +	fda->nr_stat = 0;
>>>>>>>> +	for (i = 0; i < FDARRAY__STAT_ENTRIES_MAX; i++)
>>>>>>>> +		fda->stat_entries[i].fd = -1;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>>  int fdarray__grow(struct fdarray *fda, int nr)
>>>>>>>> @@ -83,6 +89,20 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
>>>>>>>>  	return pos;
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +int fdarray__add_stat(struct fdarray *fda, int fd, short revents)
>>>>>>>> +{
>>>>>>>> +	int pos = fda->nr_stat;
>>>>>>>> +
>>>>>>>> +	if (pos >= FDARRAY__STAT_ENTRIES_MAX)
>>>>>>>> +		return -1;
>>>>>>>> +
>>>>>>>> +	fda->stat_entries[pos].fd = fd;
>>>>>>>> +	fda->stat_entries[pos].events = revents;
>>>>>>>> +	fda->nr_stat++;
>>>>>>>> +
>>>>>>>> +	return pos;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>  int fdarray__filter(struct fdarray *fda, short revents,
>>>>>>>>  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
>>>>>>>>  		    void *arg)
>>>>>>>> @@ -113,7 +133,27 @@ int fdarray__filter(struct fdarray *fda, short revents,
>>>>>>>>  
>>>>>>>>  int fdarray__poll(struct fdarray *fda, int timeout)
>>>>>>>>  {
>>>>>>>> -	return poll(fda->entries, fda->nr, timeout);
>>>>>>>> +	int nr, i, pos, res;
>>>>>>>> +
>>>>>>>> +	nr = fda->nr;
>>>>>>>> +
>>>>>>>> +	for (i = 0; i < fda->nr_stat; i++) {
>>>>>>>> +		if (fda->stat_entries[i].fd != -1) {
>>>>>>>> +			pos = fdarray__add(fda, fda->stat_entries[i].fd,
>>>>>>>> +					   fda->stat_entries[i].events);
>>>>>>>
>>>>>>> so every call to fdarray__poll will add whatever is
>>>>>>> in stat_entries to entries? how is it removed?
>>>>>>>
>>>>>>> I think you should either follow what Adrian said
>>>>>>> and put 'static' descriptors early and check for
>>>>>>> filter number to match it as an 'quick fix'
>>>>>>>
>>>>>>> or we should fix it for real and make it generic
>>>>>>>
>>>>>>> so currently the interface is like this:
>>>>>>>
>>>>>>>   pos1 = fdarray__add(a, fd1 ... );
>>>>>>>   pos2 = fdarray__add(a, fd2 ... );
>>>>>>>   pos3 = fdarray__add(a, fd2 ... );
>>>>>>>
>>>>>>>   fdarray__poll(a);
>>>>>>>
>>>>>>>   num = fdarray__filter(a, revents, destructor, arg);
>>>>>>>
>>>>>>> when fdarray__filter removes some of the fds the 'pos1,pos2,pos3'
>>>>>>> indexes are not relevant anymore
>>>>>
>>>>> and that is why the return value of fdarray__add() should be converted
>>>>> to bool (added/not added). Currently the return value is used as bool
>>>>> only allover the calling code.
>>>>>
>>>>> fdarray__add_fixed() brings the notion of fd with fixed pos which is
>>>>> valid after fdarray__add_fixed() call so the pos could be used to access
>>>>> pos fd poll status after poll() call.
>>>>>
>>>>> pos = fdarray__add_fixed(array, fd);
>>>>> fdarray_poll(array);
>>>>> 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,

and still making the atomic fdarray__poll()?

> I'm not sure why you think this is needed

In order to cause min changes to the existing code,
as in libperf as in the tool.

~Alexey

> 
> I'm busy with other things this week, but I think I can
> come up with some patch early next week if needed
> 
> jirka
>
Alexey Budankov June 8, 2020, 5:18 p.m. UTC | #11
On 08.06.2020 19:43, 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:
>>>>>>> On Fri, Jun 05, 2020 at 12:50:54PM +0200, Jiri Olsa wrote:
>>>>>>>> On Wed, Jun 03, 2020 at 06:52:59PM +0300, Alexey Budankov wrote:
>>>>>>>>>
>>>>>>>>> Implement adding of file descriptors by fdarray__add_stat() to
>>>>>>>>> fix-sized (currently 1) stat_entries array located at struct fdarray.
>>>>>>>>> Append added file descriptors to the array used by poll() syscall
>>>>>>>>> during fdarray__poll() call. Copy poll() result of the added
>>>>>>>>> descriptors from the array back to the storage for analysis.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Alexey Budankov <alexey.budankov@linux.intel.com>
>>>>>>>>> ---
>>>>>>>>>  tools/lib/api/fd/array.c                 | 42 +++++++++++++++++++++++-
>>>>>>>>>  tools/lib/api/fd/array.h                 |  7 ++++
>>>>>>>>>  tools/lib/perf/evlist.c                  | 11 +++++++
>>>>>>>>>  tools/lib/perf/include/internal/evlist.h |  2 ++
>>>>>>>>>  4 files changed, 61 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
>>>>>>>>> index 58d44d5eee31..b0027f2169c7 100644
>>>>>>>>> --- a/tools/lib/api/fd/array.c
>>>>>>>>> +++ b/tools/lib/api/fd/array.c
>>>>>>>>> @@ -11,10 +11,16 @@
>>>>>>>>>  
>>>>>>>>>  void fdarray__init(struct fdarray *fda, int nr_autogrow)
>>>>>>>>>  {
>>>>>>>>> +	int i;
>>>>>>>>> +
>>>>>>>>>  	fda->entries	 = NULL;
>>>>>>>>>  	fda->priv	 = NULL;
>>>>>>>>>  	fda->nr		 = fda->nr_alloc = 0;
>>>>>>>>>  	fda->nr_autogrow = nr_autogrow;
>>>>>>>>> +
>>>>>>>>> +	fda->nr_stat = 0;
>>>>>>>>> +	for (i = 0; i < FDARRAY__STAT_ENTRIES_MAX; i++)
>>>>>>>>> +		fda->stat_entries[i].fd = -1;
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>>  int fdarray__grow(struct fdarray *fda, int nr)
>>>>>>>>> @@ -83,6 +89,20 @@ int fdarray__add(struct fdarray *fda, int fd, short revents)
>>>>>>>>>  	return pos;
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>> +int fdarray__add_stat(struct fdarray *fda, int fd, short revents)
>>>>>>>>> +{
>>>>>>>>> +	int pos = fda->nr_stat;
>>>>>>>>> +
>>>>>>>>> +	if (pos >= FDARRAY__STAT_ENTRIES_MAX)
>>>>>>>>> +		return -1;
>>>>>>>>> +
>>>>>>>>> +	fda->stat_entries[pos].fd = fd;
>>>>>>>>> +	fda->stat_entries[pos].events = revents;
>>>>>>>>> +	fda->nr_stat++;
>>>>>>>>> +
>>>>>>>>> +	return pos;
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>  int fdarray__filter(struct fdarray *fda, short revents,
>>>>>>>>>  		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
>>>>>>>>>  		    void *arg)
>>>>>>>>> @@ -113,7 +133,27 @@ int fdarray__filter(struct fdarray *fda, short revents,
>>>>>>>>>  
>>>>>>>>>  int fdarray__poll(struct fdarray *fda, int timeout)
>>>>>>>>>  {
>>>>>>>>> -	return poll(fda->entries, fda->nr, timeout);
>>>>>>>>> +	int nr, i, pos, res;
>>>>>>>>> +
>>>>>>>>> +	nr = fda->nr;
>>>>>>>>> +
>>>>>>>>> +	for (i = 0; i < fda->nr_stat; i++) {
>>>>>>>>> +		if (fda->stat_entries[i].fd != -1) {
>>>>>>>>> +			pos = fdarray__add(fda, fda->stat_entries[i].fd,
>>>>>>>>> +					   fda->stat_entries[i].events);
>>>>>>>>
>>>>>>>> so every call to fdarray__poll will add whatever is
>>>>>>>> in stat_entries to entries? how is it removed?
>>>>>>>>
>>>>>>>> I think you should either follow what Adrian said
>>>>>>>> and put 'static' descriptors early and check for
>>>>>>>> filter number to match it as an 'quick fix'
>>>>>>>>
>>>>>>>> or we should fix it for real and make it generic
>>>>>>>>
>>>>>>>> so currently the interface is like this:
>>>>>>>>
>>>>>>>>   pos1 = fdarray__add(a, fd1 ... );
>>>>>>>>   pos2 = fdarray__add(a, fd2 ... );
>>>>>>>>   pos3 = fdarray__add(a, fd2 ... );
>>>>>>>>
>>>>>>>>   fdarray__poll(a);
>>>>>>>>
>>>>>>>>   num = fdarray__filter(a, revents, destructor, arg);
>>>>>>>>
>>>>>>>> when fdarray__filter removes some of the fds the 'pos1,pos2,pos3'
>>>>>>>> indexes are not relevant anymore
>>>>>>
>>>>>> and that is why the return value of fdarray__add() should be converted
>>>>>> to bool (added/not added). Currently the return value is used as bool
>>>>>> only allover the calling code.
>>>>>>
>>>>>> fdarray__add_fixed() brings the notion of fd with fixed pos which is
>>>>>> valid after fdarray__add_fixed() call so the pos could be used to access
>>>>>> pos fd poll status after poll() call.
>>>>>>
>>>>>> pos = fdarray__add_fixed(array, fd);
>>>>>> fdarray_poll(array);
>>>>>> 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,
> 
> and still making the atomic fdarray__poll()?

How is it about design like this?

    int fdarray__poll(struct fdarray *fda, int timeout)

with additional external array of fds to simultaneously poll() on:

    int fdarray__poll(struct fdarray *fda, int timeout,
                      int *fds, size_t fds_size)

fds would be added to array just prior poll() call.

That wouldn't cause fdarray class extension with fd types and wouldn't change 
fdarray__filter() keeping the types processing outside of fdarray and libperf.

Adoption of new fdarray__poll() signature in the tool via macro (or inline function):

#define fdarray__poll(fda, timeout) \
	fdarray__poll_ext(fda, timeout, NULL, 0)
int fdarray__poll_ext(struct fdarray *fda, int timeout,
                      int *fds, size_t fds_size)

~Alexey
Jiri Olsa June 9, 2020, 2:56 p.m. UTC | #12
On Mon, Jun 08, 2020 at 08:18:20PM +0300, Alexey Budankov wrote:

SNIP

> >>>>> 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,

so with the changes I proposed it could no longer be called fdarray ;-)
which I think was the idea at the begning.. just an array of fds

I'd like to have fully flaged events object.. but that's bigger change

> > 
> > and still making the atomic fdarray__poll()?
> 
> How is it about design like this?
> 
>     int fdarray__poll(struct fdarray *fda, int timeout)
> 
> with additional external array of fds to simultaneously poll() on:
> 
>     int fdarray__poll(struct fdarray *fda, int timeout,
>                       int *fds, size_t fds_size)
> 
> fds would be added to array just prior poll() call.

yep, I was considering something like this, having:

  fdarray__poll2(fda1, fda2)
  fdarray__pollx(fda, ...)

but it would need to create an pollfd array and write
the poll results back to arrays.. might be expensive

another idea is to forbid filter to screw the array
and return only remaining number, like below

jirka


---
diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
index 58d44d5eee31..89f9a2193c2d 100644
--- a/tools/lib/api/fd/array.c
+++ b/tools/lib/api/fd/array.c
@@ -93,22 +93,21 @@ int fdarray__filter(struct fdarray *fda, short revents,
 		return 0;
 
 	for (fd = 0; fd < fda->nr; ++fd) {
+		if (!fda->entries[fd].events)
+			continue;
+
 		if (fda->entries[fd].revents & revents) {
 			if (entry_destructor)
 				entry_destructor(fda, fd, arg);
 
+			fda->entries[fd].revents = fda->entries[fd].events = 0;
 			continue;
 		}
 
-		if (fd != nr) {
-			fda->entries[nr] = fda->entries[fd];
-			fda->priv[nr]	 = fda->priv[fd];
-		}
-
 		++nr;
 	}
 
-	return fda->nr = nr;
+	return nr;
 }
 
 int fdarray__poll(struct fdarray *fda, int timeout)
diff --git a/tools/perf/tests/fdarray.c b/tools/perf/tests/fdarray.c
index c7c81c4a5b2b..d0c8a05aab2f 100644
--- a/tools/perf/tests/fdarray.c
+++ b/tools/perf/tests/fdarray.c
@@ -12,6 +12,7 @@ static void fdarray__init_revents(struct fdarray *fda, short revents)
 
 	for (fd = 0; fd < fda->nr; ++fd) {
 		fda->entries[fd].fd	 = fda->nr - fd;
+		fda->entries[fd].events  = revents;
 		fda->entries[fd].revents = revents;
 	}
 }
@@ -29,7 +30,7 @@ static int fdarray__fprintf_prefix(struct fdarray *fda, const char *prefix, FILE
 
 int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_unused)
 {
-	int nr_fds, expected_fd[2], fd, err = TEST_FAIL;
+	int nr_fds, err = TEST_FAIL;
 	struct fdarray *fda = fdarray__new(5, 5);
 
 	if (fda == NULL) {
@@ -55,7 +56,6 @@ int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_
 
 	fdarray__init_revents(fda, POLLHUP);
 	fda->entries[2].revents = POLLIN;
-	expected_fd[0] = fda->entries[2].fd;
 
 	pr_debug("\nfiltering all but fda->entries[2]:");
 	fdarray__fprintf_prefix(fda, "before", stderr);
@@ -66,17 +66,9 @@ int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_
 		goto out_delete;
 	}
 
-	if (fda->entries[0].fd != expected_fd[0]) {
-		pr_debug("\nfda->entries[0].fd=%d != %d\n",
-			 fda->entries[0].fd, expected_fd[0]);
-		goto out_delete;
-	}
-
 	fdarray__init_revents(fda, POLLHUP);
 	fda->entries[0].revents = POLLIN;
-	expected_fd[0] = fda->entries[0].fd;
 	fda->entries[3].revents = POLLIN;
-	expected_fd[1] = fda->entries[3].fd;
 
 	pr_debug("\nfiltering all but (fda->entries[0], fda->entries[3]):");
 	fdarray__fprintf_prefix(fda, "before", stderr);
@@ -88,14 +80,6 @@ int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_
 		goto out_delete;
 	}
 
-	for (fd = 0; fd < 2; ++fd) {
-		if (fda->entries[fd].fd != expected_fd[fd]) {
-			pr_debug("\nfda->entries[%d].fd=%d != %d\n", fd,
-				 fda->entries[fd].fd, expected_fd[fd]);
-			goto out_delete;
-		}
-	}
-
 	pr_debug("\n");
 
 	err = 0;
Alexey Budankov June 9, 2020, 6:51 p.m. UTC | #13
On 09.06.2020 17:56, Jiri Olsa wrote:
> On Mon, Jun 08, 2020 at 08:18:20PM +0300, Alexey Budankov wrote:
> 
> SNIP
> 
>>>>>>> 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,
> 
> so with the changes I proposed it could no longer be called fdarray ;-)
> which I think was the idea at the begning.. just an array of fds
> 
> I'd like to have fully flaged events object.. but that's bigger change
> 
>>>
>>> and still making the atomic fdarray__poll()?
>>
>> How is it about design like this?
>>
>>     int fdarray__poll(struct fdarray *fda, int timeout)
>>
>> with additional external array of fds to simultaneously poll() on:
>>
>>     int fdarray__poll(struct fdarray *fda, int timeout,
>>                       int *fds, size_t fds_size)
>>
>> fds would be added to array just prior poll() call.
> 
> yep, I was considering something like this, having:
> 
>   fdarray__poll2(fda1, fda2)
>   fdarray__pollx(fda, ...)
> 
> but it would need to create an pollfd array and write
> the poll results back to arrays.. might be expensive

This is quite similar to currently proposed design options.
Saying expensive how do you estimate the cost?

> 
> another idea is to forbid filter to screw the array
> and return only remaining number, like below

Important thing with all this is to have atomic poll() on
all fds signalling to the tool in order to avoid tricky races
and complicated design covering them.

Signals can also be consumed via fd and recent pidfd kernel
extension can be subject for this general atomic poll().

So ideally it should look like this:
fdarray__poll(events_fds + ctl_fds + pid_fd + signal_fds)

~Alexey
Alexey Budankov June 15, 2020, 5:20 a.m. UTC | #14
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.

Thanks,
Alexey
Jiri Olsa June 15, 2020, 12:30 p.m. UTC | #15
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:
  https://lore.kernel.org/lkml/20200609145611.GI1558310@krava/

jirka
Alexey Budankov June 15, 2020, 1:13 p.m. UTC | #16
On 09.06.2020 17:56, Jiri Olsa wrote:
> On Mon, Jun 08, 2020 at 08:18:20PM +0300, Alexey Budankov wrote:
> 
> SNIP
> 
>>>>>>> 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,
> 
> so with the changes I proposed it could no longer be called fdarray ;-)
> which I think was the idea at the begning.. just an array of fds
> 
> I'd like to have fully flaged events object.. but that's bigger change
> 
>>>
>>> and still making the atomic fdarray__poll()?
>>
>> How is it about design like this?
>>
>>     int fdarray__poll(struct fdarray *fda, int timeout)
>>
>> with additional external array of fds to simultaneously poll() on:
>>
>>     int fdarray__poll(struct fdarray *fda, int timeout,
>>                       int *fds, size_t fds_size)
>>
>> fds would be added to array just prior poll() call.
> 
> yep, I was considering something like this, having:
> 
>   fdarray__poll2(fda1, fda2)
>   fdarray__pollx(fda, ...)
> 
> but it would need to create an pollfd array and write
> the poll results back to arrays.. might be expensive
> 
> another idea is to forbid filter to screw the array
> and return only remaining number, like below

Which option does it want to be? Just like below?
If yes then how does it distinguish event fds from the others?
This is required to return correct value from fdarray__filter().
Could you please clarify more?

Thanks,
Alexey

> 
> jirka
> 
> 
> ---
> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> index 58d44d5eee31..89f9a2193c2d 100644
> --- a/tools/lib/api/fd/array.c
> +++ b/tools/lib/api/fd/array.c
> @@ -93,22 +93,21 @@ int fdarray__filter(struct fdarray *fda, short revents,
>  		return 0;
>  
>  	for (fd = 0; fd < fda->nr; ++fd) {
> +		if (!fda->entries[fd].events)
> +			continue;
> +
>  		if (fda->entries[fd].revents & revents) {
>  			if (entry_destructor)
>  				entry_destructor(fda, fd, arg);
>  
> +			fda->entries[fd].revents = fda->entries[fd].events = 0;
>  			continue;
>  		}
>  
> -		if (fd != nr) {
> -			fda->entries[nr] = fda->entries[fd];
> -			fda->priv[nr]	 = fda->priv[fd];
> -		}
> -
>  		++nr;
>  	}
>  
> -	return fda->nr = nr;
> +	return nr;
>  }
>  
>  int fdarray__poll(struct fdarray *fda, int timeout)
> diff --git a/tools/perf/tests/fdarray.c b/tools/perf/tests/fdarray.c
> index c7c81c4a5b2b..d0c8a05aab2f 100644
> --- a/tools/perf/tests/fdarray.c
> +++ b/tools/perf/tests/fdarray.c
> @@ -12,6 +12,7 @@ static void fdarray__init_revents(struct fdarray *fda, short revents)
>  
>  	for (fd = 0; fd < fda->nr; ++fd) {
>  		fda->entries[fd].fd	 = fda->nr - fd;
> +		fda->entries[fd].events  = revents;
>  		fda->entries[fd].revents = revents;
>  	}
>  }
> @@ -29,7 +30,7 @@ static int fdarray__fprintf_prefix(struct fdarray *fda, const char *prefix, FILE
>  
>  int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_unused)
>  {
> -	int nr_fds, expected_fd[2], fd, err = TEST_FAIL;
> +	int nr_fds, err = TEST_FAIL;
>  	struct fdarray *fda = fdarray__new(5, 5);
>  
>  	if (fda == NULL) {
> @@ -55,7 +56,6 @@ int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_
>  
>  	fdarray__init_revents(fda, POLLHUP);
>  	fda->entries[2].revents = POLLIN;
> -	expected_fd[0] = fda->entries[2].fd;
>  
>  	pr_debug("\nfiltering all but fda->entries[2]:");
>  	fdarray__fprintf_prefix(fda, "before", stderr);
> @@ -66,17 +66,9 @@ int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_
>  		goto out_delete;
>  	}
>  
> -	if (fda->entries[0].fd != expected_fd[0]) {
> -		pr_debug("\nfda->entries[0].fd=%d != %d\n",
> -			 fda->entries[0].fd, expected_fd[0]);
> -		goto out_delete;
> -	}
> -
>  	fdarray__init_revents(fda, POLLHUP);
>  	fda->entries[0].revents = POLLIN;
> -	expected_fd[0] = fda->entries[0].fd;
>  	fda->entries[3].revents = POLLIN;
> -	expected_fd[1] = fda->entries[3].fd;
>  
>  	pr_debug("\nfiltering all but (fda->entries[0], fda->entries[3]):");
>  	fdarray__fprintf_prefix(fda, "before", stderr);
> @@ -88,14 +80,6 @@ int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_
>  		goto out_delete;
>  	}
>  
> -	for (fd = 0; fd < 2; ++fd) {
> -		if (fda->entries[fd].fd != expected_fd[fd]) {
> -			pr_debug("\nfda->entries[%d].fd=%d != %d\n", fd,
> -				 fda->entries[fd].fd, expected_fd[fd]);
> -			goto out_delete;
> -		}
> -	}
> -
>  	pr_debug("\n");
>  
>  	err = 0;
>
Alexey Budankov June 15, 2020, 2:37 p.m. UTC | #17
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:
>   https://lore.kernel.org/lkml/20200609145611.GI1558310@krava/

Do you want it to be implemented like in the patch posted by the link?

~Alexey

> 
> jirka
>
Jiri Olsa June 15, 2020, 4:58 p.m. UTC | #18
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:
> >   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) {
Alexey Budankov June 15, 2020, 5:38 p.m. UTC | #19
On 09.06.2020 17:56, Jiri Olsa wrote:
> On Mon, Jun 08, 2020 at 08:18:20PM +0300, Alexey Budankov wrote:
> 
> SNIP
> 

<SNIP>

> 
> another idea is to forbid filter to screw the array
> and return only remaining number, like below
> 
> jirka
> 
> 
> ---
> diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
> index 58d44d5eee31..89f9a2193c2d 100644
> --- a/tools/lib/api/fd/array.c
> +++ b/tools/lib/api/fd/array.c
> @@ -93,22 +93,21 @@ int fdarray__filter(struct fdarray *fda, short revents,
>  		return 0;
>  
>  	for (fd = 0; fd < fda->nr; ++fd) {
> +		if (!fda->entries[fd].events)

If we change it to
		if (!fda->entries[fd].revents)
and fix indices returned by fdarray__add() between fdarray__filter() then
it is possible to check fds status by the pos and process and zero its
revents prior fdarray__filter(). In this case fdarray__filter() would count
the number of fds skipping the ones with zeroed revents.
 
Looks like it solves current task avoiding explicit fds typing and even
providing some more flexibility for other fds different from event and ctl
ones. I will try to come up with a version implementing this approach.

~Alexey

> +			continue;
> +
>  		if (fda->entries[fd].revents & revents) {
>  			if (entry_destructor)
>  				entry_destructor(fda, fd, arg);
>  
> +			fda->entries[fd].revents = fda->entries[fd].events = 0;
>  			continue;
>  		}
>  
> -		if (fd != nr) {
> -			fda->entries[nr] = fda->entries[fd];
> -			fda->priv[nr]	 = fda->priv[fd];
> -		}
> -
>  		++nr;
>  	}
>  
> -	return fda->nr = nr;
> +	return nr;
>  }
>  
>  int fdarray__poll(struct fdarray *fda, int timeout)
> diff --git a/tools/perf/tests/fdarray.c b/tools/perf/tests/fdarray.c
> index c7c81c4a5b2b..d0c8a05aab2f 100644
> --- a/tools/perf/tests/fdarray.c
> +++ b/tools/perf/tests/fdarray.c
> @@ -12,6 +12,7 @@ static void fdarray__init_revents(struct fdarray *fda, short revents)
>  
>  	for (fd = 0; fd < fda->nr; ++fd) {
>  		fda->entries[fd].fd	 = fda->nr - fd;
> +		fda->entries[fd].events  = revents;
>  		fda->entries[fd].revents = revents;
>  	}
>  }
> @@ -29,7 +30,7 @@ static int fdarray__fprintf_prefix(struct fdarray *fda, const char *prefix, FILE
>  
>  int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_unused)
>  {
> -	int nr_fds, expected_fd[2], fd, err = TEST_FAIL;
> +	int nr_fds, err = TEST_FAIL;
>  	struct fdarray *fda = fdarray__new(5, 5);
>  
>  	if (fda == NULL) {
> @@ -55,7 +56,6 @@ int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_
>  
>  	fdarray__init_revents(fda, POLLHUP);
>  	fda->entries[2].revents = POLLIN;
> -	expected_fd[0] = fda->entries[2].fd;
>  
>  	pr_debug("\nfiltering all but fda->entries[2]:");
>  	fdarray__fprintf_prefix(fda, "before", stderr);
> @@ -66,17 +66,9 @@ int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_
>  		goto out_delete;
>  	}
>  
> -	if (fda->entries[0].fd != expected_fd[0]) {
> -		pr_debug("\nfda->entries[0].fd=%d != %d\n",
> -			 fda->entries[0].fd, expected_fd[0]);
> -		goto out_delete;
> -	}
> -
>  	fdarray__init_revents(fda, POLLHUP);
>  	fda->entries[0].revents = POLLIN;
> -	expected_fd[0] = fda->entries[0].fd;
>  	fda->entries[3].revents = POLLIN;
> -	expected_fd[1] = fda->entries[3].fd;
>  
>  	pr_debug("\nfiltering all but (fda->entries[0], fda->entries[3]):");
>  	fdarray__fprintf_prefix(fda, "before", stderr);
> @@ -88,14 +80,6 @@ int test__fdarray__filter(struct test *test __maybe_unused, int subtest __maybe_
>  		goto out_delete;
>  	}
>  
> -	for (fd = 0; fd < 2; ++fd) {
> -		if (fda->entries[fd].fd != expected_fd[fd]) {
> -			pr_debug("\nfda->entries[%d].fd=%d != %d\n", fd,
> -				 fda->entries[fd].fd, expected_fd[fd]);
> -			goto out_delete;
> -		}
> -	}
> -
>  	pr_debug("\n");
>  
>  	err = 0;
>
Jiri Olsa June 17, 2020, 9:27 a.m. UTC | #20
On Mon, Jun 15, 2020 at 06:58:04PM +0200, Jiri Olsa wrote:
> 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:
> > >   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 ;-)

Friendly reminder.

jirka

> 
> 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
>
Alexey Budankov June 17, 2020, 9:39 a.m. UTC | #21
On 17.06.2020 12:27, Jiri Olsa wrote:
> On Mon, Jun 15, 2020 at 06:58:04PM +0200, Jiri Olsa wrote:
>> 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:
>>>>   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 ;-)
> 
> Friendly reminder.

Please see v8: https://lore.kernel.org/lkml/0781a077-aa82-5b4a-273e-c17372a72b93@linux.intel.com/

~Alexey
Alexey Budankov June 22, 2020, 9:47 a.m. UTC | #22
On 15.06.2020 19:58, Jiri Olsa wrote:
> 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:
>>>   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

Well, epoll() is perhaps possible but why does it want switching to epoll()?
What are the benefits and/or specific task being solved by this switch? 

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

Your first patch accomodated in v8 actually avoids fds typing
and solves pos (=fdarray__add()) staleness issue with fdarray.

Thanks,
Alexey

> 
> 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) {
>
Jiri Olsa June 22, 2020, 10:21 a.m. UTC | #23
On Mon, Jun 22, 2020 at 12:47:19PM +0300, Alexey Budankov wrote:

SNIP

> >>>>>> 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:
> >>>   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
> 
> Well, epoll() is perhaps possible but why does it want switching to epoll()?
> What are the benefits and/or specific task being solved by this switch? 

epoll change fixes the same issues as the patch you took in v8

on top of it it's not a hack and wil make polling more user
friendly because of the clear interface

> 
> > 
> > there might be some loose ends (interface change), but
> > I think this would solve our problems with fdarray
> 
> Your first patch accomodated in v8 actually avoids fds typing
> and solves pos (=fdarray__add()) staleness issue with fdarray.

yea, it was a change meant for discussion (which never happened),
and I considered it to be more a hack than a solution

I suppose we can live with that for a while, but I'd like to
have clean solution for polling as well

jirka
Alexey Budankov June 22, 2020, 10:50 a.m. UTC | #24
On 22.06.2020 13:21, Jiri Olsa wrote:
> On Mon, Jun 22, 2020 at 12:47:19PM +0300, Alexey Budankov wrote:
> 
> SNIP
> 
>>>>>>>> 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:
>>>>>   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
>>
>> Well, epoll() is perhaps possible but why does it want switching to epoll()?
>> What are the benefits and/or specific task being solved by this switch? 
> 
> epoll change fixes the same issues as the patch you took in v8
> 
> on top of it it's not a hack and wil make polling more user
> friendly because of the clear interface

Clear. The opposite thing is /proc/sys/fs/epoll/max_user_watches limit that
will affect Perf tool usage additionally to the current process limit on 
a number of simultaneously open file descriptors (ulimit -n). So move to 
epoll() will impose one limit what can affect Perf tool scalability.

> 
>>
>>>
>>> there might be some loose ends (interface change), but
>>> I think this would solve our problems with fdarray
>>
>> Your first patch accomodated in v8 actually avoids fds typing
>> and solves pos (=fdarray__add()) staleness issue with fdarray.
> 
> yea, it was a change meant for discussion (which never happened),
> and I considered it to be more a hack than a solution
> 
> I suppose we can live with that for a while, but I'd like to
> have clean solution for polling as well

I wouldn't treat it as a hack but more as a fix because returned
pos is now a part of interface that can be safely used in callers.
Can we go with this fix for the patch set?

Thanks,
Alexey
Jiri Olsa June 22, 2020, 12:11 p.m. UTC | #25
On Mon, Jun 22, 2020 at 01:50:03PM +0300, Alexey Budankov wrote:
> 
> On 22.06.2020 13:21, Jiri Olsa wrote:
> > On Mon, Jun 22, 2020 at 12:47:19PM +0300, Alexey Budankov wrote:
> > 
> > SNIP
> > 
> >>>>>>>> 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:
> >>>>>   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
> >>
> >> Well, epoll() is perhaps possible but why does it want switching to epoll()?
> >> What are the benefits and/or specific task being solved by this switch? 
> > 
> > epoll change fixes the same issues as the patch you took in v8
> > 
> > on top of it it's not a hack and wil make polling more user
> > friendly because of the clear interface
> 
> Clear. The opposite thing is /proc/sys/fs/epoll/max_user_watches limit that
> will affect Perf tool usage additionally to the current process limit on 
> a number of simultaneously open file descriptors (ulimit -n). So move to 
> epoll() will impose one limit what can affect Perf tool scalability.

hum, I dont think this will be a problem:

    Allowing top 4% of low memory (per user) to be allocated in epoll watches,
    we have:

    LOMEM    MAX_WATCHES (per user)
    512MB    ~178000
    1GB      ~356000
    2GB      ~712000

my laptop has 19841945 allowed watches per user

> 
> > 
> >>
> >>>
> >>> there might be some loose ends (interface change), but
> >>> I think this would solve our problems with fdarray
> >>
> >> Your first patch accomodated in v8 actually avoids fds typing
> >> and solves pos (=fdarray__add()) staleness issue with fdarray.
> > 
> > yea, it was a change meant for discussion (which never happened),
> > and I considered it to be more a hack than a solution
> > 
> > I suppose we can live with that for a while, but I'd like to
> > have clean solution for polling as well
> 
> I wouldn't treat it as a hack but more as a fix because returned
> pos is now a part of interface that can be safely used in callers.
> Can we go with this fix for the patch set?

apart from this one I still have a problem with that stat factoring
having 1 complicated function deal with both fork and no fork processing,
which I already commented on, but you ignored ;-)

I'll try to go through that once more, and post some comments

jirka
Alexey Budankov June 22, 2020, 2:04 p.m. UTC | #26
On 22.06.2020 15:11, Jiri Olsa wrote:
> On Mon, Jun 22, 2020 at 01:50:03PM +0300, Alexey Budankov wrote:
>>
>> On 22.06.2020 13:21, Jiri Olsa wrote:
>>> On Mon, Jun 22, 2020 at 12:47:19PM +0300, Alexey Budankov wrote:
>>>
>>> SNIP
>>>
>>>>>>>>>> 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:
>>>>>>>   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
>>>>
>>>> Well, epoll() is perhaps possible but why does it want switching to epoll()?
>>>> What are the benefits and/or specific task being solved by this switch? 
>>>
>>> epoll change fixes the same issues as the patch you took in v8
>>>
>>> on top of it it's not a hack and wil make polling more user
>>> friendly because of the clear interface
>>
>> Clear. The opposite thing is /proc/sys/fs/epoll/max_user_watches limit that
>> will affect Perf tool usage additionally to the current process limit on 
>> a number of simultaneously open file descriptors (ulimit -n). So move to 
>> epoll() will impose one limit what can affect Perf tool scalability.
> 
> hum, I dont think this will be a problem:
> 
>     Allowing top 4% of low memory (per user) to be allocated in epoll watches,
>     we have:
> 
>     LOMEM    MAX_WATCHES (per user)
>     512MB    ~178000
>     1GB      ~356000
>     2GB      ~712000
> 
> my laptop has 19841945 allowed watches per user
> 
>>
>>>
>>>>
>>>>>
>>>>> there might be some loose ends (interface change), but
>>>>> I think this would solve our problems with fdarray
>>>>
>>>> Your first patch accomodated in v8 actually avoids fds typing
>>>> and solves pos (=fdarray__add()) staleness issue with fdarray.
>>>
>>> yea, it was a change meant for discussion (which never happened),
>>> and I considered it to be more a hack than a solution
>>>
>>> I suppose we can live with that for a while, but I'd like to
>>> have clean solution for polling as well
>>
>> I wouldn't treat it as a hack but more as a fix because returned
>> pos is now a part of interface that can be safely used in callers.
>> Can we go with this fix for the patch set?
> 
> apart from this one I still have a problem with that stat factoring
> having 1 complicated function deal with both fork and no fork processing,
> which I already commented on, but you ignored ;-)

Not an issue at all, lets split that func, dispatch_events() I suppose,
as you see it.

> 
> I'll try to go through that once more, and post some comments
> 
> jirka
> 

Thanks,
Alexey
Jiri Olsa June 23, 2020, 2:54 p.m. UTC | #27
On Mon, Jun 22, 2020 at 05:04:21PM +0300, Alexey Budankov wrote:

SNIP

> >>>>> there might be some loose ends (interface change), but
> >>>>> I think this would solve our problems with fdarray
> >>>>
> >>>> Your first patch accomodated in v8 actually avoids fds typing
> >>>> and solves pos (=fdarray__add()) staleness issue with fdarray.
> >>>
> >>> yea, it was a change meant for discussion (which never happened),
> >>> and I considered it to be more a hack than a solution
> >>>
> >>> I suppose we can live with that for a while, but I'd like to
> >>> have clean solution for polling as well
> >>
> >> I wouldn't treat it as a hack but more as a fix because returned
> >> pos is now a part of interface that can be safely used in callers.
> >> Can we go with this fix for the patch set?
> > 
> > apart from this one I still have a problem with that stat factoring
> > having 1 complicated function deal with both fork and no fork processing,
> > which I already commented on, but you ignored ;-)
> 
> Not an issue at all, lets split that func, dispatch_events() I suppose,
> as you see it.

ok,I checked it one more time and perhaps the function naming
was confusing for me.. but maybe we can give it another try,
I'm sending some comments

thanks,
jirka

Patch
diff mbox series

diff --git a/tools/lib/api/fd/array.c b/tools/lib/api/fd/array.c
index 58d44d5eee31..b0027f2169c7 100644
--- a/tools/lib/api/fd/array.c
+++ b/tools/lib/api/fd/array.c
@@ -11,10 +11,16 @@ 
 
 void fdarray__init(struct fdarray *fda, int nr_autogrow)
 {
+	int i;
+
 	fda->entries	 = NULL;
 	fda->priv	 = NULL;
 	fda->nr		 = fda->nr_alloc = 0;
 	fda->nr_autogrow = nr_autogrow;
+
+	fda->nr_stat = 0;
+	for (i = 0; i < FDARRAY__STAT_ENTRIES_MAX; i++)
+		fda->stat_entries[i].fd = -1;
 }
 
 int fdarray__grow(struct fdarray *fda, int nr)
@@ -83,6 +89,20 @@  int fdarray__add(struct fdarray *fda, int fd, short revents)
 	return pos;
 }
 
+int fdarray__add_stat(struct fdarray *fda, int fd, short revents)
+{
+	int pos = fda->nr_stat;
+
+	if (pos >= FDARRAY__STAT_ENTRIES_MAX)
+		return -1;
+
+	fda->stat_entries[pos].fd = fd;
+	fda->stat_entries[pos].events = revents;
+	fda->nr_stat++;
+
+	return pos;
+}
+
 int fdarray__filter(struct fdarray *fda, short revents,
 		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
 		    void *arg)
@@ -113,7 +133,27 @@  int fdarray__filter(struct fdarray *fda, short revents,
 
 int fdarray__poll(struct fdarray *fda, int timeout)
 {
-	return poll(fda->entries, fda->nr, timeout);
+	int nr, i, pos, res;
+
+	nr = fda->nr;
+
+	for (i = 0; i < fda->nr_stat; i++) {
+		if (fda->stat_entries[i].fd != -1) {
+			pos = fdarray__add(fda, fda->stat_entries[i].fd,
+					   fda->stat_entries[i].events);
+			if (pos >= 0)
+				fda->priv[pos].idx = i;
+		}
+	}
+
+	res = poll(fda->entries, fda->nr, timeout);
+
+	for (i = nr; i < fda->nr; i++)
+		fda->stat_entries[fda->priv[i].idx] = fda->entries[i];
+
+	fda->nr = nr;
+
+	return res;
 }
 
 int fdarray__fprintf(struct fdarray *fda, FILE *fp)
diff --git a/tools/lib/api/fd/array.h b/tools/lib/api/fd/array.h
index b39557d1a88f..9bca72e80b09 100644
--- a/tools/lib/api/fd/array.h
+++ b/tools/lib/api/fd/array.h
@@ -3,6 +3,7 @@ 
 #define __API_FD_ARRAY__
 
 #include <stdio.h>
+#include <poll.h>
 
 struct pollfd;
 
@@ -16,6 +17,9 @@  struct pollfd;
  *	  I.e. using 'fda->priv[N].idx = * value' where N < fda->nr is ok,
  *	  but doing 'fda->priv = malloc(M)' is not allowed.
  */
+
+#define FDARRAY__STAT_ENTRIES_MAX	1
+
 struct fdarray {
 	int	       nr;
 	int	       nr_alloc;
@@ -25,6 +29,8 @@  struct fdarray {
 		int    idx;
 		void   *ptr;
 	} *priv;
+	int	       nr_stat;
+	struct pollfd  stat_entries[FDARRAY__STAT_ENTRIES_MAX];
 };
 
 void fdarray__init(struct fdarray *fda, int nr_autogrow);
@@ -34,6 +40,7 @@  struct fdarray *fdarray__new(int nr_alloc, int nr_autogrow);
 void fdarray__delete(struct fdarray *fda);
 
 int fdarray__add(struct fdarray *fda, int fd, short revents);
+int fdarray__add_stat(struct fdarray *fda, int fd, short revents);
 int fdarray__poll(struct fdarray *fda, int timeout);
 int fdarray__filter(struct fdarray *fda, short revents,
 		    void (*entry_destructor)(struct fdarray *fda, int fd, void *arg),
diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
index 6a875a0f01bb..e68e4c08e7c2 100644
--- a/tools/lib/perf/evlist.c
+++ b/tools/lib/perf/evlist.c
@@ -317,6 +317,17 @@  int perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd,
 	return pos;
 }
 
+int perf_evlist__add_pollfd_stat(struct perf_evlist *evlist, int fd,
+			         short revent)
+{
+	int pos = fdarray__add_stat(&evlist->pollfd, fd, revent | POLLERR | POLLHUP);
+
+	if (pos >= 0)
+		fcntl(fd, F_SETFL, O_NONBLOCK);
+
+	return pos;
+}
+
 static void perf_evlist__munmap_filtered(struct fdarray *fda, int fd,
 					 void *arg __maybe_unused)
 {
diff --git a/tools/lib/perf/include/internal/evlist.h b/tools/lib/perf/include/internal/evlist.h
index 74dc8c3f0b66..2b3b4518c05e 100644
--- a/tools/lib/perf/include/internal/evlist.h
+++ b/tools/lib/perf/include/internal/evlist.h
@@ -46,6 +46,8 @@  struct perf_evlist_mmap_ops {
 int perf_evlist__alloc_pollfd(struct perf_evlist *evlist);
 int perf_evlist__add_pollfd(struct perf_evlist *evlist, int fd,
 			    void *ptr, short revent);
+int perf_evlist__add_pollfd_stat(struct perf_evlist *evlist, int fd,
+			         short revent);
 
 int perf_evlist__mmap_ops(struct perf_evlist *evlist,
 			  struct perf_evlist_mmap_ops *ops,