[RFC] [WIP] tevent/glib integration

[Ralph Boehme]

Hi Noel,

On Mon, Jan 25, 2016 at 07:07:41PM +0000, Noel Power wrote:
> On 22/01/16 15:56, Ralph Boehme wrote:
> > On Mon, Jan 11, 2016 at 12:56:12PM +0000, Noel Power wrote:
> >> ... long snip ...
> > ok, so we both agree that both solutions are ugly. ;)
> >
> > Metze had a better idea in a private conversation: use epoll to
> > create an epoll instance and add the returned fd to tevent via
> > tevent_add_fd(). Later add all glib fds to the new epoll instance
> > via epoll_ctl(). epoll_wait() will then return all glib fds with
> > events pending. This can all be done on top of tevent.
>
> I'm am/was little unsure what you mean't, I've never used epoll
> myself, reading a bit I was interested to see that it is possible to
> add the returned_fd from an epoll instance A to another epoll
> instance B, presumably the epoll instance B that got the epoll
> instance fd will itself monitor the fds associated with A, is that
> correct?,

yes.

> so this means it will work only when 'epoll' is the default tevent
> backend ?

no, Q3 in http://linux.die.net/man/4/epoll.

> or.... does epoll instance A not actually need anybody to
> call epoll_wait for it's fd(s) to be monitored, meaning you can just
> pass an epoll instance fd to select or poll and get an event on the
> epoll instance fd if any of the fd(s) monitored by that instance has
> an available event,

yes.

> so... if I get it right that is a great idea and
> offers a really cool way to separate and distinguish the glib from
> normal tevent_fds

yes, but after thinking about it, it really doesn't buy us anything ^w
much (performance and code quality wise) compared to the current
implementation that caches tevent_fds.

It would save us maintaing the tevent_fd_map array and some overhead
for creating and managing tevent_fd instances inside tevent (with
epoll it would just be syscall to epoll_ctl(add fd)).

Otoh creating and deleting fd event sources is a rare event, eg
running the teven_glib utitility results in two creations and two
deletions.

> > In case epoll is not available, in the glue code as a fallback use a
> > tevent_fd per glib fd and in the handler call poll() a second time on
> > the glib fds. That way we get the raw revents and can handle all
> > pending events in one swoop.
> >
> > This would work without any modifications to the tevent code.
> >
> > I have a wip branch here:
> > <https://git.samba.org/?p=slow/samba.git;a=log;h=refs/heads/tevent-glib-glue>
> >
> > This just adds the fallback method without any optimisations,
> > particulariy epoll is left out.
>
> ok, I had a look at least at the fallback code, this is really really
> clever and self contained, the use of the timers and and even the second
> poll is very neat.

Testing and close review is highly appreciated!

> But doesn't it still suffer from the same problem of being throttled by
> tevent(s) one event processed per loop iteration policy?
> e.g. it depends on either a timer or one of the glib fd(s) (with
> available events) being monitored to fire, but any number of ready
> non-glib related timers or fds could be queued for processing before and
> in the s3 case a tevent_fd that fires is put to the back of the list
> after firing :/.

timer, immediate and signal events are rare compared to fd events, so
I wouldn't worry about those.

fd event sources are maintained in a list in tevent. When checking
pending fd events the list is walked from the start and the first
pending event source is processed and then migrated to the end of the
list. This prevents us from a single fd starving other event sources.

And when one of the glib fds is active, we process all pending glibs
*and* we keep polling the g_main_context until the returned timeout is
greater then 0 and g_main_context_check() returns false.

> So I still fear it could quite easily take quite a few
> tevent_loop iterations (and that situation worsening as more & more
> normal events & timers are added by a samba process) before available
> glib events will get processed and the glib context released, granted
> when it does get processed it does process *all* glib events which is a
> significant improvement. Presumably this would be the same with the
> extra epoll source tacked on.  I don't know :/ in a busy smbd process
> for example glib won't afaics get a fair slice of the action,

I'm quite confident it well for the reasons described above and I'm
willing to test-drive it with the Spotlight RPC server. :)

> I don't
> see any wriggle room for improving things if that is the case.  At least
> if you process them in the ugly glib-fds & normal tevent fds together
> then glib processing doesn't affect tevent's own event processing and
> vice-versa. Each event loop is getting a fair share of the event
> processing action. Anyway that's my worry about this, how it will scale,
> I suppose you don't have the same opinion (and I'd even reluctantly
> sacrifice a generic solution if it meant in s3/lib for example would
> have that flexibility)
> 
> >
> > The good:
> > - completely on top of tevent
> >
> > The bad:
> > - calls poll() twice (once in tevent, once in the glue code)
> [...]
> > This patch puts the patch into tevent itself, we could also put it
> > into source3/lib/.
> well, I guess source3/lib is where I would have started because of
> spotlight (and my toy), but when you will use your approach above you
> won't need to do anything with s3

This was more about whether tevent upstream (yuck, that's us :)) is
willing to include this new feature in tevent or whether we maintain
it outside. I think it's a nice addition to tevent so it would be nice
to have it in, but maybe we want to give it some time and testing
outside before including it.

> On 25/01/16 16:49, Ralph Boehme wrote:
> > updated the branch. Two changes:
> >
> > - cache tevent_fds
> >
> > - run g_main_context_check/g_main_context_dispatch in a loop until
> >   g_main_context_check returns false
> >
> > With these changes the test binary completes as fast with the tevent
> > glue as with the native glib loop. 
> 
> well, that's not what I encountered previously where tevent was a little
> (but not much) slower,  now in fact on my machine the test is a little
> (but consistently) faster than with native glib and that doesn't sound
> right,  I am suspicious of
> 
>   >- run g_main_context_check/g_main_context_dispatch in a loop until
> g_main_context_check returns false'
> 
> that sounds suspiciously like you are artificially tweaking the priority
> (in other word the priority passed into g_main_context_check probably
> determines whether an event source is marked as ready for dispatch or
> not) if that is the case I'd say you are playing with fire,  I bet
> without the change above the performance sucks ?

no it doesn't suck, it just takes three times as long.

The plan was to fix this short circuiting anyway, by calling the
tevent_glib_prepare(). I've updated the branch with this change, it
now takes twice as long as native glib (~0.8 ms compared to ~0.4 ms on
my system where the tracker SPARQL query got ~220 results).

-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