On Wednesday 22 July 2015 10:26 AM, Daniel Axtens wrote:

quoted

+static void p8_nest_read_counter(struct perf_event *event)
+{
+	uint64_t *addr;
+	u64 data = 0;

You've got a u64 and a uint64_t, and then...

quoted

+
+	addr = (u64 *)event->hw.event_base;

... you cast to event_base to a u64 pointer, which you assign to a
uint64_t pointer.

quoted

+	data = __be64_to_cpu(*addr);

And now you dereference the pointer.
Could you just have:
    data = __be64_to_cpu(*event->hw.event_base);

quoted

+	local64_set(&event->hw.prev_count, data);
+}
+
+static void p8_nest_perf_event_update(struct perf_event *event)
+{
+	u64 counter_prev, counter_new, final_count;
+	uint64_t *addr;
+
+	addr = (uint64_t *)event->hw.event_base;

Here at least the cast type is the same as the type of addr, but again,
why do you need the different types, and why local variable?

Damn sorry, copy paste errors. When I added debug prints i messed
the type case in both the functions. I will make them as uint64_t.

Thanks for this detail review
Maddy

quoted

+	counter_prev = local64_read(&event->hw.prev_count);
+	counter_new = __be64_to_cpu(*addr);
+	final_count = counter_new - counter_prev;
+
+	local64_set(&event->hw.prev_count, counter_new);
+	local64_add(final_count, &event->count);
+}
+
+static void p8_nest_event_start(struct perf_event *event, int flags)
+{
+	event->hw.state = 0;

Should this be an enum or a #define rather than a bare 0? (It may not
need to be, I was just wondering because I don't know what 0 means.)

I could remove it since was just initializing at the start.

quoted

+	p8_nest_read_counter(event);
+}
+