From owner-freebsd-embedded@FreeBSD.ORG Fri Feb 19 03:01:38 2010 Return-Path: Delivered-To: embedded@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 68896106568B; Fri, 19 Feb 2010 03:01:38 +0000 (UTC) (envelope-from jkoshy.freebsd@gmail.com) Received: from mail-yx0-f172.google.com (mail-yx0-f172.google.com [209.85.210.172]) by mx1.freebsd.org (Postfix) with ESMTP id E968A8FC12; Fri, 19 Feb 2010 03:01:37 +0000 (UTC) Received: by yxe2 with SMTP id 2so9185547yxe.7 for ; Thu, 18 Feb 2010 19:01:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:sender:message-id:to:cc :subject:in-reply-to:references:user-agent:mime-version:content-type :from:date; bh=PpZBDsgvFjftXxLrtEqbhHdqrMsnvoE19aqbAOMIsoM=; b=HAfudWfTsPBxYhSO1Wlf539lCUZgaacoqlER0k/6qrYZRXaREbEcRbnQQFEUYNGqow BedMsWICjNlmy7Mg5HBOJTkuBR9H8xbgO2gM1/32PvPf1ETegxJDdqHw8o+MT+L6xR0N YM6eNtbJhyBq0SnaI2mRQq9Gi1DbcepkPlPV0= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=sender:message-id:to:cc:subject:in-reply-to:references:user-agent :mime-version:content-type:from:date; b=XJHwTmvG1NpG1+E9z3mszjVkGmZudYJ2IFNHizWT5dbBvJUAuaHpyJZ+ed6yeFHIsF UyKxhkxyLLzGZAgw7dvFUZGdkyABIDxkUZCUT13qyTbYgXJ8JKMdgxmDawhpAtJ3EyvQ 4KYBj89uq83CwfvnnwjAqV52XrKHrZ+yTuMLw= Received: by 10.151.59.13 with SMTP id m13mr716153ybk.291.1266547006050; Thu, 18 Feb 2010 18:36:46 -0800 (PST) Received: from moria.unixconsulting.co.in ([122.172.23.214]) by mx.google.com with ESMTPS id 7sm3959016yxd.44.2010.02.18.18.36.42 (version=TLSv1/SSLv3 cipher=RC4-MD5); Thu, 18 Feb 2010 18:36:44 -0800 (PST) Sender: Joseph Koshy Message-ID: <867hqa9d0h.wl%koshy@unixconsulting.co.in> To: George Neville-Neil In-Reply-To: <42B59FCC-7A59-4383-BE4E-366B80B504BF@neville-neil.com> References: <42B59FCC-7A59-4383-BE4E-366B80B504BF@neville-neil.com> User-Agent: Wanderlust/2.14.0 (Africa) SEMI/1.14.6 (Maruoka) FLIM/1.14.8 (=?ISO-8859-4?Q?Shij=F2?=) APEL/10.7 Emacs/22.3 (amd64-portbld-freebsd6.3) MULE/5.0 (SAKAKI) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII From: Joseph Koshy Date: Fri, 19 Feb 2010 02:30:55 -0000 Cc: embedded@freebsd.org, fabient@freebsd.org Subject: Re: First cut at hwpmc support on MIPS X-BeenThere: freebsd-embedded@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Dedicated and Embedded Systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Feb 2010 03:01:38 -0000 > Please review and let me know if you have comments. I'd like to commit this > in a week or so. > http://people.freebsd.org/~gnn/mipshwpmc_1.diff Nice work! Review comments, adding to those already commented on: 1) There appears to be unnecessary gunk (UTF8?) in the manual page. +.%B "MIPS32 24K Processor Core Family Software User<80><99>s Manual" ... +to a D-cache miss. The LSU can signal a <80><9C>long stall<80><9D> on a D-cache etc. You could use .Bq, .Sq. Plain ASCII quote marks would also work here. 2) The manual page is missing a short description of the capabilities of these PMCs. 3) The code doesn't appear to support sampling; the manual page should mention this. 4) The debug printf() in mips_intr should be removed: +static int +mips_intr(int cpu, struct trapframe *tf) +{ + printf("intr\n"); + return 0; +} 5) It would be help to split "hwpmc_mips.c" into two files: - One for code specific to this PMC family. Say "hwpmc_mips24k.c". - One for "generic" MIPS related code, e.g., code to walk stacks. You should expect that every manufacturer will have their own kind of PMCs, with differing capabilities and programming models. Catering for the variability upfront would be prudent. As commented on by earlier reviewers, you may want to consider changing symbol naming to suit. 56 These definitions will cause trouble on 64 bit MIPS systems: +#define PMCLOG_READADDR PMCLOG_READ32 +#define PMCLOG_EMITADDR PMCLOG_EMIT32 7) From the definitions in the header file, these PMCs appear to support the concept of sampling based on processor mode: +#define MIPS_PMC_USER_ENABLE 0x08 /* Count in USER mode */ +#define MIPS_PMC_SUPER_ENABLE 0x04 /* Count in SUPERVISOR mode */ +#define MIPS_PMC_KERNEL_ENABLE 0x02 /* Count in KERNEL mode */ If that is the case, then you should support those modifiers in libpmc's event parsing. The libpmc code in the patch appears to be a stub: +static int +mips_allocate_pmc(enum pmc_event pe, char *ctrspec __unused, + struct pmc_op_pmcallocate *pmc_config __unused) +{ + switch (pe) { + default: + break; + } + + return (0); +} 8) You can reduce the size of the following table in "hwpmc_mips.c", by treating the pe_counter field as a set of flags. +struct mips_event_code_map { + enum pmc_event pe_ev; /* enum value */ + uint8_t pe_counter; /* Which counter this can be counted in. */ + uint8_t pe_code; /* numeric code */ +}; +const struct mips_event_code_map mips_event_codes[] = { + { PMC_EV_MIPS_CYCLE, 0, 0}, + { PMC_EV_MIPS_CYCLE, 1, 0}, <<<--- repeated information 9) You'd want to support flags that control counting based on processor modes. For this, you would want to pass down flags from userland and change the `pm_mips_evsel' field to suit: +static int +mips_allocate_pmc(int cpu, int ri, struct pmc *pm, + const struct pmc_op_pmcallocate *a) +{ ... + pm->pm_md.pm_mips.pm_mips_evsel = config; 10) If the number and width of these PMCs are fixed, you should document that in the manual page: pmc_md_initialize() { + mips_npmcs = 2; ... + pcd->pcd_width = 32; /* XXX: Fix for 64 bit MIPS */ Regards, Koshy