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>
next in thread | previous in thread | raw e-mail | index | archive | help
On Feb 18, 2010, at 21:30 , Joseph Koshy wrote: >=20 >> Please review and let me know if you have comments. I'd like to = commit this >> in a week or so. >=20 >> http://people.freebsd.org/~gnn/mipshwpmc_1.diff >=20 > Nice work! >=20 Thanks. > Review comments, adding to those already commented on: >=20 > 1) There appears to be unnecessary gunk (UTF8?) in the manual page. >=20 > +.%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 >=20 > etc. >=20 > You could use .Bq, .Sq. Plain ASCII quote marks would also work here. >=20 Fixed. > 2) The manual page is missing a short description of the capabilities > of these PMCs. >=20 Fixed. > 3) The code doesn't appear to support sampling; the manual page should > mention this. >=20 Fixed. > 4) The debug printf() in mips_intr should be removed: >=20 > +static int > +mips_intr(int cpu, struct trapframe *tf) > +{ > + printf("intr\n"); > + return 0; > +} >=20 Fixed. > 5) It would be help to split "hwpmc_mips.c" into two files: >=20 > - One for code specific to this PMC family. Say "hwpmc_mips24k.c". > - One for "generic" MIPS related code, e.g., > code to walk stacks. >=20 > You should expect that every manufacturer will have their own kind > of PMCs, with differing capabilities and programming models. >=20 > Catering for the variability upfront would be prudent. As > commented on by earlier reviewers, you may want to consider > changing symbol naming to suit. >=20 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: >=20 > +#define PMCLOG_READADDR PMCLOG_READ32 > +#define PMCLOG_EMITADDR PMCLOG_EMIT32 >=20 Good point. >=20 > 7) =46rom the definitions in the header file, these PMCs appear to > support the concept of sampling based on processor mode: >=20 > +#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 */ >=20 > 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: >=20 > +static int > +mips_allocate_pmc(enum pmc_event pe, char *ctrspec __unused, > + struct pmc_op_pmcallocate *pmc_config __unused) > +{ > + switch (pe) { > + default: > + break; > + } > + =20 > + return (0); > +} >=20 >=20 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. >=20 > +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 */ > +}; >=20 > +const struct mips_event_code_map mips_event_codes[] =3D { > + { PMC_EV_MIPS_CYCLE, 0, 0}, > + { PMC_EV_MIPS_CYCLE, 1, 0}, <<<--- repeated information=20 >=20 >=20 > 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: >=20 > +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 =3D config; >=20 Again, for both of these, is there an example I should work from? >=20 > 10) If the number and width of these PMCs are fixed, you should > document that in the manual page: >=20 > pmc_md_initialize() > { > + mips_npmcs =3D 2; > ... > + pcd->pcd_width =3D 32; /* XXX: Fix for 64 bit MIPS */ >=20 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, George
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3BF42672-9790-4D7F-9723-3D80601930B7>