[RFC] [WIP] tevent/glib integration

[Ralph Boehme]

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.

> >>> 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.

> 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.

> 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.

> >  Not sure, but maybe we should acquire the
> > context once at the beginning and never release it.
> 
> I don't get why you would want to acquire the context and never release
> it, ...

Nevermind, wooly thinking on my side. ;)

> >> 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.

> >>>> + imho it is not at all an intuitive api to use, that the fds of the
> >>>> poll function could be modified by these trace points isn't at all
> >>>> obvious even by name
> >>> afaict we're not modifyint the pollfds, are we?
> >> well not directly but indirectly we are, calling tevent_add_fd
> >> essentially is the same as modifying the pollfds isn't it ?
> > No. Using tevent_add_fd is a well defined interface. Directly hacking
> > the pollfd array is a completely different game.
> umm I am not sure what you mean here still, all I meant was that looking
> at the 'loop_once' function it is not clear that EVENT_TRACE_BEFORE and
> TEVENT_TRACE_BEFORE are influencing the fds that the poll function is
> using in addition to a timeout override that seems disconnected from the
> surrounding code. ok. granted comments will help.
> Maybe we can't agree here, opinions differ, you might call it elegant
> design and I might call it confusing :-) But then again I am easily
> confused :-),I like things spelt out nice and clearly

meeeeeeetzeeeeee, what would you prefer? :)

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

By using the new defines adding the foreign loop integration to all
relevant backends would be straight forward.

> >> 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?

> [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.

-Ralph

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de,mailto:kontakt@sernet.de