Re: [patch 05/24] perfmon: X86 generic code (x86)

[stephane eranian]

Thomas,

On Wed, Nov 26, 2008 at 10:18 PM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> On Wed, 26 Nov 2008, Andi Kleen wrote:
>
>> 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.
>
> It's even worse than I thought when looking at it a second time:
>
> All the loops iterate over an array which means in the worst case we
> do full array_size iterations. In each iteration we check whether the
> corresponding bit in the bitmask is set or not.
>
> What a nonsense. We have a bitmask already. Why not iterate over the
> bitmask and be done ?
>

Bitmask can be sparsed. Num represents the number of bits we have to find.
The idea is that we don't need to scan the entire bitmask, we stop as soon as
we have found all the bits we care about (i.e., all the bits that are set).

Example:
         num = 3
     bitmask=0000000010001001
                                    ^ we will iterate until we are
done with that bit.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/