Messages in this thread | | | Date | Wed, 26 Nov 2008 15:00:54 +0100 | From | Andi Kleen <> | Subject | Re: [patch 05/24] perfmon: X86 generic code (x86) |
| |
On Wed, Nov 26, 2008 at 02:35:18PM +0100, Thomas Gleixner wrote: > > + * does not work with other types of PMU registers.Thus, no > > + * address is ever exposed by counters > > + * > > + * - there is never a dependency between one pmd register and > > + * another > > + */ > > + for (i = 0; num; i++) { > > + if (likely(pfm_arch_bv_test_bit(i, set->used_pmds))) { > > + pfm_write_pmd(ctx, i, set->pmds[i]); > > + num--; > > + } > > + } > > This loop construct looks scary. It relies on set->nused_pmds >= > bits set in set->used_pmds. I had to look more than once to > understand that. It's used all over the code in variations.
FWIW this loop style tripped me up during review too.
> > + */ > > + pfm_arch_resend_irq(ctx); > > Do we really need this whole NMI business ?
Without it you cannot profile interrupts off regions well.
> > 9 simple wrappers around generic bitops. The only reason you need > those is because you use 64bit variables and that does not work on > 32bit BE machines. > > I do not understand in the first place why you cant use simple > unsigned longs for the bitfields, but if this is necessary for > whatever non obvious reason, then its not an excuse to make this arch > dependent code at all. You need a LE/BE64 and a BE32 version. So you > need a generic and a special be32 version. That's not arch specific.
Or a unsigned long x[VALUE_DEPENDS_ON_WORD_SIZE]
-Andi
| |