| 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
| ||
|
Message-ID: <06033C969873E840BDE9FC9B584F66B519433D5D@ORSMSX116.amr.corp.intel.com>
Date: Mon, 3 Aug 2015 17:51:08 +0000
From: "Juvva, Kanaka D" <kanaka.d.juvva@...el.com>
To: Thomas Gleixner <tglx@...utronix.de>,
Kanaka Juvva <kanaka.d.juvva@...ux.intel.com>
CC: "Williamson, Glenn P" <glenn.p.williamson@...el.com>,
"Fleming, Matt" <matt.fleming@...el.com>,
"Auld, Will" <will.auld@...el.com>,
Andi Kleen <andi@...stfloor.org>,
LKML <linux-kernel@...r.kernel.org>,
"Luck, Tony" <tony.luck@...el.com>,
Peter Zijlstra <peterz@...radead.org>,
Tejun Heo <tj@...nel.org>,
"Autee, Priya V" <priya.v.autee@...el.com>,
"x86@...nel.org" <x86@...nel.org>,
"mingo@...hat.com" <mingo@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>,
"Shivappa, Vikas" <vikas.shivappa@...el.com>
Subject: RE: [PATCH v2] perf,x86: add Intel Memory Bandwidth Monitoring
(MBM) PMU
Thomas,
Acknowledged. I'll update the patch and generate separate patch(s) wherever cqm changes
are required. One thing was checkpatch.pl gave errors in current cqm code. I had to break lines
as it complained about some line(s) exceeding 80 chars.
Regards.
-Kanaka
> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@...utronix.de]
> Sent: Tuesday, July 28, 2015 12:59 AM
> To: Kanaka Juvva
> Cc: Juvva, Kanaka D; Williamson, Glenn P; Fleming, Matt; Auld, Will; Andi Kleen;
> LKML; Luck, Tony; Peter Zijlstra; Tejun Heo; Autee, Priya V; x86@...nel.org;
> mingo@...hat.com; H. Peter Anvin; Shivappa, Vikas
> Subject: Re: [PATCH v2] perf,x86: add Intel Memory Bandwidth Monitoring
> (MBM) PMU
>
> On Tue, 21 Jul 2015, Kanaka Juvva wrote:
> > diff --git a/arch/x86/kernel/cpu/perf_event_intel_cqm.c
> > b/arch/x86/kernel/cpu/perf_event_intel_cqm.c
> > index 1880761..dc1ce58 100644
> > --- a/arch/x86/kernel/cpu/perf_event_intel_cqm.c
> > +++ b/arch/x86/kernel/cpu/perf_event_intel_cqm.c
> > @@ -12,10 +12,20 @@
> > #define MSR_IA32_PQR_ASSOC 0x0c8f
> > #define MSR_IA32_QM_CTR 0x0c8e
> > #define MSR_IA32_QM_EVTSEL 0x0c8d
> > +#define MAX_MBM_CNTR 0xffffff
>
> Can we please have a consistent name space MBM_... here?
>
> > +#define MBM_SOCKET_MAX 8
> > +#define MBM_TIME_DELTA_MAX 1000
> > +#define MBM_TIME_DELTA_MIN 1000
> > +#define MBM_SCALING_FACTOR 1000
> > +#define MBM_SCALING_HALF (MBM_SCALING_FACTOR/2)
> > +#define MBM_FIFO_SIZE_MIN 10
> > +#define MBM_FIFO_SIZE_MAX 300
> >
> > -static u32 cqm_max_rmid = -1;
> > -static unsigned int cqm_l3_scale; /* supposedly cacheline size */
> >
> > +static u32 cqm_max_rmid = -1;
> > +static unsigned int cqm_l3_scale;/* supposedly cacheline size */
>
> Pointless white space change
>
> > +static unsigned mbm_window_size = MBM_FIFO_SIZE_MIN; static bool
> > +cqm_llc_occ, cqm_mbm;
> > /**
> > * struct intel_pqr_state - State cache for the PQR MSR
> > * @rmid: The cached Resource Monitoring ID
> > @@ -42,6 +52,34 @@ struct intel_pqr_state {
> > * interrupts disabled, which is sufficient for the protection.
> > */
> > static DEFINE_PER_CPU(struct intel_pqr_state, pqr_state);
> > +static DEFINE_PER_CPU(struct mbm_pmu *, mbm_pmu); static
> > +DEFINE_PER_CPU(struct mbm_pmu *, mbm_pmu_to_free);
> > +
> > +/*
> > + * mbm pmu is used for storing mbm_local and mbm_total
> > + * events
> > + */
> > +struct mbm_pmu {
> > + spinlock_t lock;
>
> What is this lock protecting and how does it nest into perf locking?
>
> > + int n_active; /* number of active events */
>
> If you want to document your struct members, please use kerneldoc and not
> these hard to read tail comments.
>
> > + struct list_head active_list;
> > + struct pmu *pmu; /* pointer to mbm perf_event */
> > + ktime_t timer_interval; /* in ktime_t unit */
> > + struct hrtimer hrtimer;
> > +};
> > +
> > +struct sample {
> > + u64 bytes; /* mbm counter value read*/
> > + u64 cum_avg; /* current running average of bandwidth */
> > + ktime_t prev_time;
> > + u64 index; /* current sample no */
> > + u32 mbmfifo[MBM_FIFO_SIZE_MAX]; /* window of last n bw values */
> > + u32 fifoin; /* mbmfifo in counter for sliding window */
> > + u32 fifoout; /* mbmfifo out counter for sliding window */
>
> So above you use a proper layout which is actualy readable (aside of the tail
> comments). Can you please stick with a consistent coding style?
>
> > +};
> > +
> > +struct sample *mbm_total; /* curent stats for mbm_total events */
> > +struct sample *mbm_local; /* current stats for mbm_local events */
>
> static ? And please get rid of these tail comments.
>
> > /*
> > * Protects cache_cgroups and cqm_rmid_free_lru and cqm_rmid_limbo_lru.
> > @@ -66,6 +104,9 @@ static cpumask_t cqm_cpumask;
> > #define RMID_VAL_UNAVAIL (1ULL << 62)
> >
> > #define QOS_L3_OCCUP_EVENT_ID (1 << 0)
> > +#define QOS_MBM_TOTAL_EVENT_ID (1 << 1)
>
> Please use tabs not spaces.
>
> > +#define QOS_MBM_LOCAL_EVENT_ID1 0x3
>
> What's ID1 for heavens sake? Looks like
>
> (QOS_L3_OCCUP_EVENT_ID | QOS_MBM_TOTAL_EVENT_ID)
>
> So this wants a descriptive ID name and a comment.
>
> > +#define QOS_MBM_LOCAL_EVENT_ID (1 << 2)
> >
> > #define QOS_EVENT_MASK QOS_L3_OCCUP_EVENT_ID
> >
> > @@ -90,7 +131,8 @@ static u32 intel_cqm_rotation_rmid;
> > */
> > static inline bool __rmid_valid(u32 rmid) {
> > - if (!rmid || rmid == INVALID_RMID)
> > + WARN_ON_ONCE(rmid > cqm_max_rmid);
> > + if (!rmid || (rmid == INVALID_RMID) || (rmid > cqm_max_rmid))
> > return false;
>
> How is that related to this patch? Looks like a fix or prerequisite change to the
> existing code.
>
> > return true;
> > @@ -125,6 +167,7 @@ struct cqm_rmid_entry {
> > enum rmid_recycle_state state;
> > struct list_head list;
> > unsigned long queue_time;
> > + bool config;
> > };
> >
> > /*
> > @@ -176,6 +219,17 @@ static inline struct cqm_rmid_entry
> *__rmid_entry(u32 rmid)
> > return entry;
> > }
> >
> > +static void mbm_reset_stats(u32 rmid) {
> > + u32 vrmid = topology_physical_package_id(smp_processor_id()) *
> > + cqm_max_rmid + rmid;
>
> Can you please explain that magic calculation? It's far from obvious why this
> would be correct.
>
> > +
> > + if ((!cqm_mbm) || (!mbm_local) || (!mbm_total))
> > + return;
> > + memset(&mbm_local[vrmid], 0, sizeof(struct sample));
> > + memset(&mbm_total[vrmid], 0, sizeof(struct sample)); }
> > +
> > /*
> > * Returns < 0 on fail.
> > *
> > @@ -190,8 +244,10 @@ static u32 __get_rmid(void)
> > if (list_empty(&cqm_rmid_free_lru))
> > return INVALID_RMID;
> >
> > - entry = list_first_entry(&cqm_rmid_free_lru, struct cqm_rmid_entry,
> list);
> > + entry = list_first_entry(&cqm_rmid_free_lru,
> > + struct cqm_rmid_entry, list);
>
> And the point of this change is?
>
> > list_del(&entry->list);
> > + mbm_reset_stats(entry->rmid);
> >
> > return entry->rmid;
> > }
> > @@ -207,6 +263,7 @@ static void __put_rmid(u32 rmid)
> >
> > entry->queue_time = jiffies;
> > entry->state = RMID_YOUNG;
> > + mbm_reset_stats(rmid);
> >
> > list_add_tail(&entry->list, &cqm_rmid_limbo_lru); } @@ -232,6
> > +289,8 @@ static int intel_cqm_setup_rmid_cache(void)
> >
> > INIT_LIST_HEAD(&entry->list);
> > entry->rmid = r;
> > + entry->config = false;
> > +
> > cqm_rmid_ptrs[r] = entry;
> >
> > list_add_tail(&entry->list, &cqm_rmid_free_lru); @@ -254,6
> +313,8
> > @@ fail:
> > kfree(cqm_rmid_ptrs[r]);
> >
> > kfree(cqm_rmid_ptrs);
> > + kfree(mbm_local);
> > + kfree(mbm_total);
> > return -ENOMEM;
> > }
> >
> > @@ -403,9 +464,11 @@ static void __intel_cqm_event_count(void *info);
> > static u32 intel_cqm_xchg_rmid(struct perf_event *group, u32 rmid) {
> > struct perf_event *event;
> > +
>
> Superfluous newline.
>
> > struct list_head *head = &group->hw.cqm_group_entry;
> > u32 old_rmid = group->hw.cqm_rmid;
> >
> > +
>
> Another one. Sigh!
>
> > lockdep_assert_held(&cache_mutex);
> >
> > /*
> > @@ -494,6 +557,166 @@ static bool intel_cqm_sched_in_event(u32 rmid)
> > return false;
> > }
> >
> > +static u32 mbm_fifo_sum_lastn_out(int rmid, bool is_localbw)
>
> Random space after u32
>
> > +{
> > + u32 val = 0, i, j, index;
> > +
> > + if (is_localbw) {
> > + if (++mbm_local[rmid].fifoout >= mbm_window_size)
> > + mbm_local[rmid].fifoout = 0;
> > + index = mbm_local[rmid].fifoout;
> > + for (i = 0; i < mbm_window_size - 1; i++) {
> > + if (index + i >= mbm_window_size)
> > + j = index + i - mbm_window_size;
> > + else
> > + j = index + i;
> > + val += mbm_local[rmid].mbmfifo[j];
> > + }
> > + return val;
> > + }
> > +
> > + if (++mbm_total[rmid].fifoout >= mbm_window_size)
> > + mbm_total[rmid].fifoout = 0;
> > + index = mbm_total[rmid].fifoout;
> > + for (i = 0; i < mbm_window_size - 1; i++) {
> > + if (index + i >= mbm_window_size)
> > + j = index + i - mbm_window_size;
> > + else
> > + j = index + i;
> > + val += mbm_total[rmid].mbmfifo[j];
> > + }
>
> The concept of helper functions to avoid code duplication is
> definitely applicable here.
>
> > + return val;
> > +}
> > +
> > +static int mbm_fifo_in(int rmid, u32 val, bool is_localbw)
>
> Random space after int.
>
> > +{
> > + if (is_localbw) {
> > + mbm_local[rmid].mbmfifo[mbm_local[rmid].fifoin] = val;
> > + if (++mbm_local[rmid].fifoin >= mbm_window_size)
> > + mbm_local[rmid].fifoin = 0;
> > + } else {
> > + mbm_total[rmid].mbmfifo[mbm_total[rmid].fifoin] = val;
> > + if (++mbm_total[rmid].fifoin >= mbm_window_size)
> > + mbm_total[rmid].fifoin = 0;
> > + }
>
> static void mbm_fifo_in(struct sample *, u32 val)
>
> would get rid of code duplication.
>
> > + return 0;
>
> What's the purpose of having a return value here?
>
> > +}
> > +
> > +/*
> > + * __rmid_read_mbm checks whether it is LOCAL or GLOBAL MBM event and
> reads
> > + * its MSR counter. if (MSR current value < MSR previous value) it is an
> > + * overflow and overflow is handled. If MSR is read within last 100ms,
> > + * then the value is ignored; this will suppress small deltas. We don't
> > + * process MBM samples that are within 100ms. Bandwidth is calculated as:
> > + * bandwidth = difference of last two msr counter values/time difference.
> > + * cum_avg = Running Average bandwidth of last 'n' samples that are
> processed
> > + * Sliding window is used to save the last 'n' samples. Hence,
> > + * n = sliding_window_size
> > + * cum_avg is scaled down by a factor MBM_SCALING_FACTOR and rounded
> to nearest
> > + * integer. User interface reads the BW in MB/sec.
> > + * Rounding algorithm : (X + 0.5):
> > + * where X is scaled BW value. To avoid floating point arithmetic :
> > + * BW- unscaled value
> > + * (BW + MBM_SCALING_HALF)/MBM_SCALING_FACTOR is computed which
> gives the
> > + * scaled bandwidth.
>
> This is completely unreadable.
>
> > + */
> > +static u64 __rmid_read_mbm(unsigned int rmid, bool read_mbm_local)
> > +{
> > + u64 val, tmp, diff_time, cma, bytes, index;
> > + bool overflow = false;
> > + ktime_t cur_time;
> > + u32 tmp32 = rmid;
> > + u32 vrmid = topology_physical_package_id(smp_processor_id()) *
> > + cqm_max_rmid + rmid;
> > +
> > + rmid = vrmid;
>
> This is completely backwards.
>
> tmp32 = rmid;
> rmid = vrmid;
> do_stuff(rmid);
> rmid = tmp32;
> do_other_stuff(rmid);
>
> Why can't you use vrmid for do_stuff() and leave rmid alone? Just
> because it would make the code simpler to read?
>
> > + cur_time = ktime_get_real();
>
> Why would this operate on wall time and not on clock monotonic time?
>
> > + if (read_mbm_local) {
> > + cma = mbm_local[rmid].cum_avg;
> > + diff_time = ktime_ms_delta(cur_time,
> > + mbm_local[rmid].prev_time);
> > + if (diff_time < 100)
> > + return cma;
> > + mbm_local[rmid].prev_time = ktime_set(0,
> > + (unsigned long)ktime_to_ns(cur_time));
> > + bytes = mbm_local[rmid].bytes;
> > + index = mbm_local[rmid].index;
> > + rmid = tmp32;
> > + wrmsr(MSR_IA32_QM_EVTSEL, QOS_MBM_LOCAL_EVENT_ID1,
> rmid);
>
> Why is this using ID1?
>
> > + } else {
> > + cma = mbm_total[rmid].cum_avg;
> > + diff_time = ktime_ms_delta(cur_time,
> > + mbm_total[rmid].prev_time);
> > + if (diff_time < 100)
> > + return cma;
> > + mbm_total[rmid].prev_time = ktime_set(0,
> > + (unsigned long)ktime_to_ns(cur_time));
> > + bytes = mbm_total[rmid].bytes;
> > + index = mbm_total[rmid].index;
> > + rmid = tmp32;
> > + wrmsr(MSR_IA32_QM_EVTSEL, QOS_MBM_TOTAL_EVENT_ID,
> rmid);
> > + }
>
> Random space after tab.
>
> > + rdmsrl(MSR_IA32_QM_CTR, val);
> > + if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
> > + return val;
> > +
> > + tmp = val;
>
> You really have a faible for random storage. tmp gets assigned to
> mbm_*[rmid].bytes further down. This makes no sense at all.
>
> > + if (val < bytes) /* overflow handle it */ {
>
> These tail comments are crap.
>
> > + val = MAX_MBM_CNTR - bytes + val;
> > + overflow = true;
> > + } else
> > + val = val - bytes;
>
> That else clause is missing braces.
>
> > + if (diff_time < MBM_TIME_DELTA_MAX - MBM_TIME_DELTA_MIN)
> > + val = (val * MBM_TIME_DELTA_MAX) / diff_time;
>
> That resolves to
>
> if (diff_time < 0)
>
> Which is always false because diff_time is u64.
>
> What's the logic behind this?
>
> > +
> > + if ((diff_time > MBM_TIME_DELTA_MAX) && (!cma))
> > + /* First sample, we can't use the time delta */
> > + diff_time = MBM_TIME_DELTA_MAX;
>
> And this?
>
> > +
> > + rmid = vrmid;
>
> More obfuscation
>
> > + if ((diff_time <= MBM_TIME_DELTA_MAX + MBM_TIME_DELTA_MIN)
> ||
> > + overflow) {
>
> Again, that resolves to
>
> if (overflow)
>
> And even if MBM_TIME_DELTA_MAX would be not equal
> MBM_TIME_DELTA_MIN
> then this wants some explanation about
>
> MBM_TIME_DELTA_MAX - MBM_TIME_DELTA_MIN
>
> versus
>
> MBM_TIME_DELTA_MAX + MBM_TIME_DELTA_MIN
>
> and the whole logic behind this time delta magic, if there is any.
>
> > + int bw, ret;
> > +
> > + if (index & (index < mbm_window_size))
> > + val = cma * MBM_SCALING_FACTOR + val / index -
> > + cma / index;
> > +
> > + val = (val + MBM_SCALING_HALF) / MBM_SCALING_FACTOR;
> > + if (index >= mbm_window_size) {
>
> These conditionals along with the magic math are undocumented.
>
> > + /*
> > + * Compute the sum of recent n-1 samples and slide the
> > + * window by 1
> > + */
> > + ret = mbm_fifo_sum_lastn_out(rmid, read_mbm_local);
> > + /* recalculate the running average with current bw */
> > + ret = (ret + val) / mbm_window_size;
> > + if (ret < 0)
>
> The average of positive numbers becomes negative ?
>
> > + ret = 0;
> > + bw = val;
> > + val = ret;
>
> Your back and forth assignements of random variables is not making the
> code any more readable.
>
> > + } else
> > + bw = val;
> > + /* save the recent bw in fifo */
> > + mbm_fifo_in(rmid, bw, read_mbm_local);
> > +
> > + if (read_mbm_local) {
>
> Oh well. You have this read_mbm_local conditional 3 times in this
> function. Why don't you make the function oblivious of this and hand
> in a pointer to mbm_local or mbm_total? Would be too simple and not
> enough obfuscated, right?
>
> > + mbm_local[rmid].index++;
> > + mbm_local[rmid].cum_avg = val;
> > + mbm_local[rmid].bytes = tmp;
> > + mbm_local[rmid].prev_time = ktime_set(0,
> > + (unsigned long)ktime_to_ns(cur_time));
> > + } else {
> > + mbm_total[rmid].index++;
> > + mbm_total[rmid].cum_avg = val;
> > + mbm_total[rmid].bytes = tmp;
> > + mbm_total[rmid].prev_time = ktime_set(0,
> > + (unsigned long)ktime_to_ns(cur_time));
> > + }
> > + return val;
> > + }
> > + return cma;
> > +}
> > +
> > /*
> > * Initially use this constant for both the limbo queue time and the
> > * rotation timer interval, pmu::hrtimer_interval_ms.
> > @@ -568,7 +791,8 @@ static bool intel_cqm_rmid_stabilize(unsigned int
> *available)
> > /*
> > * Test whether an RMID is free for each package.
> > */
> > - on_each_cpu_mask(&cqm_cpumask, intel_cqm_stable, NULL, true);
> > + if (entry->config)
>
> This entry->config change wants to be a separate patch. It's
> completely non obvious how this changes the behaviour of the existing code.
>
> > + on_each_cpu_mask(&cqm_cpumask, intel_cqm_stable, NULL,
> true);
> >
> > list_for_each_entry_safe(entry, tmp, &cqm_rmid_limbo_lru, list) {
> > /*
> > @@ -846,8 +1070,9 @@ static void intel_cqm_setup_event(struct perf_event
> *event,
> > struct perf_event **group)
> > {
> > struct perf_event *iter;
> > - bool conflict = false;
> > +
> > u32 rmid;
> > + bool conflict = false;
>
> Fits the overall picture of this patch: Sloppy
>
> > list_for_each_entry(iter, &cache_groups, hw.cqm_groups_entry) {
> > rmid = iter->hw.cqm_rmid;
> > @@ -875,6 +1100,40 @@ static void intel_cqm_setup_event(struct
> perf_event *event,
> > event->hw.cqm_rmid = rmid;
> > }
> >
> > +static void intel_cqm_event_update(struct perf_event *event)
> > +{
> > + unsigned int rmid;
> > + u64 val = 0;
> > +
> > + /*
> > + * Task events are handled by intel_cqm_event_count().
> > + */
> > + if (event->cpu == -1)
> > + return;
> > +
> > + rmid = event->hw.cqm_rmid;
> > + if (!__rmid_valid(rmid))
> > + return;
> > +
> > + switch (event->attr.config) {
> > + case QOS_MBM_TOTAL_EVENT_ID:
> > + val = __rmid_read_mbm(rmid, false);
> > + break;
> > + case QOS_MBM_LOCAL_EVENT_ID:
> > + val = __rmid_read_mbm(rmid, true);
> > + break;
> > + default:
> > + break;
>
> return?
>
> > + }
> > + /*
> > + * Ignore this reading on error states and do not update the value.
> > + */
> > + if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
> > + return;
> > +
> > + local64_set(&event->count, val);
> > +}
> > +
> > static void intel_cqm_event_read(struct perf_event *event)
> > {
> > unsigned long flags;
> > @@ -887,6 +1146,12 @@ static void intel_cqm_event_read(struct perf_event
> *event)
> > if (event->cpu == -1)
> > return;
> >
> > + if ((event->attr.config & QOS_MBM_TOTAL_EVENT_ID) ||
> > + (event->attr.config & QOS_MBM_LOCAL_EVENT_ID))
> > + intel_cqm_event_update(event);
> > +
> > + if (!(event->attr.config & QOS_L3_OCCUP_EVENT_ID))
> > + return;
> > raw_spin_lock_irqsave(&cache_lock, flags);
> > rmid = event->hw.cqm_rmid;
> >
> > @@ -906,6 +1171,28 @@ out:
> > raw_spin_unlock_irqrestore(&cache_lock, flags);
> > }
> >
> > +static void __intel_cqm_event_total_bw_count(void *info)
> > +{
> > + struct rmid_read *rr = info;
> > + u64 val;
> > +
> > + val = __rmid_read_mbm(rr->rmid, false);
> > + if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
> > + return;
> > + atomic64_add(val, &rr->value);
> > +}
> > +
> > +static void __intel_cqm_event_local_bw_count(void *info)
> > +{
> > + struct rmid_read *rr = info;
> > + u64 val;
> > +
> > + val = __rmid_read_mbm(rr->rmid, true);
> > + if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
> > + return;
> > + atomic64_add(val, &rr->value);
>
> You're really a fan of copy and paste.
>
> > +}
> > +
> > static void __intel_cqm_event_count(void *info)
> > {
> > struct rmid_read *rr = info;
> > @@ -967,7 +1254,21 @@ static u64 intel_cqm_event_count(struct perf_event
> *event)
> > if (!__rmid_valid(rr.rmid))
> > goto out;
> >
> > - on_each_cpu_mask(&cqm_cpumask, __intel_cqm_event_count, &rr, 1);
> > + switch (event->attr.config) {
> > + case QOS_L3_OCCUP_EVENT_ID:
> > + on_each_cpu_mask(&cqm_cpumask,
> __intel_cqm_event_count, &rr, 1);
> > + break;
> > + case QOS_MBM_TOTAL_EVENT_ID:
> > + on_each_cpu_mask(&cqm_cpumask,
> __intel_cqm_event_total_bw_count,
> > + &rr, 1);
> > + break;
> > + case QOS_MBM_LOCAL_EVENT_ID:
> > + on_each_cpu_mask(&cqm_cpumask,
> __intel_cqm_event_local_bw_count,
> > + &rr, 1);
> > + break;
> > + default:
> > + break;
>
> So there is nothing to do and you happily proceed?
>
> > + }
> >
> > raw_spin_lock_irqsave(&cache_lock, flags);
> > if (event->hw.cqm_rmid == rr.rmid)
> > @@ -977,6 +1278,39 @@ out:
> > return __perf_event_count(event);
> > }
>
> > static void intel_cqm_event_start(struct perf_event *event, int mode)
> > {
> > struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
> > @@ -995,19 +1329,48 @@ static void intel_cqm_event_start(struct
> perf_event *event, int mode)
> > }
> >
> > state->rmid = rmid;
> > + if (event->attr.config & QOS_L3_OCCUP_EVENT_ID) {
> > + struct cqm_rmid_entry *entry;
> > +
> > + entry = __rmid_entry(rmid);
> > + entry->config = true;
> > + }
> > wrmsr(MSR_IA32_PQR_ASSOC, rmid, state->closid);
> > +
> > + if (((event->attr.config & QOS_MBM_TOTAL_EVENT_ID) ||
> > + (event->attr.config & QOS_MBM_LOCAL_EVENT_ID)) &&
> (cqm_mbm)) {
> > + int cpu = get_cpu();
>
> pmu->start() is called with interrupts disabled. So why do you need
> get_cpu() here?
>
> > + struct mbm_pmu *pmu = per_cpu(mbm_pmu, cpu);
> > +
> > + if (pmu) {
> > + if (pmu->n_active == 0)
> > + mbm_hrtimer_init(pmu);
>
> That pmu timer wants to be initialized when the pmu is initialized.
>
> > + pmu->n_active++;
> > + list_add_tail(&event->active_entry,
> > + &pmu->active_list);
> > + if (pmu->n_active == 1)
> > + mbm_start_hrtimer(pmu);
> > + }
> > + put_cpu();
> > + }
> > }
> >
> > static void intel_cqm_event_stop(struct perf_event *event, int mode)
> > {
> > struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
> > + struct mbm_pmu *pmu = __this_cpu_read(mbm_pmu);
> >
> > if (event->hw.cqm_state & PERF_HES_STOPPED)
> > return;
> >
> > event->hw.cqm_state |= PERF_HES_STOPPED;
> >
> > - intel_cqm_event_read(event);
> > + if (event->attr.config & QOS_L3_OCCUP_EVENT_ID)
> > + intel_cqm_event_read(event);
> > +
> > + if ((event->attr.config & QOS_MBM_TOTAL_EVENT_ID) ||
> > + (event->attr.config & QOS_MBM_LOCAL_EVENT_ID))
> > + intel_cqm_event_update(event);
> >
> > if (!--state->rmid_usecnt) {
> > state->rmid = 0;
> > @@ -1015,8 +1378,18 @@ static void intel_cqm_event_stop(struct
> perf_event *event, int mode)
> > } else {
> > WARN_ON_ONCE(!state->rmid);
> > }
> > +
> > + if (pmu) {
> > + WARN_ON_ONCE(pmu->n_active <= 0);
> > + pmu->n_active--;
> > + if (pmu->n_active == 0)
> > + mbm_stop_hrtimer(pmu);
> > + list_del(&event->active_entry);
> > + }
> > +
> > }
> >
> > +
>
> Sigh
>
> > static int intel_cqm_event_add(struct perf_event *event, int mode)
> > {
>
> > +static ssize_t
> > +sliding_window_size_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + unsigned int bytes;
> > + int ret;
> > +
> > + ret = kstrtouint(buf, 0, &bytes);
> > + if (ret)
> > + return ret;
> > +
> > + mutex_lock(&cache_mutex);
> > + if (bytes > 0 && bytes <= MBM_FIFO_SIZE_MAX)
> > + mbm_window_size = bytes;
> > + else
> > + bytes = MBM_FIFO_SIZE_MIN;
>
> What kind of twisted logic is that?
>
> > +
> > + mutex_unlock(&cache_mutex);
> > +
> > + return count;
> > +}
> > +
> > static DEVICE_ATTR_RW(max_recycle_threshold);
> > +static DEVICE_ATTR_RW(sliding_window_size);
> >
> > static struct attribute *intel_cqm_attrs[] = {
> > &dev_attr_max_recycle_threshold.attr,
> > + &dev_attr_sliding_window_size.attr,
> > NULL,
> > };
> >
> > @@ -1241,16 +1699,17 @@ static inline void cqm_pick_event_reader(int cpu)
> >
> > for_each_cpu(i, &cqm_cpumask) {
> > if (phys_id == topology_physical_package_id(i))
> > - return; /* already got reader for this socket */
> > + return; /* already got reader for this socket */
>
> More random crap.
>
> > }
> >
> > cpumask_set_cpu(cpu, &cqm_cpumask);
> > }
> >
> > -static void intel_cqm_cpu_prepare(unsigned int cpu)
> > +static int intel_cqm_cpu_prepare(unsigned int cpu)
> > {
> > struct intel_pqr_state *state = &per_cpu(pqr_state, cpu);
> > struct cpuinfo_x86 *c = &cpu_data(cpu);
> > + struct mbm_pmu *pmu = per_cpu(mbm_pmu, cpu);
> >
> > state->rmid = 0;
> > state->closid = 0;
> > @@ -1258,12 +1717,27 @@ static void intel_cqm_cpu_prepare(unsigned int
> cpu)
> >
> > WARN_ON(c->x86_cache_max_rmid != cqm_max_rmid);
> > WARN_ON(c->x86_cache_occ_scale != cqm_l3_scale);
> > +
> > + if ((!pmu) && (cqm_mbm)) {
> > + pmu = kzalloc_node(sizeof(*mbm_pmu), GFP_KERNEL,
> NUMA_NO_NODE);
> > + if (!pmu)
> > + return -ENOMEM;
> > + spin_lock_init(&pmu->lock);
> > + INIT_LIST_HEAD(&pmu->active_list);
> > + pmu->pmu = &intel_cqm_pmu;
> > + pmu->n_active = 0;
> > + pmu->timer_interval = ms_to_ktime(MBM_TIME_DELTA_MAX);
> > + per_cpu(mbm_pmu, cpu) = pmu;
> > + per_cpu(mbm_pmu_to_free, cpu) = NULL;
>
> So that mbm_pmu_to_free per cpu variable is NULL already and never
> becomes anything else than NULL. What's the purpose of setting it NULL
> again? And what's the purpose of it in general?
>
> > + }
> > + return 0;
> > }
> >
> > static void intel_cqm_cpu_exit(unsigned int cpu)
> > {
> > int phys_id = topology_physical_package_id(cpu);
> > int i;
> > + struct mbm_pmu *pmu = per_cpu(mbm_pmu, cpu);
> >
> > /*
> > * Is @cpu a designated cqm reader?
> > @@ -1280,6 +1754,13 @@ static void intel_cqm_cpu_exit(unsigned int cpu)
> > break;
> > }
> > }
> > +
> > + /* cancel overflow polling timer for CPU */
> > + if (pmu)
> > + mbm_stop_hrtimer(pmu);
> > + kfree(mbm_local);
> > + kfree(mbm_total);
>
> So this frees the module global storage if one cpu is unplugged and
> leaves the pointer initialized so the rest of the code can happily
> dereference it.
>
> > +
> > }
> >
> > static int intel_cqm_cpu_notifier(struct notifier_block *nb,
> > @@ -1289,7 +1770,7 @@ static int intel_cqm_cpu_notifier(struct
> notifier_block *nb,
> >
> > switch (action & ~CPU_TASKS_FROZEN) {
> > case CPU_UP_PREPARE:
> > - intel_cqm_cpu_prepare(cpu);
> > + return intel_cqm_cpu_prepare(cpu);
> > break;
> > case CPU_DOWN_PREPARE:
> > intel_cqm_cpu_exit(cpu);
> > @@ -1305,17 +1786,74 @@ static int intel_cqm_cpu_notifier(struct
> notifier_block *nb,
> > static const struct x86_cpu_id intel_cqm_match[] = {
> > { .vendor = X86_VENDOR_INTEL, .feature =
> X86_FEATURE_CQM_OCCUP_LLC },
> > {}
> > + }, intel_mbm_match[] = {
>
> What's wrong with having a separate
>
> static const struct x86_cpu_id intel_mbm_match[] = {
>
> > + { .vendor = X86_VENDOR_INTEL, .feature =
> X86_FEATURE_CQM_MBM_LOCAL },
> > + {}
> > };
>
> That again would make the change obvious, but that's not on your agenda.
>
> >
> > static int __init intel_cqm_init(void)
> > {
> > char *str, scale[20];
> > - int i, cpu, ret;
> > + int i = 0, cpu, ret;
>
> Initializing i is required because?
>
> > - if (!x86_match_cpu(intel_cqm_match))
> > + if (!x86_match_cpu(intel_cqm_match) &&
> > + (!x86_match_cpu(intel_mbm_match)))
> > return -ENODEV;
> >
> > cqm_l3_scale = boot_cpu_data.x86_cache_occ_scale;
> > + cqm_max_rmid = boot_cpu_data.x86_cache_max_rmid;
> > +
> > + if (x86_match_cpu(intel_cqm_match)) {
> > + cqm_llc_occ = true;
> > + intel_cqm_events_group.attrs = intel_cmt_events_attr;
> > + } else
> > + cqm_llc_occ = false;
>
> I see you do not trust the compiler to put cqm_llc_occ into the BSS
> section and neither the kernel to zero it out proper.
>
> > +
> > + if (x86_match_cpu(intel_mbm_match)) {
> > + u32 mbm_scale_rounded = 0;
> > +
> > + cqm_mbm = true;
> > + cqm_l3_scale = boot_cpu_data.x86_cache_occ_scale;
> > + /*
> > + * MBM counter values are in bytes. To conver this to MB/sec,
> > + * we scale the MBM scale factor by 1000. Another 1000 factor
> > + * scaling down is done
> > + * after reading the counter val i.e. in the function
> > + * __rmid_read_mbm()
>
> So the event attribute string is in KB/sec and at some other place you
> convert it to MB/sec? There seems to be some hidden logic for this,
> but that's definitely not decodeable for me.
>
> > + */
> > + mbm_scale_rounded = (cqm_l3_scale + 500) / 1000;
>
> div_round_up
>
> > + cqm_max_rmid = boot_cpu_data.x86_cache_max_rmid;
> > + snprintf(scale, sizeof(scale), "%u", mbm_scale_rounded);
> > + str = kstrdup(scale, GFP_KERNEL);
> > + if (!str) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > +
> > + if (cqm_llc_occ)
> > + intel_cqm_events_group.attrs =
> > + intel_cmt_mbm_events_attr;
>
> And this line break is required because?
>
> > + else
> > + intel_cqm_events_group.attrs =
> intel_mbm_events_attr;
> > +
> > + event_attr_intel_cqm_llc_local_bw_scale.event_str
> > + = event_attr_intel_cqm_llc_total_bw_scale.event_str = str;
>
> You really love to write unreadable crap. This is neither perl nor the
> obfuscated c-code contest. What's wrong with two separate lines which
> assign 'str' to each of the event attributes?
>
> > + mbm_local = kzalloc_node(sizeof(struct sample) *
> > + (cqm_max_rmid + 1) * MBM_SOCKET_MAX,
>
> Calculating the size in a separate line with a proper variable would
> make it readable.
>
> > + GFP_KERNEL, NUMA_NO_NODE);
> > + if (!mbm_local) {
>
> Leaks str
>
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > + mbm_total = kzalloc_node(sizeof(struct sample) *
> > + (cqm_max_rmid + 1) * MBM_SOCKET_MAX,
> > + GFP_KERNEL, NUMA_NO_NODE);
> > + if (!mbm_total) {
> > + ret = -ENOMEM;
>
> Leaks str and mbm_local
>
> > + goto out;
> > + }
>
> What about sticking that mess into a separate function?
>
> > + } else
> > + cqm_mbm = false;
>
> Sigh.
>
> > /*
> > * It's possible that not all resources support the same number
> > @@ -1328,44 +1866,48 @@ static int __init intel_cqm_init(void)
> > */
> > cpu_notifier_register_begin();
> >
> > - for_each_online_cpu(cpu) {
> > - struct cpuinfo_x86 *c = &cpu_data(cpu);
> > + if (cqm_llc_occ) {
> > + for_each_online_cpu(cpu) {
> > + struct cpuinfo_x86 *c = &cpu_data(cpu);
> >
> > - if (c->x86_cache_max_rmid < cqm_max_rmid)
> > - cqm_max_rmid = c->x86_cache_max_rmid;
> > + if (c->x86_cache_max_rmid < cqm_max_rmid)
> > + cqm_max_rmid = c->x86_cache_max_rmid;
> >
> > - if (c->x86_cache_occ_scale != cqm_l3_scale) {
> > - pr_err("Multiple LLC scale values, disabling\n");
> > - ret = -EINVAL;
> > - goto out;
> > + if (c->x86_cache_occ_scale != cqm_l3_scale) {
> > + pr_err("Multiple LLC scale values, disabling\n");
> > + ret = -EINVAL;
>
> More memory leaks
>
> > + goto out;
> > + }
> > }
> > - }
> >
> > - /*
> > - * A reasonable upper limit on the max threshold is the number
> > - * of lines tagged per RMID if all RMIDs have the same number of
> > - * lines tagged in the LLC.
> > - *
> > - * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC.
> > - */
> > - __intel_cqm_max_threshold =
> > + /*
> > + * A reasonable upper limit on the max threshold is the number
> > + * of lines tagged per RMID if all RMIDs have the same number
> of
> > + * lines tagged in the LLC.
> > + *
> > + * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC.
> > + */
> > + __intel_cqm_max_threshold =
> > boot_cpu_data.x86_cache_size * 1024 / (cqm_max_rmid + 1);
> >
> > - snprintf(scale, sizeof(scale), "%u", cqm_l3_scale);
> > - str = kstrdup(scale, GFP_KERNEL);
> > - if (!str) {
> > - ret = -ENOMEM;
> > - goto out;
> > - }
> > + snprintf(scale, sizeof(scale), "%u", cqm_l3_scale);
> > + str = kstrdup(scale, GFP_KERNEL);
> > + if (!str) {
> > + ret = -ENOMEM;
>
> Ditto.
>
> > + goto out;
> > + }
> >
> > - event_attr_intel_cqm_llc_scale.event_str = str;
> > + event_attr_intel_cqm_llc_scale.event_str = str;
> > + }
> >
> > ret = intel_cqm_setup_rmid_cache();
> > if (ret)
> > goto out;
> >
> > for_each_online_cpu(i) {
> > - intel_cqm_cpu_prepare(i);
> > + ret = intel_cqm_cpu_prepare(i);
> > + if (ret)
> > + goto out;
>
> And that leaves even more half initialized stuff around.
>
> > cqm_pick_event_reader(i);
> > }
>
> I fear your assessment in
>
>
> http://lkml.kernel.org/r/06033C969873E840BDE9FC9B584F66B58BB89D@ORS
> MSX116.amr.corp.intel.com
>
> is slightly wrong.
>
> That's a completely convoluted mess. Aside of that its broken all over
> the place.
>
> This wants to be split up into preparatory patches which remodel the
> existing code and move out cqm stuff into helper functions, so the mbm
> stuff can be plugged in with its own helpers nicely.
>
> Thanks,
>
> tglx
--
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/
Powered by blists - more mailing lists