Re: [PATCH v3 1/2] perf, x86: Implement event scheduler helper functions

[Robert Richter <robert.richter@amd.com>]

On 16.11.11 17:02:16, Peter Zijlstra wrote:
> On Mon, 2011-11-14 at 18:51 +0100, Robert Richter wrote:
> > @@ -22,8 +22,14 @@ extern unsigned long __sw_hweight64(__u64 w);
> >  #include <asm/bitops.h>
> >  
> >  #define for_each_set_bit(bit, addr, size) \
> > -       for ((bit) = find_first_bit((addr), (size)); \
> > -            (bit) < (size); \
> > +       for ((bit) = find_first_bit((addr), (size));            \
> > +            (bit) < (size);                                    \
> > +            (bit) = find_next_bit((addr), (size), (bit) + 1))
> > +
> > +/* same as for_each_set_bit() but use bit as value to start with */
> > +#define for_each_set_bit_cont(bit, addr, size) \
> > +       for ((bit) = find_next_bit((addr), (size), (bit));      \
> > +            (bit) < (size);                                    \
> >              (bit) = find_next_bit((addr), (size), (bit) + 1)) 
> 
> So my version has the +1 for the first as well, this is from the
> assumption that the bit passed in has been dealt with and should not be
> the first. ie. cont _after_ @bit instead of cont _at_ @bit.
> 
> This seems consistent with the list_*_continue primitives as well, which
> will start with the element after (or before for _reverse) the given
> position.
> 
> Thoughts?

The problem is that you then can't start with bit 0 unless you pass a
-1 which seems unsane.

You hit this actually too in your code with

	for_each_set_bit_cont(j, c->idxmsk, X86_PMC_IDX_MAX)
		...

Your intention was to start with X86_PMC_IDX_MAX, the first fixed
counter. But you always started with X86_PMC_IDX_MAX+1 never getting
the first fixed counter.

With list_*_continue it is slightly different because there is the
list header that *points* to the first element. Thus it can start with
the fist element of the list too by passing the list header.

In the end, passing the first bit to start with seems more logically.

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center