lkml.org 
[lkml]   [2014]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@linux.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.



\
 
 \ /
  Last update: 2014-03-04 09:41    [W:0.088 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site