Re: [PATCH 2/2] OO tracepoint_action

[Yao Qi <yao at codesourcery dot com>]

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 (éå)