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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1299026246.4937.99.camel@minggr.sh.intel.com>
Date:	Wed, 02 Mar 2011 08:37:26 +0800
From:	Lin Ming <ming.m.lin@...el.com>
To:	Stephane Eranian <eranian@...gle.com>
Cc:	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Ingo Molnar <mingo@...e.hu>, Andi Kleen <andi@...stfloor.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/4] perf-events: Add support for supplementary
 event registers

On Wed, 2011-03-02 at 01:09 +0800, Stephane Eranian wrote:
> Lin,
> 
> See comments below:
> 
> On Tue, Mar 1, 2011 at 5:47 PM, Lin Ming <ming.m.lin@...el.com> wrote:
> > From: Andi Kleen <ak@...ux.intel.com>
> >  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.

Right, that is redundant.
Remove it.

> 
> > +               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.

config0 means perf_event_attr::config.

Peter suggested to extend the attr config space.
http://marc.info/?l=linux-kernel&m=129838808907525&w=2


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