From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9791 invoked by alias); 7 Jan 2010 18:01:05 -0000 Received: (qmail 9676 invoked by uid 22791); 7 Jan 2010 18:01:03 -0000 X-SWARE-Spam-Status: No, hits=-0.4 required=5.0 tests=AWL,BAYES_50,KAM_STOCKGEN,SPF_HELO_PASS,SPF_PASS X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 07 Jan 2010 18:00:58 +0000 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o07I0C4E005161 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 7 Jan 2010 13:00:37 -0500 Received: from fche.csb (vpn-231-10.phx2.redhat.com [10.3.231.10]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o07I0C2Y006465; Thu, 7 Jan 2010 13:00:12 -0500 Received: by fche.csb (Postfix, from userid 2569) id 54E7A5813A; Thu, 7 Jan 2010 13:00:11 -0500 (EST) To: Prerna Saxena Cc: systemtap@sourceware.org Subject: Re: [RFC] Systemtap translator support for hardware breakpoints on References: <4B459CC8.2030402@linux.vnet.ibm.com> From: fche@redhat.com (Frank Ch. Eigler) Date: Thu, 07 Jan 2010 18:01:00 -0000 In-Reply-To: <4B459CC8.2030402@linux.vnet.ibm.com> (Prerna Saxena's message of "Thu, 07 Jan 2010 14:05:20 +0530") Message-ID: User-Agent: Gnus/5.1008 (Gnus v5.10.8) Emacs/21.4 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sourceware.org X-SW-Source: 2010-q1/txt/msg00040.txt.bz2 Prerna Saxena writes: > [...] > This patch enables systemtap translator to probe hardware breakpoints > on x86 and x86_64 (based on mainline kernel). Looks like a good start. In order to merge this, it is essential to have a testsuite, posted results on a variety of architectures and kernel versions (kfail for older kernels), and documentation. > Syntax : > probe kernel.data(ADDRESS).write > probe kernel.data(ADDRESS).rw > probe kernel.data(ADDRESS).length(LEN).write > probe kernel.data(ADDRESS).length(LEN).rw > probe kernel.data("SYMBOL_NAME").write > probe kernel.data("SYMBOL_NAME").rw (I wonder if simple .read or .write would be sufficient as suffixes, and let a script render both as probe kernel.data(ADDRESS).write, probe kernel.data(ADDRESS).read and let the translator collapse such duplication into the internal RW case.) > + // Warn of misconfigured kernels > + s.op->newline() << "#ifndef CONFIG_HAVE_HW_BREAKPOINT"; > + s.op->newline() << "#error \"Need CONFIG_HAVE_HW_BREAKPOINT!\""; > + s.op->newline() << "#endif"; Instead or in addition, the translator could make the whole probe point type conditional on session.kernel_config("CONFIG_HAVE_HW_BREAKPOINT"). > +// Define macros for access type > + s.op->newline() << "#define HWBKPT_READ 0"; > + s.op->newline() << "#define HWBKPT_WRITE 1"; > + s.op->newline() << "#define HWBKPT_RW 2"; (Are these necessary? Could the translator not emit symbols like HW_BREAKPOINT_W directly?) > + s.op->newline() << "static struct stap_hwbkpt_probe {"; > + s.op->newline() << "int registered_p:1;"; > +// registered_p = -1 signifies an invalid probe that must not be registered > +// registered_p = 0 signifies a probe that failed registration > +// registered_p = 1 signifies a probe that got registered successfully A one-bit value can't represent three different values. > + if (p->symbol_name.size()) > + s.op->line() << " .address=(unsigned long)0x0" << "ULL,"; (You probably don't need explicit initialization to zero.) > + s.op->newline() << "static int enter_hwbkpt_probe (struct perf_event *bp,"; > [...] > + s.op->newline() << "for ( i=0; i<" << hwbkpt_probes_vector.size() << "; i++) {"; > + s.op->newline() << "struct perf_event_attr *hp = & stap_hwbkpt_probe_array[i];"; > + s.op->newline() << "if (bp->attr.bp_addr==hp->bp_addr && bp->attr.bp_type==hp->bp_type && bp->attr.bp_len==hp->bp_len ) {"; > [...] To what extent does this handle multiple hw breakpoints on the same address? > [...] > +void > +hwbkpt_derived_probe_group::emit_module_init (systemtap_session& s) > +{ [...] > + s.op->newline() << "const char *hwbkpt_symbol_name = addr ? NULL : sdp->symbol;"; > + s.op->newline() << " hw_breakpoint_init(hp);"; Normally we use newline(1) or (-1) to indent instead of explicit spaces like this. > + s.op->newline() << " if (addr) "; > + s.op->newline() << " hp->bp_addr = (unsigned long) addr;"; > + s.op->newline() << " else "; > + s.op->newline() << " hp->bp_addr = kallsyms_lookup_name(hwbkpt_symbol_name);"; Is kallsyms_lookup_name actually module-exported these days? Should the translator try to resolve symbols at translate time? Wildcards? > + s.op->newline() << "#ifdef CONFIG_X86"; > + s.op->newline() << " switch(sdp->len) {"; > + s.op->newline() << " case HW_BREAKPOINT_LEN_1:"; > + s.op->newline() << " hp->bp_len = HW_BREAKPOINT_LEN_1;"; > [...] Is this sort of switch really necessary? Why not just set hp->bp_len = sdp->len; > + s.op->newline() << " default:"; > + s.op->newline() << " _stp_warn(\"Probe %s registration skipped : unsupported length. Supported lengths for x86 are 1,2,4 {8 for x86_84 only} \",sdp->pp);"; > + s.op->newline() << " sdp->registered_p = -1;"; > + s.op->newline() << " stap_hwbkpt_ret_array[i] = ERR_PTR(-EINVAL);"; Why is this necessary ... > + s.op->newline() << " case HWBKPT_READ:"; > + s.op->newline() << " _stp_warn(\"Probe %s registration skipped : READ-ONLY hardware breakpoints not supported on x86\",sdp->pp);"; > + s.op->newline() << " sdp->registered_p = -1;"; > + s.op->newline() << " break;"; ... and this too ... > + s.op->newline() << "if (sdp->registered_p >= 0) {"; > + s.op->newline() << " stap_hwbkpt_ret_array[i] = register_wide_hw_breakpoint( hp, (void *)&enter_hwbkpt_probe );"; ... considering that register_wide_hw_breakpoint() would surely check its inputs and reject requests deemed inappropriate for the current architecture? Note also that registered_p always >= 0 for your actual int:1 data type. Consider simply == 0 for unregistered, == 1 for registered. > + s.op->newline() << " if (IS_ERR(stap_hwbkpt_ret_array[i])) {"; > + s.op->newline() << " rc = PTR_ERR(stap_hwbkpt_ret_array[i]);"; > + s.op->newline() << " sdp->registered_p = 0;"; > + s.op->newline(1) << " _stp_warn(\" Hwbkpt Probe %s : Registration error rc= %d, Addr = %p, name = %s\", probe_point, rc, addr, hwbkpt_symbol_name);"; > + s.op->newline(-1) << " }"; > + s.op->newline() << " else sdp->registered_p = 1;"; > + s.op->newline() << "}"; > + s.op->newline(-1) << "}"; // for loop Not so simple: if you get a failure on the sixth registration, you have to unregister the prior ones. - FChE