linux-kernel - Re: [PATCH v3 02/11] perf: add PMU_FORMAT_RANGE() helper for use by sw-like pmus

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 for Android: free password hash cracker in your pocket

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

Date:	Tue, 04 Mar 2014 00:09:19 -0800
From:	Cody P Schafer <cody@...ux.vnet.ibm.com>
To:	Michael Ellerman <mpe@...erman.id.au>,
	Linux PPC <linuxppc-dev@...ts.ozlabs.org>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>,
	Ingo Molnar <mingo@...hat.com>,
	Paul Mackerras <paulus@...ba.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
CC:	Peter Zijlstra <peterz@...radead.org>,
	LKML <linux-kernel@...r.kernel.org>, scottwood@...escale.com
Subject: Re: [PATCH v3 02/11] perf: add PMU_FORMAT_RANGE() helper for use
 by sw-like pmus

On 03/03/2014 09:19 PM, Michael Ellerman wrote:
> On Thu, 2014-27-02 at 21:04:55 UTC, Cody P Schafer wrote:
>> Add PMU_FORMAT_RANGE() and PMU_FORMAT_RANGE_RESERVED() (for reserved
>> areas) which generate functions to extract the relevent bits from
>> event->attr.config{,1,2} for use by sw-like pmus where the
>> 'config{,1,2}' values don't map directly to hardware registers.
>>
>> Signed-off-by: Cody P Schafer <cody@...ux.vnet.ibm.com>
>> ---
>>   include/linux/perf_event.h | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index e56b07f..3da5081 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -871,4 +871,21 @@ _name##_show(struct device *dev,					\
>>   									\
>>   static struct device_attribute format_attr_##_name = __ATTR_RO(_name)
>>
>> +#define PMU_FORMAT_RANGE(name, attr_var, bit_start, bit_end)		\
>> +PMU_FORMAT_ATTR(name, #attr_var ":" #bit_start "-" #bit_end);		\
>> +PMU_FORMAT_RANGE_RESERVED(name, attr_var, bit_start, bit_end)
>
> I really think these should have event in the name.
>
> Someone looking at the code is going to see event_get_foo() and wonder where
> that is defined. Grep won't find a definition, tags won't find a definition,
> the least you can do is have the macro name give some hint.
>

That is a good point (grep-ability). Let me think about this. There is 
also the possibility that I could adjust the event_get_*() naming to 
something else. format_get_*()? event_get_format_*()? (these names keep 
growing...)

>> +#define PMU_FORMAT_RANGE_RESERVED(name, attr_var, bit_start, bit_end)	\
>
> It doesn't generate a format attribute.

This was done with the idea that the term "format" didn't just refer to 
the attribute exposed in sysfs, it referred to "some subset of bits 
extractable from attr.config{,1,2}". Which is also the reasoning for the 
above naming.

>> +static u64 event_get_##name##_max(void)					\
>> +{									\
>> +	int bits = (bit_end) - (bit_start) + 1;				\
>> +	return ((0x1ULL << (bits - 1ULL)) - 1ULL) |			\
>> +		(0xFULL << (bits - 4ULL));				\
>
> What's wrong with:
>
> 	(0x1ULL << ((bit_end) - (bit_start) + 1)) - 1ULL;

Overflowing the << when bit_end = 63 and bit_start = 0 results in max(0, 
63) = 0.
That said, the current implementation is wrong when (bits < 4). Here's 
one that actually works (without overflowing):

         return (((1ull << (bit_end - bit_start)) - 1) << 1) + 1;

And an examination of the problematic case:

         #if 0
         typedef unsigned long long ull;
         ull a = bits - 1; /* 63 */
         ull b = 1 << a;   /* 0x8000000000000000 */
         ull c = b - 1;    /* 0x7fffffffffffffff */
         ull d = b << 1;   /* 0xfffffffffffffffe */
         ull e = d + 1;    /* 0xffffffffffffffff */
         return e;
         #endif

Small number of valid inputs, so I also tested it for all of them using

	unsigned bits = (bit_end) - (bit_start) + 1;
	return (bits < (sizeof(0ULL) * CHAR_BIT))
			? ((1ULL << bits) - 1ULL)
			: ~0ULL;

As the baseline correct one.

--
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/