[PATCH] tevent patches

Stefan (metze) Metzmacher metze at samba.org
Fri Feb 28 05:32:58 MST 2014


Am 28.02.2014 12:39, schrieb Amitay Isaacs:
> On Fri, Feb 28, 2014 at 8:26 PM, Stefan (metze) Metzmacher
> <metze at samba.org>wrote:
> 
>> Hi Amitay,
>>
>>>> On Thu, Feb 27, 2014 at 09:03:30AM +0100, Stefan (metze) Metzmacher
>> wrote:
>>>>> Hi Amitay,
>>>>>
>>>>>> Here are some improvements to tevent tracing functionality.  This was
>>>>>> especially useful for debugging CTDB and finding out what events are
>>>> being
>>>>>> triggered often.
>>>>>>
>>>>>> The patches are also available in my tree:
>>>>>
>>>>> We can't make trace point an compile time optional feature,
>>>>> smbd relies on this feature.
>>>>>
>>>>> Can you add a TEVENT_HAS_HANDLER_TRACE_POINTS define similar to
>>>>> TEVENT_HAS_LOOP_ONCE_TRACE_POINTS?
>>>>
>>>> Just brainstorming....
>>>>
>>>> Would it be possible to make these tracepoints if not used
>>>> 0-cost when a handler is executed? If a caller wants to have
>>>> trace points, can we wrap the handlers in another function
>>>> at tevent_add_fd time & friends?
>>>>
>>>
>>> Here's second attempt at tracing patches. This one implements 0-cost
>>> handler when trace points are not used.
>>> Also added a define that can be used for configuration checks.
>>
>> The problem I have with this is that this costs much more than
>> the first patchset for the most performance critical users
>> smbd and ctdbd. A additional talloc() call per event registration
>> costs a lot more than a function call to a noop function.
>>
>> Also this add a memory leak in tevent_schedule_immediate().
>>
>> So I'd prefer to just have my issues on the first attempt fixed.
>> Additionally we could think about something like
>> tevent_active_trace_point() to activate just the tracepoints
>> the implementation is interested in, while all of them are
>> activated by default.
>>
>> However the struct tevent_trace_handler_data inspired me how to
>> possibly solve some problems in my attempt to implement
>> in my master3-impersonate branch.
>>
>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=625286f6cf915f8f10a472ae9c066aee648d2086
>> This is also needed for our new dcerpc infrastructure, I'll work on that
>> in the next weeks.
>>
>> Once we have a generic infrastructure to register wrapper handlers,
>> which doesn't require talloc and which are to some extent stackable,
>> we can think about changing the internal implementation.
>> But I fear this will require incompatible ABI changes.
>>
> 
> Hi Metze,
> 
> Yes, I was also wondering about the extra talloc cost when tracing is
> enabled.
> 
> Here's another attempt after removing the extra talloc.  This one just adds
> extra pointer to the event structures.
> 
> This is now a smaller change, but does not provide any generic mechanism
> for stackable handlers.

For now I'd avoid changing the structures and dereferencing them
in functions like _tevent_add_fd(), the reason is that
struct tevent_ops is public and the backend is able to use its own
version of 'struct tevent_fd', while the generic code should not
dereference it.

My plan for the future is that introduce thinks like
tevent_fd_ops/tevent_timer_ops...
in order to hold the [g|s]et_fd_*() function pointers.

There we can add a add_wrapper_handler() function pointer which can be
implemented by the backend.
For the builtin backends we can just add some new members to struct
tevent_fd.

The tricky thing is tevent_immediate, as this has a fixed size in order
to be preallocated.
We may need ways to have tevent_immediate and maybe tevent_timer objects
preallocated,
but based for a specific backend, it would rememver the tevent_ops
pointer and
check it in the _schedule() function and abort if the tevent_ops pointer
doesn't match.

If we're already changing the backend ABI, we can also think about
somehow adding support
for CLOCK_MONOTONIC timer events.

metze


More information about the samba-technical mailing list