| Message ID | 20131211124352.GB21999@twins.programming.kicks-ass.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, 2013-12-11 at 13:43 +0100, Peter Zijlstra wrote: > On Wed, Dec 11, 2013 at 01:29:15PM +0100, Mike Galbraith wrote: > > On Wed, 2013-12-11 at 12:52 +0100, Peter Zijlstra wrote: > > > On Wed, Dec 11, 2013 at 12:38:39PM +0100, Borislav Petkov wrote: > > > > Right, if it turns out that this is really the case and that this > > > > erratum hasn't been fixed for models later than 29 - we'd need the > > > > additional model numbers to set X86_FEATURE_CLFLUSH_MONITOR correctly. > > > > > > You also need: https://lkml.org/lkml/2013/11/19/143 > > > > > > Because obviously not all mwait idle loops check that cpu bit. > > > > I had tried that patch, to see if it would magically make the thing > > start working, nope. I had also tried... > > > + if (this_cpu_has(X86_FEATURE_CLFLUSH_MONITOR)) > > + clflush((void *)¤t_thread_info()->flags); > > Yeah, you need a bit extra to enable that feature bit for your CPU as > bpetkov said. Works for me, one more for the stable bucket. So as soon as Len resurrects mwait_idle for Q6600 (and other core2 when booted max_cstates=1 so tsc clocksource is used instead of pos hpet), all the (known) idle regressions should be history. -Mike -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 11, 2013 at 01:43:52PM +0100, Peter Zijlstra wrote: > Something like the below.. someone needs to double check and possibly > add SNB/IVB EX parts if they're already available. Right, our friends at Intel would need to tell us which families/models does AAI65 span... if, this is actually the case. But now that Mike's box is fixed too, it very much looks like it. Oh, and AAIxx errata are Intel Xeon Processor 7400 Series ones, we would also need to know whether other incarnations are affected too.
* Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Dec 11, 2013 at 01:29:15PM +0100, Mike Galbraith wrote: > > On Wed, 2013-12-11 at 12:52 +0100, Peter Zijlstra wrote: > > > On Wed, Dec 11, 2013 at 12:38:39PM +0100, Borislav Petkov wrote: > > > > Right, if it turns out that this is really the case and that this > > > > erratum hasn't been fixed for models later than 29 - we'd need the > > > > additional model numbers to set X86_FEATURE_CLFLUSH_MONITOR correctly. > > > > > > You also need: https://lkml.org/lkml/2013/11/19/143 > > > > > > Because obviously not all mwait idle loops check that cpu bit. > > > > I had tried that patch, to see if it would magically make the thing > > start working, nope. I had also tried... > > > + if (this_cpu_has(X86_FEATURE_CLFLUSH_MONITOR)) > > + clflush((void *)¤t_thread_info()->flags); > > Yeah, you need a bit extra to enable that feature bit for your CPU as > bpetkov said. > > Something like the below.. someone needs to double check and possibly > add SNB/IVB EX parts if they're already available. > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > index dc1ec0dff939..015642eed045 100644 > --- a/arch/x86/kernel/cpu/intel.c > +++ b/arch/x86/kernel/cpu/intel.c > @@ -387,8 +387,15 @@ static void init_intel(struct cpuinfo_x86 *c) > set_cpu_cap(c, X86_FEATURE_PEBS); > } > > - if (c->x86 == 6 && c->x86_model == 29 && cpu_has_clflush) > - set_cpu_cap(c, X86_FEATURE_CLFLUSH_MONITOR); > + if (c->x86 == 6 && cpu_has_clflush) { > + switch (c->x86_model) { > + case 29: /* Core2 EX */ > + case 46: /* NHM EX */ > + case 47: /* WSM EX */ > + set_cpu_cap(c, X86_FEATURE_CLFLUSH_MONITOR); > + break; > + } > + } Another thing that is required I think is to issue a write barrier before CLFLUSH instruction. By my (possibly incorrect ...) reading of the documentation CLFLUSH does not appear to be ordered (at all), so it might execute before the modification to the affected memory? So something like: if (this_cpu_has(X86_FEATURE_CLFLUSH_MONITOR)) { smp_wmb(); /* order CLFLUSH */ clflush(¤t_thread_info()->flags); } possibly put behind some utility function, smp_clflush() or so, hiding the CPU feature bit check as well: smp_clflush(¤t_thread_info()->flags); or so? Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Borislav Petkov <bp@alien8.de> wrote: > On Wed, Dec 11, 2013 at 01:43:52PM +0100, Peter Zijlstra wrote: > > Something like the below.. someone needs to double check and possibly > > add SNB/IVB EX parts if they're already available. > > Right, our friends at Intel would need to tell us which families/models > does AAI65 span... if, this is actually the case. I think CLFLUSH should be pretty universally available, IIRC graphics drivers were using it rather heavily in combination with write-combining MTRRs, both on Linux and on Windows. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 11 Dec 2013, Ingo Molnar wrote: > > Another thing that is required I think is to issue a write barrier > before CLFLUSH instruction. By my (possibly incorrect ...) reading of > the documentation CLFLUSH does not appear to be ordered (at all), so > it might execute before the modification to the affected memory? We already have a write barrier in the code which modifies current_thread_info()->flags. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Thomas Gleixner <tglx@linutronix.de> wrote: > On Wed, 11 Dec 2013, Ingo Molnar wrote: > > > > Another thing that is required I think is to issue a write barrier > > before CLFLUSH instruction. By my (possibly incorrect ...) reading of > > the documentation CLFLUSH does not appear to be ordered (at all), so > > it might execute before the modification to the affected memory? > > We already have a write barrier in the code which modifies > current_thread_info()->flags. Which code is that, could you please cite it? (I tried finding it but my grep-fu failed me.) Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 11, 2013 at 03:56:55PM +0100, Ingo Molnar wrote: > I think CLFLUSH should be pretty universally available, IIRC > graphics drivers were using it rather heavily in combination with > write-combining MTRRs, both on Linux and on Windows. ... and it is also very expensive. So I don't think it would be in Intel's best interest to do CLFLUSH unconditionally on all families but only on those which really need to.
On Wed, Dec 11, 2013 at 03:56:55PM +0100, Ingo Molnar wrote: > > * Borislav Petkov <bp@alien8.de> wrote: > > > On Wed, Dec 11, 2013 at 01:43:52PM +0100, Peter Zijlstra wrote: > > > Something like the below.. someone needs to double check and possibly > > > add SNB/IVB EX parts if they're already available. > > > > Right, our friends at Intel would need to tell us which families/models > > does AAI65 span... if, this is actually the case. > > I think CLFLUSH should be pretty universally available, IIRC graphics > drivers were using it rather heavily in combination with > write-combining MTRRs, both on Linux and on Windows. The availability isn't the problem; the cost is. We shouldn't issue one if its not required. Only 'broken' EX hardware needs it. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 11, 2013 at 03:42:38PM +0100, Ingo Molnar wrote: > Another thing that is required I think is to issue a write barrier > before CLFLUSH instruction. By my (possibly incorrect ...) reading of > the documentation CLFLUSH does not appear to be ordered (at all), so > it might execute before the modification to the affected memory? > > > So something like: > > if (this_cpu_has(X86_FEATURE_CLFLUSH_MONITOR)) { > smp_wmb(); /* order CLFLUSH */ > clflush(¤t_thread_info()->flags); > } smp_wmb() is a NO-OP on x86 remember :-) Also, a wmb doesn't actually need to flush the store buffers. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 11, 2013 at 04:09:11PM +0100, Ingo Molnar wrote: > > * Thomas Gleixner <tglx@linutronix.de> wrote: > > > On Wed, 11 Dec 2013, Ingo Molnar wrote: > > > > > > Another thing that is required I think is to issue a write barrier > > > before CLFLUSH instruction. By my (possibly incorrect ...) reading of > > > the documentation CLFLUSH does not appear to be ordered (at all), so > > > it might execute before the modification to the affected memory? > > > > We already have a write barrier in the code which modifies > > current_thread_info()->flags. > > Which code is that, could you please cite it? (I tried finding it but > my grep-fu failed me.) current_set_polling_and_test(). -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Dec 11, 2013 at 03:42:38PM +0100, Ingo Molnar wrote: > > Another thing that is required I think is to issue a write barrier > > before CLFLUSH instruction. By my (possibly incorrect ...) reading of > > the documentation CLFLUSH does not appear to be ordered (at all), so > > it might execute before the modification to the affected memory? > > > > > > So something like: > > > > if (this_cpu_has(X86_FEATURE_CLFLUSH_MONITOR)) { > > smp_wmb(); /* order CLFLUSH */ > > clflush(¤t_thread_info()->flags); > > } > > smp_wmb() is a NO-OP on x86 remember :-) Well, it's a compiler barrier but yes - I suspect a smp_mb() might be needed - at least according to the CLFLUSH documentation it has no implicit guaranteed ordering wrt. preceding writes. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Dec 11, 2013 at 04:09:11PM +0100, Ingo Molnar wrote: > > > > * Thomas Gleixner <tglx@linutronix.de> wrote: > > > > > On Wed, 11 Dec 2013, Ingo Molnar wrote: > > > > > > > > Another thing that is required I think is to issue a write barrier > > > > before CLFLUSH instruction. By my (possibly incorrect ...) reading of > > > > the documentation CLFLUSH does not appear to be ordered (at all), so > > > > it might execute before the modification to the affected memory? > > > > > > We already have a write barrier in the code which modifies > > > current_thread_info()->flags. > > > > Which code is that, could you please cite it? (I tried finding it but > > my grep-fu failed me.) > > current_set_polling_and_test(). Thx, those variants have the right barrier indeed. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Dec 11, 2013 at 03:56:55PM +0100, Ingo Molnar wrote: > > > > * Borislav Petkov <bp@alien8.de> wrote: > > > > > On Wed, Dec 11, 2013 at 01:43:52PM +0100, Peter Zijlstra wrote: > > > > Something like the below.. someone needs to double check and possibly > > > > add SNB/IVB EX parts if they're already available. > > > > > > Right, our friends at Intel would need to tell us which > > > families/models does AAI65 span... if, this is actually the > > > case. > > > > I think CLFLUSH should be pretty universally available, IIRC > > graphics drivers were using it rather heavily in combination with > > write-combining MTRRs, both on Linux and on Windows. > > The availability isn't the problem; the cost is. We shouldn't issue > one if its not required. Only 'broken' EX hardware needs it. Well, availability could be a problem too, if some CPU (real or virtual) implements MWAIT but not CLFLUSH. In theory we could make mwait an alternatives variant and patch in the right combination of instructions? The CLFLUSH goes to the same address as on which the monitoring happens, so it could be considered one meta-instruction. Thansk, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/11/2013 09:50 AM, Ingo Molnar wrote: > > Well, availability could be a problem too, if some CPU (real or > virtual) implements MWAIT but not CLFLUSH. > > In theory we could make mwait an alternatives variant and patch in the > right combination of instructions? The CLFLUSH goes to the same > address as on which the monitoring happens, so it could be considered > one meta-instruction. > The first thing to do is probably to drop the use of thread_info as a wakeup doorbell. It seemed like a good idea at the time -- after all, there is one for each thread -- but it is extremely likely to be dirty in the cache, which is (presumably) what causes these kinds of bugs to be maximally likely. Even if we don't do the CLFLUSH it is likely that the hardware has to do something expensive behind the scenes. So I would like to propose that we switch to using a percpu variable which is a single cache line of nothing at all. It would only ever be touched by MONITOR and for explicit wakeup. Hopefully that will resolve this problem without the need for the CLFLUSH. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 11, 2013 at 03:08:35PM -0800, H. Peter Anvin wrote: > So I would like to propose that we switch to using a percpu variable > which is a single cache line of nothing at all. It would only ever > be touched by MONITOR and for explicit wakeup. Hopefully that will > resolve this problem without the need for the CLFLUSH. Yep, makes a lot of sense to me to have an exclusive (overloaded meaning here :-)) cacheline only for that. And, if it works, we'll save us the penalty from the CLFLUSH too, cool.
On Wed, Dec 11, 2013 at 03:08:35PM -0800, H. Peter Anvin wrote: > On 12/11/2013 09:50 AM, Ingo Molnar wrote: > > > > Well, availability could be a problem too, if some CPU (real or > > virtual) implements MWAIT but not CLFLUSH. > > > > In theory we could make mwait an alternatives variant and patch in the > > right combination of instructions? The CLFLUSH goes to the same > > address as on which the monitoring happens, so it could be considered > > one meta-instruction. > > > > The first thing to do is probably to drop the use of thread_info as a > wakeup doorbell. It seemed like a good idea at the time -- after all, > there is one for each thread -- but it is extremely likely to be dirty > in the cache, which is (presumably) what causes these kinds of bugs to > be maximally likely. Even if we don't do the CLFLUSH it is likely that > the hardware has to do something expensive behind the scenes. > > So I would like to propose that we switch to using a percpu variable > which is a single cache line of nothing at all. It would only ever be > touched by MONITOR and for explicit wakeup. Hopefully that will resolve > this problem without the need for the CLFLUSH. The reason we use thread_info::flags is because we need to write TIF_NEED_RESCHED into it to wake up anyhow. Using another cacheline would mean the wakeup path would need to write a second cross cpu cacheline -- that is badness too. So no, I don't think we want to listen to another line. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Peter Zijlstra <peterz@infradead.org> wrote: > On Wed, Dec 11, 2013 at 03:08:35PM -0800, H. Peter Anvin wrote: > > On 12/11/2013 09:50 AM, Ingo Molnar wrote: > > > > > > Well, availability could be a problem too, if some CPU (real or > > > virtual) implements MWAIT but not CLFLUSH. > > > > > > In theory we could make mwait an alternatives variant and patch in the > > > right combination of instructions? The CLFLUSH goes to the same > > > address as on which the monitoring happens, so it could be considered > > > one meta-instruction. > > > > > > > The first thing to do is probably to drop the use of thread_info as a > > wakeup doorbell. It seemed like a good idea at the time -- after all, > > there is one for each thread -- but it is extremely likely to be dirty > > in the cache, which is (presumably) what causes these kinds of bugs to > > be maximally likely. Even if we don't do the CLFLUSH it is likely that > > the hardware has to do something expensive behind the scenes. > > > > So I would like to propose that we switch to using a percpu variable > > which is a single cache line of nothing at all. It would only ever be > > touched by MONITOR and for explicit wakeup. Hopefully that will resolve > > this problem without the need for the CLFLUSH. > > The reason we use thread_info::flags is because we need to write > TIF_NEED_RESCHED into it to wake up anyhow. > > Using another cacheline would mean the wakeup path would need to write a > second cross cpu cacheline -- that is badness too. > > So no, I don't think we want to listen to another line. Seconded ... Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/12/2013 05:28 AM, Ingo Molnar wrote: >> >> The reason we use thread_info::flags is because we need to write >> TIF_NEED_RESCHED into it to wake up anyhow. >> >> Using another cacheline would mean the wakeup path would need to write a >> second cross cpu cacheline -- that is badness too. >> >> So no, I don't think we want to listen to another line. > Right, okay, so that's the implicit wakeup. However, I would think the CLFLUSH would hurt a lot more. -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 12, 2013 at 07:06:57AM -0800, H. Peter Anvin wrote: > On 12/12/2013 05:28 AM, Ingo Molnar wrote: > >> > >> The reason we use thread_info::flags is because we need to write > >> TIF_NEED_RESCHED into it to wake up anyhow. > >> > >> Using another cacheline would mean the wakeup path would need to write a > >> second cross cpu cacheline -- that is badness too. > >> > >> So no, I don't think we want to listen to another line. > > > > Right, okay, so that's the implicit wakeup. However, I would think the > CLFLUSH would hurt a lot more. Maybe, but still, who cares? Its only a few broken cpus that actually need the clflush, normal cpus do not. We should not optimize for the broken case. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c index dc1ec0dff939..015642eed045 100644 --- a/arch/x86/kernel/cpu/intel.c +++ b/arch/x86/kernel/cpu/intel.c @@ -387,8 +387,15 @@ static void init_intel(struct cpuinfo_x86 *c) set_cpu_cap(c, X86_FEATURE_PEBS); } - if (c->x86 == 6 && c->x86_model == 29 && cpu_has_clflush) - set_cpu_cap(c, X86_FEATURE_CLFLUSH_MONITOR); + if (c->x86 == 6 && cpu_has_clflush) { + switch (c->x86_model) { + case 29: /* Core2 EX */ + case 46: /* NHM EX */ + case 47: /* WSM EX */ + set_cpu_cap(c, X86_FEATURE_CLFLUSH_MONITOR); + break; + } + } #ifdef CONFIG_X86_64 if (c->x86 == 15)