[PATCH] Use libpcl for portable coroutines

Fri May 15 11:52:55 GMT 2009

Hi Sam,

>>> This is a proof of concept patch, to make using libpcl easy for smb
>>> clients, so that sync functions can easily be converted to async functions.
>>>     
>> I added this based on tevent, see the top 4 patches in:
>> http://gitweb.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-tevent-coro
>>
>> I haven't posted this before, because I want to do more testing...
>>
>> Also we need to discuss if we really want them, and guide lines for
>> which code parts should use them.
>>   
> I notice that volker had added async_req stuff too.

that will be replaced by tevent_req step by step.

> Looking at your code, I have some thoughts and comments;
> 
> I see a fixed stack of 4196 - I found even 8K to be too small for what I
> was doing. I think we ought to allow a size, and ought to (as you are)
> allocate our own stack - but not with talloc or malloc, but using mmap,
> so that RAM is only committed as required. This should help keep
> co-routines cheap in such platforms that support this.
> 

We may need to experiment with this.

But with the tevent_coroutine design (where only the main coroutine body
can call yield) I can't see how we can end up with a large stack trace.
As long as don't call recursive function and don't use large buffer
variables on the stack.

A self growing stack would be perfect, but maybe not really needed.
(Maybe we can add that to libpcl)

> It took me a while to grasp the example at:
> http://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=1efdd2469b7ae2b4c0dd4e78ec7c864b3cf47faa;hp=ced03d86f39a5f87af682f48defef5fc73d61388
> but it seems that the main principle is that you can nest co-routines
> providing each inner one is given a completion callback by it's creator
> that switches back to it's creators stack.
> 
> The risk seems to be in ensuring that the creator stack isn't activated
> before the thing it waits on is complete - or that it can tell if this
> occurs and switches to co_main to await the official callback.
> 
> I think co_resume() isn't generally safe to call - co_resume() doesn't
> neccessarily  switch to the creator coroutine which might be useful. If
> a co-routine was launched from another coroutine then the two coroutines
> may keep flipping between each-other, which is why I switched to
> co_call(co_main) which in our case is the stack running the event loop in.
> I gnash my teeth that the real co_main is static - so I had to
> "discover" with co_current() when the first co_create() is called by
> wrapping co_create().

I changed my code a little bit to make sure we never nest coroutines.
http://gitweb.samba.org/?p=metze/samba/wip.git;a=shortlog;h=refs/heads/master4-tevent-coro-2
http://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=f6c3ea8784156e3697c3930fe46cb2c49aba37d2
http://gitweb.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=dba77a388d644828c17f816f88645e2bfd90230d

tevent_coroutine_run() is now static and only every triggered from
the main event loop. (See the immediate trigger event).

The example_coro_send() only schedules the immediate trigger event.

> 
> 
> I think it's a shame that the active tevent_coroutine needs to be passed
> to the yield function,

It's not a shame, it's really good, because it only allows the main
coroutine body to call tevent_coroutine_yield(). It makes
the yield explicit and avoids implicit side effects.

> but I see we must unless we extend libpcl to
> allow user data to be attached to coroutine_t. Maybe we could nick the
> first few bytes from the stack to point to the tevent_cououtine? I think
> if we may be linking to other libraries that also use libpcl that
> perhaps we must do as you have done...
> 
> I also prefer (as you saw in my patch) that tevent_coroutine_yield
> should be called implicitly by example_ocor_recv.

No! This is exactly what leads to nested event loops.

We should have this explicit for a "sync" call driven
by a coroutine:

subreq = foo_send(mem_ctx, event_ctx);
if (!tevent_coroutine_yield(subreq, tco)) {
	return tevent_coroutine_return(tco);
}
result = foo_recv(subreq);

> One consequence is
> that code can be written synchronously without much thought for
> co-routines, but work asynchronously as co-routines if support is
> available. Also sometimes the _send and _recv functions may be hidden
> behind an api and the caller might not have anything on which to yield.

Then it's a real sync function that doesn't need an event context
or it's a bug that needs to be fixed.

> I also note the continuation pattern that runs through
> ntvfs_request->async_states->send_fn and smbcli_request->async.fn and
> composite_context->async.fn and now tevent_req->async.fn. I've heard it
> regretted that composites and smbcli_request structs aren't more similar.

> If these are all reworked with the async struct at the top, perhaps
> being a union with it - then co-routines can be adopted with respect to
> all these types of continuations without having to be tied to tevent or
> any one of them.

See above they should all use tevent_req in future.

we could possibly add samba4 specific tevent_coroutine_yield_composite(),
tevent_coroutine_yield_smbcli() function until everything is converted.

> In the example I think the *_send() are misnamed because unlike the old
> async pattern, they don't send, they are just a trampoline launch the
> co-routine.

Now the _send function schedules the execution of the body function.

> I dearly want to get rid of the *_send() functions in your example - the
> fact that they exist at all is because there isn't a portable way to
> replicate the current stackframe onto the new stack, otherwise co_create
> could work like fork(). As this isn't possible, I think that this
> trampoline should be minimized as much as possible as it is a
> distraction from the intent of the code.
> 
> I tried to keep the trampoline to be nothing beyond the necessary
> declarations:
> 
> void proxy_connect_trampoline(va_list args)
> {
>     co_get_va(struct ntvfs_module_context *, ntvfs);
>     co_get_va(struct ntvfs_request *, req);
>     co_get_va(union smb_tcon *, tcon);
> 
>     ntvfs_co_passthrough_exit(proxy_connect(ntvfs, req, tcon));
> }
> 
> but yet the trampoline here (by means of the passthrough macro) also
> deals with co_exit and other cleaning up. The body function can then be
> just a regular synchronous function.
> 

That's too much magic for my taste.

We should have only one way to call async function and that's
with tevent_req based _send/_recv pair. And the caller really
should not need to know how the internals look like.

> Perhaps we have different needs in this work.
> 
> It's good that tevent integrates nicely with co-routines, but I think
> that co-routines here are integrating too tightly with tevent, and
> becoming too dependant on it. I think the private_data and co_call
> routine should be the key interface to activating a co-routine, and not
> tevent; partly because this IS the interface that already exists with
> composites, ntvfs_requests and smbcli_requests.

I don't see the problem here, see my example how ntvfs_requests and
tevent_req based coroutines can interact fine. In the long term
composite_context, ntvfs_request and others should be converted
to tevent_req.

> I think that co_main is fundamental to co-routine use here. A function
> may call another function asynchronously by launching the co-routine
> before calling, or it may permit the other function to do this
> internally. Whichever function launches the co-routine must also set the
> handler to switch back on completion, but the special case arises when
> the co-routine is launched from the main stack.
> 
> If the main stack launches a co-routine, then it must return signifying
> that a response will follow later.
> If one co-routine launches another, then the launching co-routine is
> suspended until the other returns, and the invoking caller from the main
> stack already expects an asynchronous response.
> 
> Are there cases where one co-routine will launch another, and expect to
> be returned to before the other has completed? There may if it wants to
> parallelize requests. It will have to launch all co-routines and then
> enter a loop that keeps co_call(co_main) until all requests are
> complete. It does not need to use tevent directly, as long as the
> requests it is waiting for use tevent.

As every async function needs tevent, I think it makes no sense to
decouple both.

As said above() with my code we only every switch from the
main stack to a coroutine and back. We never switch between
coroutines directly.

> So these are my thoughts;
> * that it should be simple to launch a new coroutine without using tevent

That doesn't make sense to me.

> * that one day composites, smbcli_requests, ntvfs_requests and
> tevent_req will share a common async union

tevent_req is that union, the other's should went away in future.

> * that trampolines be kept as tiny as C allows, and allow the body to be
> written as a regular sync function
> * that _recv functions do the yielding so the caller doesn't have to
> (saves re-writing lots of code)

For long term, we have to rewrite the code.
Which we have to do any way, because the really bad bugs with broken
connections.

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 252 bytes
Desc: OpenPGP digital signature
Url : http://lists.samba.org/archive/samba-technical/attachments/20090515/9f436d74/signature.bin