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

[Alexey Budankov <alexey.budankov@linux.intel.com>]

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