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

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

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