From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9844 invoked by alias); 2 Mar 2012 13:36:15 -0000 Received: (qmail 9714 invoked by uid 22791); 2 Mar 2012 13:36:13 -0000 X-SWARE-Spam-Status: No, hits=-1.4 required=5.0 tests=AWL,BAYES_00,TW_PX X-Spam-Check-By: sourceware.org Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 02 Mar 2012 13:35:59 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1S3SeL-00020x-Or from Yao_Qi@mentor.com ; Fri, 02 Mar 2012 05:35:57 -0800 Received: from SVR-ORW-FEM-02.mgc.mentorg.com ([147.34.96.206]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Fri, 2 Mar 2012 05:35:57 -0800 Received: from [127.0.0.1] (147.34.91.1) by svr-orw-fem-02.mgc.mentorg.com (147.34.96.168) with Microsoft SMTP Server id 14.1.289.1; Fri, 2 Mar 2012 05:35:56 -0800 Message-ID: <4F50CCBA.70704@codesourcery.com> Date: Fri, 02 Mar 2012 13:36:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:10.0.2) Gecko/20120216 Thunderbird/10.0.2 MIME-Version: 1.0 To: Pedro Alves CC: Subject: Re: [PATCH 2/2] OO tracepoint_action References: <1330650011-31899-1-git-send-email-yao@codesourcery.com> <1330650011-31899-3-git-send-email-yao@codesourcery.com> <4F50B3D2.4060504@redhat.com> In-Reply-To: <4F50B3D2.4060504@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-03/txt/msg00083.txt.bz2 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 (齐尧)