Re: [PATCH 2/2] OO tracepoint_action

Yao Qi
2012-03-02T13:36:15Z

On 03/02/2012 07:49 PM, Pedro Alves wrote:
>> > #ifdef IN_PROCESS_AGENT
>> > /* Only record the first error we get. */
>> > if (cmpxchg (&expr_eval_result,
>> > @@ -1263,8 +1285,6 @@ record_tracepoint_error (struct tracepoint *tpoint, const char *which,
>> > if (expr_eval_result != expr_eval_no_error)
>> > return;
>> > #endif
>> > -
>> > - error_tracepoint = tpoint;
> By moving this error_tracepoint set elsewhere, you're bypassing the cmpxchg
> synchronization just above, introducing the race this method was supposed to
> prevent. What was the motivation for this change?
>

The motivation here is to not pass tracepoint `tpoint' to
tracepoint_action_ops->execute, because `tpoint' is not used in actions
except X action.

The update to error_tracepoint is guarded by action->ops->execute

if (taction->ops->execute (taction, ctx, tframe))
error_tracepoint = tpoint;

and ops->execute calls cmpxchg (for X action). `execute' of other types
of action always return 0. So I think this is equivalent to original.

>> > +
>> > +static CORE_ADDR
>> > +l_tracepoint_action_download (struct tracepoint_action *action)
>> > +{
>> > + CORE_ADDR ipa_action
>> > + = target_malloc (sizeof (struct collect_static_trace_data_action));
> spurious double-space.
>
>> > @@ -4412,105 +4685,31 @@ do_action_at_tracepoint (struct tracepoint_hit_ctx *ctx,
>> > struct traceframe *tframe,
>> > struct tracepoint_action *taction)
>> > {
>> > - enum eval_result_type err;
>> > -
>> > - switch (taction->type)
>> > +#ifdef IN_PROCESS_AGENT
>> > + if (taction->ops == NULL)
>> > {
> So ops is lazily initialized in the IPA? I'm a little warry of the potential
> for slowing down the collect path (adding several indirections, and
> extra calls that are hard for the compiler to optimize out work against code
> density, cache locality, etc.). We want to squeeze out performance in the
> nano-second range, though what matters the most is the case of tracepoint hit
> and then the condition evaluating false.
>

Hmm, if we concern performance here, it is fine with me. Then, I
suggest that we drop the changes in do_action_at_tracepoint, and get rid
of fields `init' and `execute' from struct tracepoint_action_ops, like this,

struct tracepoint_action_ops
{
CORE_ADDR (*download) (struct tracepoint_action *action);
};

struct tracepoint_action
{
#ifndef IN_PROCESS_AGENT
struct tracepoint_action_ops *ops;
#endif
char type;
};

What do you think? Note that I plan to add a new field `send' in
tracepoint_action_ops to send different types of actions to agent
through "command buffer".

--
Yao (é½å°§)