[RFC] [WIP] tevent/glib integration

[Noel Power]

Hi Ralf
On 11/01/16 11:18, Ralph Boehme wrote:
> Hi Noel,
>
> On Tue, Jan 05, 2016 at 05:36:42PM +0000, Noel Power wrote:
>> Sorry for the delay in getting back (only returned from vacation today
>> and climbing the mail mountain)
> no prob, currently on vacation myself. :) If you like, we can further
> discuss this on the phone/tmate next week when I'm back.
sure, and enjoy the rest of your vacation
>
>>>>> I believe you meant the TEVENT_TRACE_BEFORE_LOOP_ONCE and
>>>>> TEVENT_TRACE_BEFORE_AFTER_ONCE trace points.
>>>> I also looked at using those but they stretch the scope of the
>>>> prepare_poll/check_poll much much longer than I wanted. e.g. in this
>>>> specific implementation case the glib context being acquired and held
>>>> for the entire event_loop_once call. I was purposely trying to keep that
>>>> to a minimum (and additionally now with your patch  the glib context
>>>> could be held even while normal tevent events are processed, I don't
>>>> know if that specifically can cause a problem but it looked like it
>>>> might have potential for unwanted/unexpected sideaffects).
>>> I don't see a problem and it greatly simplifies code. It will block
>>> other glib threads from acquiring and using the context and that's
>>> probably a good idea anyway.
>> Acquiring the context acts like a mutex doesn't it ? and that is why I
>> think acquiring the context for the duration of the whole 'loop_once'
>> function is not a good idea, surely it makes more sense to hold and
>> acquire the context for as little time as possible.
> Sure, but keeping it locked across across loop_once is good enough imo.
I disagree
>
>> Additionally allowing the context to be held acquired when a normal
>> tevent fires (meaning there is certainly no glib event available)
>> doesn't make sense to me at all.
> I don't see a problem in it.
well you have no idea how long a tevent callback will last, it could be
some long synchronous call, I'd prefer to depend on and make little to
no assumptions about how either loop behaves that's why I favour just
using the tevent to hook into the poll function and pass the results to
glib (or whatever loop)
>
>> Also I suppose also I still hold onto the idea of a more
>> generic solution (where glib support is built on a generic mechanism for
>> integrating foreign_fds (and thus any foreign loop) into tevent) so I
>> don't want to make any assumptions about what the
>> process_poll/check_poll functions actually do, or what resources they
>> may hold
>> Making that sequence as short as possible is easily achieved, it just
>> means not using TEVENT_TRACE_(BEFORE | AFTER) as the mechanism to call
>> the 'process_poll' & 'check_poll' function but instead exercising finer
>> grained control by calling them directly at the best call site (close to
>> the poll call), I wouldn't call that making things more the code more
>> complex I actually think it makes it simpler as it's obvious what is
>> called when.
> Well, yes. But otoh when using the trace points the only modification
> in loop_once is checking the foreign loop timeout. And we'll have to
> touch at least three backends, so I still believe it's worthwhile
> having the loop_once modification as simple as possible.
I guess I am influenced by the fact that I think you need to hold the
context (and perhaps other resources for a different event loop) for as
short as time as possible, additionally I think you need to change code
in the backends anyway to deal with HUP and ERR (and transfer at least
timeout info) I think the extra loop_once modifications wouldn't be that
bad
>
>>>> If it was possible to ignore the points above (and maybe it is, perhaps
>>>> I am being too careful or extreme) using the trace points imo only makes
>>>> sense if it is possible to use them in external code, by external code I
>>>> mean code that links to the tevent library and uses the public headers
>>>> available from tevent. In your code the 'prepare_poll' and 'check_poll'
>>>> code is internal to the 's3' tevent backend and thus for example the
>>>> timeout information can be easily transferred. But.. how could the code
>>>> called by TEVENT_TRACE_BEFORE and TEVENT_TRACE_BEFORE transfer timeout
>>>> information to generic 'tevent' backend event_loop_once function(s),
>>> tevent_get_foreign_poll_ctx(). That's the only modification necessary
>>> in s3_event_loop_once(). Call tevent_get_foreign_poll_ctx() to get at
>>> the timeout of the foreign loop.
>> sorry, I am probably not explaining myself well enough, here I am
>> thinking about a generic solution, imagine for example you want to
>> implement adding such foreign fds (maybe even glib integration) to one
>> (or all) of the existing tevent backends, in an external application you
>> can easily set up the TEVENT_TRACE_BEFORE | AFTER to call some functions
>> to add/remove some fds etc. as you have already done but how can you
>> influence the timeout say in tevent_poll.c
> In the prepare functions the external loop stores the timeout in
> tevent_foreign_loop_ctx.timeout. In tevent loop_once fetch the
> tevent_foreign_loop_ctx, read the timeout from there.
I guess I am not sure what you finally intend the exposure and
visibility (and how public) some these structures will be, perhaps I
just need to reread things to de-confuse myself
>
> Also 'run_events_poll besides' not
> being aware of the FOREIGN_FD_XYZ flags additionally doesn't monitor
> POLL or ERR flags (and glib for example at least in the api wants to
> have the possibility to register interest in those events) It's true
> that the example (as you have seen) and perhaps even libtracker-sparql
> in general doesn't need to know about POLL or ERR events (I wouldn't
> like to bet on that though)
>>> Maybe it's time to add TEVENT_FD_HUP and TEVENT_FD_ERR to tevent. The
>>> caller of tevent_add_fd() could signalize interest in these events and
>>> tevent would pass the corresponding flag to the callback in case the
>>> event (ie POLLHUP, POLLER) was triggered.
>> sure, I wanted to and thought about adding TEVENT_FD_HUP and
>> TEVENT_FD_ERR however if you look at each of the existing tevent
>> backends there seems quite some intentional and custom code for
>> 'non-handling' of these events (converting them and/or ignoring them). I
>> did not feel confident enough to make changes around that stuff and that
>> is the reason I introduced the FOREIGN_XYZ event stuff, that's not to
>> say I like it though :-(
> I've updated my branch with a patch that adds
> TEVENT_FOREIGN_FD_ERR|HUP to tevent_internal.h.
>
> <https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/tevent-glib-spotlight>
> <https://git.samba.org/?p=slow/samba.git;a=commit;h=8e7e600552acff46d5b3e0ffc2fcd241bd8bf402>
At a quick look I see where you are going but these changes don't
prevent, POLL & ERR events already getting converted at
https://git.samba.org/?p=slow/samba.git;a=blob;f=lib/tevent/tevent_poll.c;h=2ea5e68fe9d106efb6a236ab0cd982ee2c37f235;hb=8e7e600552acff46d5b3e0ffc2fcd241bd8bf402#l579
(presumably the same for the other backends)

And you notice you no are already modifying the loop_once function here
introducing special code around handling TEVENT_FOREIGN_FD, (I actually
don't think you can get away without doing that) so I think although a
lofty goal

> Well, yes. But otoh when using the trace points the only modification
> in loop_once is checking the foreign loop timeout. And we'll have to
> touch at least three backends, so I still believe it's worthwhile
> having the loop_once modification as simple as possible
the sentiments above are not easily achievable and it's one of the
reasons why I suggest making the changes outside of TEVENT_BEFORE/AFTER
is just as easy (and certainly more flexible)

I tried the approach of adding (locally) TEVENT_FD_HUP  TEVENT_FD_ERR (I
tried to keep the more controversial commits separate) 
http://cgit.freedesktop.org/~noelp/noelp-samba/log/?h=ralph%2bme%2bglib-spotlight
as an interim step
>
> By using the new defines adding the foreign loop integration to all
> relevant backends would be straight forward.
if you don't get it right you get 'unexpected' behaviour, this is why I
tried hard not to make assumptions and pass things directly to the
foreign loop (glib in this case) in a way as close to what that loop
expects see.
http://cgit.freedesktop.org/~noelp/noelp-samba/commit/?h=ralph%2bme%2bglib-spotlight&id=aaafb95f3487ccfbb5974f906ca9c1f8aaaa9c77
and 
http://cgit.freedesktop.org/~noelp/noelp-samba/commit/?h=ralph%2bme%2bglib-spotlight&id=37c9f2f4b9bc2ff975fcb03db2fb5b3a0f5795ff
>
>>>> But like I mentioned earlier I would like to see the relevant
>>>> results from the poll function for foreign fds processed separately,
>>>> ...
>>> Hm, I see no need for that.
>> see below
>>>>>> e.g. in the s3
>>>>>> tevent backend at least 'event_add_to_poll_args' and maybe even
>>>>>> 'run_events_poll'. In anycase I would prefer to process the foreign
>>>>>> events separately for 2 reasons and that activity also needs to be done
>>>>>> inbetween the 'BEFORE' and 'AFTER' trace points. My reasons for
>>>>>> processing *all* foreign fds at once are
>>>>>>    a) I want the behaviour and results of the poll to be as close as
>>>>>> possible to what the external code would expect if it polled the fds
>>>>>> itself, that means if events for more than one fd are available I want
>>>>>> them all and not one at a time
>>>>>>    b) I think it could be wise to ensure the sequence (add foreign fds,
>>>>>> poll foreign fds, report foreign fd poll results, react to foreign fds)
>>>>>> happens all together without normal 'tevent' events happening inbetween
>>>>> See attached patch, I think the glib loop is only entered once in
>>>>> s3_tevent_foreign_loop_process().
>>>> not sure what you mean, what I meant was I wanted to avoid the following
>>>> scenario
>>>>
>>>> prepare_poll (add some glib fds)
>>>> poll  (some glib fds have events reported by poll)
>>>> check_poll (is called but doesn't receive all the available events
>>>> available from the glib fds because)
>>>>
>>>>    a) some tevent has happened e.g. a normal fd monitored by tevent, a
>>>> tevent timeout or a tevent signal
>>>>    b) only a single glib fd event is available (because tevent loop only
>>>> processes a single fd at a time) but the poll actually reported multiple
>>>> glib fds have available events
>>>>
>>>> Instead I wanted glib (or whatever foreign loop) to get the same results
>>>> as if they called the poll function themselves. I know perhaps this is
>>>> breaking the semantics of the tevent loop_once function but I look at
>>>> this as something as 'outside' of tevent, I'm not really looking to
>>>> impose tevent behaviour around polling foreign fds but rather just
>>>> gather the results from whatever 'poll/select/epoll' function the 
>>>> tevent backend is using and pass that on to whatever integration code is
>>>> plugged in.
>>> Not convinced. :) Both the tevent loop and the glib loop poll event
>>> sources and run event handlers that must support being called in any
>>> order. Adding tevent event handlers to the mix doesn't change the
>>> requirements for glib event handlers.
>> It's not about ordering per se (aside from concerns re. holding the
>> context either too long or holding it unnecessarily while a non-glib
>> fd,timer or signal event is serviced) it's more about if you don't
>> process the glib tevent_fds separately then the net effect is that it
>> throttles the glib loop event handing, at best only one available glib
>> fd event per loop iteration can be serviced (and that is if there are
>> *no* normal non-glib tevent events to be processed),
> Hm, that's a problem. Indeed, glib processes all pending event sources
> while tevent just processes one, favouring immediate and signal events
> over fds.
>
> metze, is there a reason for only processing one pending event source
> at a time in tevent?
well I think the documentation is explicit enough in that loop_once
processes one single tevent
https://tevent.samba.org/tevent_context.html. This is why I did say
originally about my approach

"I know perhaps this is breaking the semantics of the tevent loop_once function but I look at
this as something as 'outside' of tevent, I'm not really looking to
impose tevent behaviour around polling foreign fds but rather just
gather the results from whatever 'poll/select/epoll' function the 
tevent backend is using and pass that on to whatever integration code is
plugged in."

I still believe in that approach, I also think it should be possible to
make it less ugly than my clumsy attempts
>
>> [1] - I did some very very cheap and nasty testing
>> ...
> As expected, this shows that proessing only one ready event source per
> loop_once iteration has a larger overhead.
>
yup, and I think you can appreciate that in a samba process where more
'tevent' events will be in the loop that the affects will get even worse

thanks,
Noel