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

[Alexey Budankov]

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@xxxxxxxxxxxxxxx>
>>>>>>>> ---
>>>>>>>>  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
>