Messages in this thread | | | Subject | Re: [PATCH v7 01/13] tools/libperf: introduce notion of static polled file descriptors | From | Alexey Budankov <> | Date | Mon, 8 Jun 2020 19:43:30 +0300 |
| |
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 >
| |