Re: [PATCH v2 1/4] perf-events: Add support for supplementary event registers

[Stephane Eranian]

Lin,

See comments below:

On Tue, Mar 1, 2011 at 5:47 PM, Lin Ming <ming.m.lin@xxxxxxxxx> wrote:
> From: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> Âstatic struct event_constraint *
> +intel_percore_constraints(struct cpu_hw_events *cpuc, struct perf_event *event)
> +{
> + Â Â Â struct hw_perf_event *hwc = &event->hw;
> + Â Â Â unsigned int e = hwc->config & ARCH_PERFMON_EVENTSEL_EVENT;
> + Â Â Â struct event_constraint *c;
> + Â Â Â struct intel_percore *pc;
> + Â Â Â struct er_account *era;
> + Â Â Â int i;
> + Â Â Â int free_slot;
> + Â Â Â int found;
> +
> + Â Â Â if (!x86_pmu.percore_constraints || hwc->extra_alloc)
> + Â Â Â Â Â Â Â return NULL;
> +
> + Â Â Â for (c = x86_pmu.percore_constraints; c->cmask; c++) {
> + Â Â Â Â Â Â Â if (e != c->code)
> + Â Â Â Â Â Â Â Â Â Â Â continue;
> +
> + Â Â Â Â Â Â Â /*
> + Â Â Â Â Â Â Â Â* Allocate resource per core.
> + Â Â Â Â Â Â Â Â*/
> + Â Â Â Â Â Â Â c = NULL;

Not sure I understand this c = NULL, given you hardcoded return NULL
outside of the
loop. I forgot to mention this earlier.

> + Â Â Â Â Â Â Â pc = cpuc->per_core;
> + Â Â Â Â Â Â Â if (!pc)
> + Â Â Â Â Â Â Â Â Â Â Â break;
> + Â Â Â Â Â Â Â c = &emptyconstraint;
> + Â Â Â Â Â Â Â raw_spin_lock(&pc->lock);
> + Â Â Â Â Â Â Â free_slot = -1;
> + Â Â Â Â Â Â Â found = 0;
> + Â Â Â Â Â Â Â for (i = 0; i < MAX_EXTRA_REGS; i++) {
> + Â Â Â Â Â Â Â Â Â Â Â era = &pc->regs[i];
> + Â Â Â Â Â Â Â Â Â Â Â if (era->ref > 0 && hwc->extra_reg == era->extra_reg) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* Allow sharing same config */
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â if (hwc->extra_config == era->extra_config) {
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â era->ref++;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â cpuc->percore_used = 1;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â hwc->extra_alloc = 1;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â c = NULL;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â }
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â /* else conflict */
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â found = 1;
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â break;
> + Â Â Â Â Â Â Â Â Â Â Â } else if (era->ref == 0 && free_slot == -1)
> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â free_slot = i;
> + Â Â Â Â Â Â Â }
> + Â Â Â Â Â Â Â if (!found && free_slot != -1) {
> + Â Â Â Â Â Â Â Â Â Â Â era = &pc->regs[free_slot];
> + Â Â Â Â Â Â Â Â Â Â Â era->ref = 1;
> + Â Â Â Â Â Â Â Â Â Â Â era->extra_reg = hwc->extra_reg;
> + Â Â Â Â Â Â Â Â Â Â Â era->extra_config = hwc->extra_config;
> + Â Â Â Â Â Â Â Â Â Â Â cpuc->percore_used = 1;
> + Â Â Â Â Â Â Â Â Â Â Â hwc->extra_alloc = 1;
> + Â Â Â Â Â Â Â Â Â Â Â c = NULL;
> + Â Â Â Â Â Â Â }
> + Â Â Â Â Â Â Â raw_spin_unlock(&pc->lock);
> + Â Â Â Â Â Â Â return c;
> + Â Â Â }
> +
> + Â Â Â return NULL;
> +}
> +
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 8ceb5a6..48d966a 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -225,8 +225,14 @@ struct perf_event_attr {
> Â Â Â Â};
>
> Â Â Â Â__u32 Â Â Â Â Â Â Â Â Â bp_type;
> - Â Â Â __u64 Â Â Â Â Â Â Â Â Â bp_addr;
> - Â Â Â __u64 Â Â Â Â Â Â Â Â Â bp_len;
> + Â Â Â union {
> + Â Â Â Â Â Â Â __u64 Â Â Â Â Â bp_addr;
> + Â Â Â Â Â Â Â __u64 Â Â Â Â Â config1; /* extension of config0 */
> + Â Â Â };
> + Â Â Â union {
> + Â Â Â Â Â Â Â __u64 Â Â Â Â Â bp_len;
> + Â Â Â Â Â Â Â __u64 Â Â Â Â Â config2; /* extension of config1 */
> + Â Â Â };
> Â};
>
I don't see where those config0 or config1 are coming from.
N‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i