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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20111116192331.GR15738@erda.amd.com>
Date:	Wed, 16 Nov 2011 20:23:31 +0100
From:	Robert Richter <robert.richter@....com>
To:	Peter Zijlstra <peterz@...radead.org>
CC:	Stephane Eranian <eranian@...gle.com>, Ingo Molnar <mingo@...e.hu>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] perf, x86: Implement event scheduler helper
 functions

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/