Date: Mon, 22 Feb 2010 22:25:00 -0500 From: George Neville-Neil <gnn@neville-neil.com> To: Joseph Koshy <jkoshy@freebsd.org> Cc: embedded@freebsd.org, fabient@freebsd.org Subject: Re: First cut at hwpmc support on MIPS Message-ID: <3BF42672-9790-4D7F-9723-3D80601930B7@neville-neil.com> In-Reply-To: <867hqa9d0h.wl%koshy@unixconsulting.co.in> References: <42B59FCC-7A59-4383-BE4E-366B80B504BF@neville-neil.com> <867hqa9d0h.wl%koshy@unixconsulting.co.in>
index | next in thread | previous in thread | raw e-mail
On Feb 18, 2010, at 21:30 , Joseph Koshy wrote: > >> 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! > Thanks. > 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<E2><80><99>s Manual" > ... > +to a D-cache miss. The LSU can signal a <E2><80><9C>long stall<E2><80><9D> on a D-cache > > etc. > > You could use .Bq, .Sq. Plain ASCII quote marks would also work here. > Fixed. > 2) The manual page is missing a short description of the capabilities > of these PMCs. > Fixed. > 3) The code doesn't appear to support sampling; the manual page should > mention this. > Fixed. > 4) The debug printf() in mips_intr should be removed: > > +static int > +mips_intr(int cpu, struct trapframe *tf) > +{ > + printf("intr\n"); > + return 0; > +} > Fixed. > 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. > This is partly fixed, in that I moved to using symbols with 24K but I have not yet split the files. Probably easier now than later, but I'm not a huge fan of having the files proliferate for minor changes. I'm about to look at octeon and if I find that sufficiently different I'll do the split. > 6) These definitions will cause trouble on 64 bit MIPS systems: > > +#define PMCLOG_READADDR PMCLOG_READ32 > +#define PMCLOG_EMITADDR PMCLOG_EMIT32 > Good point. > > 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); > +} > > Is there any other processor that does this? Right now I make the chip sample in all modes by fiat. > 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; > Again, for both of these, is there an example I should work from? > > 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 */ > Fixed. Thanks for the feedback. Get back to me on these last couple of issues and I'll finish the clean up, send out a new patch and then eventually commit. Best, Georgehelp
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3BF42672-9790-4D7F-9723-3D80601930B7>