On Wed, 23 Nov 2011 10:56:24 -0700, Alex Rousskov wrote:
> On 11/23/2011 05:11 AM, Amos Jeffries wrote:
>> The problem:
>> CommCalls functionality is conflated with calls created to do
>> general
>> FD handling (FD as pipe handle, FD as disk handle, FD as pointer
>> into
>> the fd_table structure). Sometimes because they do operations
>> mirroring
>> comm handlers and also use FD. None of this actually requires the
>> comm
>> layer to be involved though.
>
> I do not understand the above. Comm deals with pipe, disk, and socket
> communications. Are you assigning some more restricted meaning to
> "comm
> handler" (e.g., comm handler can only deal with communication using
> connected sockets)?
comm as a whole (include fde functionality) does deal with all three.
The comm.cc code though makes a lot of network-centric assumptions about
things that can be done with the FD.
CommCalls.cc functionality in particular is optimized for use by the
comm.cc TCP code, and has almost no relevance to the pipe or disk code.
Yet the pipe and disk callbacks are expected to receive CommCalls
parameters.
>
>> The Comm::Connection objects which comm
>> handlers pass around is also very inappropriate for these FD types.
>
> I agree that using Comm::Connection for manipulating non-connections
> is
> wrong. We need a lower level "descriptor" type(s) that non-connection
> users and Comm::Connection would use.
>
From the look I've had so far we don't actually need that.
The pipes are using "void *data" as ahandle to some state object
controllign teh pipe. Disk OPs are still using "int fd" as a handle to
an fde object when they are forced to go through CommCalls. Both the
objects referenced by those alternative handle types maintain their own
state members and flags mostly independent from the comm.cc systems.
>
>> This patch adds functionality for two types of AsyncCall to replace
>> the
>> Comm parameters being abused.
>
> I see the "adds" part but I do not see the "replace" part. Which Comm
> parameters we will no longer abuse after this patch?
Three calls in trunk are "replaced" / altered. They are now using
CommCloseCbParams to pass FD (becomes FdeCbParams) or void*data (becomes
an instance of the CBDATA template Params field)
>
> s/two types of AsyncCall/two types of AsyncCall Dialers/?
>
One is a new params type, one a new dialer type *and* params type.
Description is a bit fuzzy. I was intending to convey "AsyncCalls" as
feature name more than specific alteration to AsyncCall class.
>
>
>> * FdeCbParams is added to CommCalls infrastructure, for use
>> internally
>> and "lower" than comm API to pass around raw FD values. This should
>> be
>> avoided on TCP socket FD, but may be used by callers needing FD
>> where
>> Comm::Connection is inappropriate.
>>
>> * UnaryCbdataDialer<T> template is added to dial a call on any
>> wrapper
>> function taking one cbdata state object as a parameter pointer.
>> This dialer checks the cbdata base object validity prior to
>> calling. As
>> such it acts like the cbdata callbacks, but adding async scheduling
>> ability.
>> It also adds the AsyncCalls ability for the wrapper functions to
>> have
>> typed parameters instead of "void *data" and casting.
>>
>>
>> Alex: I've been searching the Async code for a long while trying to
>> figure out how to do this with the existing API. If you can point me
>> at
>> how to do it without adding a new template I'd be grateful. Or
>> alternatively for any reasons why this does not already exist.
>
> I do not think there is anything wrong with your UnaryCbdataDialer
> approach as such. I probably thought that the same energy was better
> spent on converting remaining call recipients to Jobs instead. If we
> go
> your route, we will have to change the handlers once again when their
> code becomes an AsyncJob, and I do not like changing the same code
> twice.
>
AI agree on the double-change things. I've just been bitten several
times this year needing to add new functionality and not having time to
re-code a whole AsyncJob.
This helper callback is one such case in point. Re-writing the entire
helper IPC API as a Job just to detatch the state destructor from
CommCalls API dependency would easily blow out into a massive effort and
code change.
>
>> For now this dialer template is isolated in helper.cc and used only
>> for
>> the helper pipe() FD close handlers. If others agree I'd like to
>> make it
>> globally available in new src/base/AsyncCbdataCalls.h
>
> Yes, please.
>
>
>> and use it to ease
>> the transition from cbdata objects to Jobs. It seems the big hitch
>> currently to transitioning is that to use generic Unary/Nullary
>> templates the object must be a fully transitioned AsyncJob so we end
>> up
>> with custom-coded dialers for each cbdata class type.
>
> I do not see how UnaryCbdataDialer "eases the transition", but it
> will
> improve old-style code so I welcome its addition.
>
> IMO, the "big hitch" is in satisfying AsyncJob requirements. Once
> that
> is done, adjusting callbacks should be very straightforward.
>
I was assuming that since AsyncJob is cbdata child and does cbdata
validation prior to dialing the job members, that mirroring that check
would be sufficient for other cbdata.
- validate is performed before dialing the call and cbdata done() is
used on destructing the call (dialed or not).
The other core requirement that the async call handler has no
expectation of timing, only of sequence. Seems to be met by the cbdata
handlers assumption that the calling code must exit immediately, or move
on to independent code operations not trusting the passed cbdata object
or parameters.
Have I missed anything?
>
>> +/// Special Calls, for direct use of an FD without a controlling
>> Comm::Connection
>> +/// This is used for pipe() FD with helpers, and internally by Comm
>> when handling some special FD actions.
>> +class FdeCbParams;
>
> Please move class description to the class declaration.
Done.
>
> Do we need to predeclare at all? In the patch, I do not see
> FdeCbParams
> (and FDECB) used before they are fully declared.
I put the typedef there to match the other typedefs (I assume you did
that to make the API handler types easily found). At this stage I'm
unsure how many of the pipe() and disk I/O handlers will need to use
this new CommCall parameter object. Only the accept() and close()
handlers have been re-checked so far. I hope to remove it from the comm
API into an internal type, but won't know for sure until the handler
audit is finished.
Some compilers require declaration before typedef to avoid incomplete
or undefined type warnings. This pre-declare seems to silence them fine.
>
>
>> +class UnaryCbdataDialer : public CallDialer
>> +{
>> +public:
>> + typedef T Params;
>> + typedef void Handler(T *);
>> +
>> + UnaryCbdataDialer(Handler *aHandler, Params *aParam) :
>> + data(cbdataReference(aParam)),
>> + handler(aHandler)
>> + {}
>> +
>> + ~UnaryCbdataDialer() { cbdataReferenceDone(data); }
>> +
>> + bool canDial(AsyncCall &call) { return true; }
>> + void dial(AsyncCall &call) {
>> + if (cbdataReferenceValid(data))
>> + handler(data);
>> + }
>> +
>> + void print(std::ostream &os) const {} // dummy for now.
>> +
>
> Please add explicit "virtual" prefix to virtual methods and a
> /* CallDialer API */
> comment to explain where they came from.
>
> canDial() should probably return cbdataReferenceValid(data) for call
> debugging to work correctly and possibly to reduce call setup
> overheads.
> Similarly, dial() should probably not check
> cbdataReferenceValid(data)
> again because canDial() does.
>
Hm. Okay. Was trying to avoid complex/slow canDial(), but agreed. done.
I was not entirely clear what happens to the scheduled call if
!canDial() code path. Drop the queue refcount and destruct?
> Please implement the print() method by printing the argument. See
> UnaryMemFunT::print(). This is easy (especially after you fix data's
> type, see below) and will help with finding the right calls in an
> ALL,9
> trace.
Done.
>
>> +template<class T>
>> +class UnaryCbdataDialer : public CallDialer
>> +{
> ...
>> + Params *data;
>> + Handler *handler;
>
> The "data" member should be of type CbcPointer<T>. Besides reusing
> existing code, this will simplify your constructor, remove explicit
> destructor, and make copying of UnaryCbdataDialer objects work. If
> you
> do not do that, you probably should add private copy constructor and
> assignment operator declarations (without implementation).
>
Done.
>
> To preserve symmetry with UnaryMemFunT, please rename T to Data OR
> Argument1, removing Params, especially since we only have one
> parameter
> here and not many parameterS.
Done.
>
> To avoid the need to typedef dialers manually, consider adding a
> helper
> function. Something along these lines:
>
> template <class Argument1>
> UnaryCbdataDialer<Argument1>
> cbdataDialer(typename UnaryCbdataDialer<Argument1>::Handler *handler,
> Argument1 arg1)
> {
> return UnaryCbdataDialer<Argument1>(handler, arg1);
> }
>
Done.
> See existing JobMemFun() for similar code and adjust as needed. When
> you
> are done, you should be able to replace:
>
>> + typedef UnaryCbdataDialer<helper_server> SrvDialer;
>> + srv->asyncDestructor = asyncCall(5,4, "helperServerFree",
>> SrvDialer(helperServerFree, srv));
>
> with
>
>> + srv->asyncDestructor = asyncCall(5,4, "helperServerFree",
>> cbdataDialer(helperServerFree, srv));
>
>
Done.
>
>> + /// the AsyncCall which has been created to destruct this
>> object on FD close
>> + AsyncCall::Pointer asyncDestructor;
>
> Why do we need to remember this callback? The data member seems to be
> unused.
>
I was thinking explicit cancel. But it seems not necessary in the end.
Dropped.
>
>> - int fd; // raw FD which the call was about. use conn instead
>> for new code.
>> + int fd; /// FD which the call was about. Set by the caller
>> creator. Use conn instead for new code.
>
> s/caller creator/caller/? The creator of the callback does not set
> the
> fd value.
The current ones using this do. See _comm_close() in comm.cc.
I would like the commCallCloseHandlers() not care about duplicating
that effort. Just to schedule them all and finish.
>
> Also, since not all new code should use Connection after your
> changes, I
> would replace "Use conn instead for new code" with "Use conn if
> available" or something like that. I have to say that your changes
?? missing text?
>
>
>> /* CommCalls implement AsyncCall interface for comm_* callbacks.
>> * The classes cover two call dialer kinds:
>> * - A C-style call using a function pointer (depricated);
>> * - A C++-style call to an AsyncJob child.
>> * and several comm_* callback kinds:
>> * - accept (IOACB)
>> * - connect (CNCB)
>> * - I/O (IOCB)
>> * - timeout (CTCB)
>> * - close (CLCB)
>> */
>
> This comment in src/CommCalls.h needs to be updated to reflect the
> addition of "FD event" callback.
>
Done.
Thank you. Build testing that all now. Updated patch to follow.
Amos
Received on Thu Nov 24 2011 - 00:37:39 MST
This archive was generated by hypermail 2.2.0 : Thu Nov 24 2011 - 12:00:06 MST