[RFC] [WIP] tevent/glib integration

[Noel Power]

Hi Ralph,

Sorry for the delay in getting back (only returned from vacation today
and climbing the mail mountain)

On 29/12/15 00:53, Ralph Boehme wrote:
> Hi Noel,
>
> On Mon, Dec 28, 2015 at 11:01:22AM +0000, Noel Power wrote:
>> On 22/12/15 19:55, Ralph Boehme wrote:
>>> Hi Noel,
>>>
>>> On Mon, Dec 21, 2015 at 03:07:36PM +0000, Noel Power wrote:
>>>> On 16/12/15 14:08, Stefan Metzmacher wrote:
>>>>> very interesting work! It would be really good to have this
>>>>> problem solved cleanly in future.
>>>> [...]
>>>>> Instead we can use a tracecallback and hook into the
>>>>> TEVENT_TRACE_BEFORE_WAIT (and maybe TEVENT_TRACE_AFTER_WAIT)
>>>>> events.
>>> 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. 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. 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.
>  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, from reading the glib documentation it seems that it would not be
unusual for a glib library to use worker threads that each use their own
gmain loop and associated thread context and dispatch the final results
(callbacks) to the main/default gmain loop right ? (maybe I have the
wrong end of the stick here though, like I said my glib knowledge is
well.... not great)

>> In the 's3' backend effectively the same thing
>> can happen because 'run_events_poll' will also process timeouts/signals
>> etc. first and may not process fds at all (until a subsequent iteration)
>> If you want flexibility around when/where you call
>> prepare_poll/check_poll the trace points imo just don't cut it.
> But it keeps the tevent loop_once function simpler. I'm not arguing
> for using the trace points no matter what, imo we should implement the
> most simple and elegant design.
sure, understood, but the penalty for this are the points we don't quite
agree on :(
>> 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 Afaics you need to introduce
some new api to tevent in order to have the data (in this case the
timeout) exchanged between the external application code (prepare_poll &
check_poll implementation) and the 'internal' xyz_poll_once function.
Perhaps discussing this aspect is muddying the waters
> <https://git.samba.org/?p=slow/samba.git;a=blobdiff;f=source3/lib/events.c;h=716f809712f00b7fea6e06497b61efbd1eda2923;hp=0bc56e454add8445782f95c1b94746218f19791c;hb=refs/heads/tevent-glib-spotlight;hpb=dffe228283ba756d4370753a1e2aed3cee3acccd;ds=inline>
>
>>>> + 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
>> Also now because you removed process_foreign_fds we no longer can
>> get FOREIGN_FD_XYZ flags set (the code only works now because
>> FOREIGN_FD_READ & FOREIGN_FD_WRITE have the same values as
>> TEVENT_FD_READ & TEVENT_FD_WRITE)
> Yes, but afair process_foreign_fds() had no effect, you set
> fde->additional_flags but the result wasn't used anywhere. But maybe I
> missed something.
nope you missed nothing but just found a bug, I meant
process_foreign_fds to use fde->flags, I was again thinking about an
eventual generic solution and was trying to avoid the tevent_internal.h
dependency (which is needed to access fde->additional_flags) and I
messed up when changing the code (and also got caught out by using the
same values of TEVENT_FD_READ|WRITE)
>  
>> 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 :-(
>> 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), at worst it starves
the glib event loop e.g. if normal tevent signals, timers etc. fire.
Also early on I found that glib seems particularly sensitive to how many
loop iterations it needs before I observed effects (e.g. the callbacks I
expected) so allowing this throttling behaviour as mentioned above could
adversely affect performance [1]. And again with the generic hat on not
making any assumptions and passing 'all' available foreign-fd results on
to the interested code to me seems sensible (and less likely to create
surprises)
I realise that this make the code a little uglier because some extra
lines (only a couple) need to be added to the '_loop_once'  function but
I still believe that is the better option (some rewrite could make it
less ugly as well)

thanks,
Noel


[1] - I did some very very cheap and nasty testing

a) mostly my posted branch e.g. no BEFORE/AFTER tracepoints used and the
glib fds are *all* processed (via process_foreign_fds)

for count in {1..10}; do cmd="./bin/default/source3/tevent_glib -t";
result=`$cmd | grep all`; echo $result; done | awk '{ total += $5;
count++ } END { print total/count }'
0.0597105
npower at ultra-trouble:/data6/WORK/samba-tevent-glib2> for count in
{1..10}; do cmd="./bin/default/source3/tevent_glib -t"; result=`$cmd |
grep all`; echo $result; done | awk '{ total += $5; count++ } END {
print total/count }'
0.0605737
npower at ultra-trouble:/data6/WORK/samba-tevent-glib2> for count in
{1..10}; do cmd="./bin/default/source3/tevent_glib -t"; result=`$cmd |
grep all`; echo $result; done | awk '{ total += $5; count++ } END {
print total/count }'
0.061678
npower at ultra-trouble:/data6/WORK/samba-tevent-glib2> for count in
{1..10}; do cmd="./bin/default/source3/tevent_glib -t"; result=`$cmd |
grep all`; echo $result; done | awk '{ total += $5; count++ } END {
print total/count }'
0.0601209

b) no BEFORE/AFTER tracepoints used and the glib fds are processed in
'run_events_poll'
for count in {1..10}; do cmd="./bin/default/source3/tevent_glib -t";
result=`$cmd | grep all`; echo $result; done | awk '{ total += $5;
count++ } END { print total/count }'
0.0686039
for count in {1..10}; do cmd="./bin/default/source3/tevent_glib -t";
result=`$cmd | grep all`; echo $result; done | awk '{ total += $5;
count++ } END { print total/count }'
0.0634941
npower at ultra-trouble:/data6/WORK/samba-tevent-temp> for count in
{1..10}; do cmd="./bin/default/source3/tevent_glib -t"; result=`$cmd |
grep all`; echo $result; done | awk '{ total += $5; count++ } END {
print total/count }'
0.0698212
npower at ultra-trouble:/data6/WORK/samba-tevent-temp> for count in
{1..10}; do cmd="./bin/default/source3/tevent_glib -t"; result=`$cmd |
grep all`; echo $result; done | awk '{ total += $5; count++ } END {
print total/count }'
0.0721467

c) your branch  e.g. BEFORE/AFTER tracepoints used and fds are processed
in 'run_events_poll'

I needed a little tweak to get it to work in your branch though

-               if (!s3_tevent_integrate_glib(mem_ctx, event_ctx, ctx)) {
+               if (!s3_tevent_integrate_glib(event_ctx, mem_ctx, ctx)) {


for count in {1..10}; do cmd="./bin/default/source3/tevent_glib -t";
result=`$cmd | grep all`; echo $result; done | awk '{ total += $5;
count++ } END { print total/count }'
0.0687106
npower at ultra-trouble:/data6/WORK/ralph-samba> for count in {1..10}; do
cmd="./bin/default/source3/tevent_glib -t"; result=`$cmd | grep all`;
echo $result; done | awk '{ total += $5; count++ } END { print
total/count }'
0.0722482
npower at ultra-trouble:/data6/WORK/ralph-samba> for count in {1..10}; do
cmd="./bin/default/source3/tevent_glib -t"; result=`$cmd | grep all`;
echo $result; done | awk '{ total += $5; count++ } END { print
total/count }'
0.0670879
npower at ultra-trouble:/data6/WORK/ralph-samba> for count in {1..10}; do
cmd="./bin/default/source3/tevent_glib -t"; result=`$cmd | grep all`;
echo $result; done | awk '{ total += $5; count++ } END { print
total/count }'
0.0779033

since tevent_glib doesn't add any 'normal' tevent fd,signal, immediate
or timer events (like a smbd process would) I would expect the results
from a samba process to be much worse. The tests above of course were
run on the same machine, there is also a good bit of variation in the
results between runs however option a) is consistently showing better
results  than b) & c) and broadly speaking b) & c) mostly have similar
results.