From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linutronix.de (146.0.238.70:993) by crypto-ml.lab.linutronix.de with IMAP4-SSL for ; 11 Feb 2019 10:40:50 -0000 Received: from bombadil.infradead.org ([2607:7c80:54:e::133]) by Galois.linutronix.de with esmtps (TLS1.2:RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1gt917-00026N-3z for speck@linutronix.de; Mon, 11 Feb 2019 11:40:49 +0100 Received: from j217100.upc-j.chello.nl ([24.132.217.100] helo=hirez.programming.kicks-ass.net) by bombadil.infradead.org with esmtpsa (Exim 4.90_1 #2 (Red Hat Linux)) id 1gt914-0002H0-Dd for speck@linutronix.de; Mon, 11 Feb 2019 10:40:46 +0000 Date: Mon, 11 Feb 2019 11:40:44 +0100 From: Peter Zijlstra Subject: [MODERATED] Re: [RFC][PATCH] performance walnuts Message-ID: <20190211104044.GW32511@hirez.programming.kicks-ass.net> References: <3dd5d6e2bc9ac53f826c251c68ce84fcc79a6872.1549582769.git.ak@linux.intel.com> <20190208090147.GK32477@hirez.programming.kicks-ass.net> <20190208093950.GD32534@hirez.programming.kicks-ass.net> <20190208105318.GE32534@hirez.programming.kicks-ass.net> <20190208180753.GC16922@tassilo.jf.intel.com> MIME-Version: 1.0 In-Reply-To: <20190208180753.GC16922@tassilo.jf.intel.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: On Fri, Feb 08, 2019 at 10:07:53AM -0800, speck for Andi Kleen wrote: > On Fri, Feb 08, 2019 at 11:53:18AM +0100, speck for Peter Zijlstra wrote: > > * From here on, the constraint is dynamic. > > @@ -3345,6 +3387,26 @@ glp_get_event_constraints(struct cpu_hw_events *cpuc, int idx, > > return c; > > } > > > > +static bool allow_tsx_force_abort = true; > > The default needs more discussion. This is what Thomas wants and makes sense to me. I'm done talking about this. > > + > > +static struct event_constraint * > > +skl_get_event_constraints(struct cpu_hw_events *cpuc, int idx, > > + struct perf_event *event) > > +{ > > + struct event_constraint *c = hsw_get_event_constraints(cpuc, idx, event); > > + > > + /* > > + * Without TFA we must not use PMC3. > > + */ > > + if (!allow_tsx_force_abort && test_bit(3, c->idxmsk)) { > > This still needs the extra changes in my patchkit to allow user/kvm opt-in/out. It needs no such thing. Userspace has the one knob. And I want to hear from Thomas and Paolo on what KVM wants; Andrew already said he doesn't want this crap exposed to virt. And the less I have to worry about virt the happier I am. I've yet to see compelling arguments for making this more complicated. Minimal and correct are the name of the game here; we need to backport this because that fscking ucode default screws us over. If you want complicated; you can try arguing that after we've gone public. > > @@ -4061,9 +4126,12 @@ static struct attribute *intel_pmu_caps_attrs[] = { > > NULL > > }; > > > > +DEVICE_BOOL_ATTR(allow_tsx_force_abort, 0644, allow_tsx_force_abort); > > > I still think "enable_all_counters" is a far better name. This is more explicit; the 'feature' is tsx_force_abort and will be called so in /proc/cpuid. > > static struct attribute *intel_pmu_attrs[] = { > > &dev_attr_freeze_on_smi.attr, > > NULL, > > This needs a comment, as done in my patch. Sure can do. > > + NULL, > > }; > > > > static __init struct attribute ** > > @@ -4546,6 +4614,7 @@ __init int intel_pmu_init(void) > > x86_pmu.flags |= PMU_FL_HAS_RSP_1; > > x86_pmu.flags |= PMU_FL_NO_HT_SHARING; > > > > + > > Unnecessary change. Already gone. > > x86_pmu.hw_config = hsw_hw_config; > > x86_pmu.get_event_constraints = hsw_get_event_constraints; > > extra_attr = boot_cpu_has(X86_FEATURE_RTM) ? > > @@ -4557,6 +4626,16 @@ __init int intel_pmu_init(void) > > tsx_attr = hsw_tsx_events_attrs; > > intel_pmu_pebs_data_source_skl( > > boot_cpu_data.x86_model == INTEL_FAM6_SKYLAKE_X); > > + > > + /* If our CPU haz a walnut */ > > + if (boot_cpu_has(X86_FEATURE_TSX_FORCE_ABORT)) { > > + x86_pmu.flags |= PMU_FL_WALNUT; > > I don't think the Walnut name will be publicly documented, so it will > be just confusing. Better to give it an descriptive name. > TSX_COUNTER3 or something like this. Then WTH expose us to it in the first place?