| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 8 Jun 2020 19:43:30 +0300
From: Alexey Budankov <alexey.budankov@...ux.intel.com>
To: Jiri Olsa <jolsa@...hat.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Andi Kleen <ak@...ux.intel.com>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v7 01/13] tools/libperf: introduce notion of static polled
file descriptors
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@...ux.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
>
Powered by blists - more mailing lists