Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1272261 > unrolled thread

Re: i915.ko WC writes are slow after ea8596bb2d8d379

Started byChris Wilson <chris@chris-wilson.co.uk>
First post2015-11-18 16:00 +0100
Last post2015-11-19 09:20 +0100
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: i915.ko WC writes are slow after ea8596bb2d8d379 Chris Wilson <chris@chris-wilson.co.uk> - 2015-11-18 16:00 +0100
    Re: i915.ko WC writes are slow after ea8596bb2d8d379 Andy Lutomirski <luto@amacapital.net> - 2015-11-18 17:00 +0100
      Re: i915.ko WC writes are slow after ea8596bb2d8d379 Ingo Molnar <mingo@kernel.org> - 2015-11-19 09:20 +0100
        [PATCH] kernel: Remove stop_machine() Kconfig dependency Chris Wilson <chris@chris-wilson.co.uk> - 2015-11-19 11:10 +0100
    Re: i915.ko WC writes are slow after ea8596bb2d8d379 Ingo Molnar <mingo@kernel.org> - 2015-11-19 09:20 +0100

#1272261 — Re: i915.ko WC writes are slow after ea8596bb2d8d379

FromChris Wilson <chris@chris-wilson.co.uk>
Date2015-11-18 16:00 +0100
SubjectRe: i915.ko WC writes are slow after ea8596bb2d8d379
Message-ID<qwcng-4gQ-15@gated-at.bofh.it>
On Wed, Oct 08, 2014 at 05:10:59AM -0500, Chuck Ebbert wrote:
> On Wed, 8 Oct 2014 10:03:36 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > 
> > I ran into a problem on a Sandybridge i5-2500s whilst measuring the
> > performance of GTT write-combining access. I found subsequent runs were
> > about 10-40x slower than the first. For example,
> > 
> > igt/gem_gtt_speed:
> > 
> > Time to read 16k through a GTT map:             325.285µs
> > Time to write 16k through a GTT map:              4.729µs
> > Time to clear 16k through a GTT map:              4.584µs
> > Time to clear 16k through a cached GTT map:       1.342µs
> > 
> > on the second run became:
> > 
> > Time to read 16k through a GTT map:             332.148µs
> > Time to write 16k through a GTT map:            209.411µs
> > Time to clear 16k through a GTT map:             56.460µs
> > Time to clear 16k through a cached GTT map:      50.897µs
> > 
> > Naively I would say that we lost the wc on our ioremap.
> > /sys/kernel/debug/x86/pat_memtype_list remained the same across repeated
> > runs.
> > 
> > A bisection pointed to 
> > 
> > commit ea8596bb2d8d37957f3e92db9511c50801689180
> > Author: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > Date:   Thu Jul 18 20:47:53 2013 +0900
> > 
> >     kprobes/x86: Remove unused text_poke_smp() and text_poke_smp_batch() functions
> > 
> > of which the active ingredient was just
> > 
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index b32ebf9..f4001e0 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -2334,7 +2334,6 @@ config HAVE_ATOMIC_IOMAP
> >  
> >  config HAVE_TEXT_POKE_SMP
> >         bool
> > -       select STOP_MACHINE if SMP
> >  
> >  config X86_DEV_DMA_OPS
> >         bool
> > 
> > and adding that back into the current build, e.g.
> 
> Hmm, set_mtrr() uses stop_machine(). I wonder if your MTRRs are out of
> sync and your results depend on which CPU the test runs on?

(From the other reply, it did and is still required).

I have run into other issues where stop_machine() tries to only do a
irq-disabled callback on the local CPU as opposed to halting all CPUs
and running the callback universally.

My understanding is that the root cause of the issue is:

diff --git a/init/Kconfig b/init/Kconfig
index af09b4f..8235e0b 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1993,8 +1993,7 @@ config INIT_ALL_POSSIBLE
 
  config STOP_MACHINE
          bool
	  -       default y
	  -       depends on (SMP && MODULE_UNLOAD) || HOTPLUG_CPU
	  +       default y if SMP || HOTPLUG_CPU
	          help
		            Need stop_machine() primitive.

Although

diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
index d2abbdb..ff4f029 100644
--- a/include/linux/stop_machine.h
+++ b/include/linux/stop_machine.h
@@ -97,7 +97,7 @@ static inline int try_stop_cpus(const struct cpumask *cpumask,
  * grabbing every spinlock (and more).  So the "read" side to such a
  * lock is anything which disables preemption.
  */
-#if defined(CONFIG_STOP_MACHINE) && defined(CONFIG_SMP)
+#if defined(CONFIG_SMP) || defined(CONFIG_HOTPLUG_CPU)
 
 /**
  * stop_machine: freeze the machine on all CPUs and run this function
@@ -128,7 +128,7 @@ int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus);
 int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
                                   const struct cpumask *cpus);
 
-#else   /* CONFIG_STOP_MACHINE && CONFIG_SMP */
+#else   /* CONFIG_SMP */
 
 static inline int __stop_machine(int (*fn)(void *), void *data,
                                 const struct cpumask *cpus)
@@ -153,5 +153,5 @@ static inline int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
        return __stop_machine(fn, data, cpus);
 }
 
-#endif /* CONFIG_STOP_MACHINE && CONFIG_SMP */
+#endif /* CONFIG_SMP || CONFIG_HOTPLUG_CPU */
 #endif /* _LINUX_STOP_MACHINE */
diff --git a/init/Kconfig b/init/Kconfig
index af09b4f..44600a8 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1991,13 +1991,6 @@ config INIT_ALL_POSSIBLE
          it was better to provide this option than to break all the archs
          and have several arch maintainers pursuing me down dark alleys.
 
-config STOP_MACHINE
-       bool
-       default y
-       depends on (SMP && MODULE_UNLOAD) || HOTPLUG_CPU
-       help
-         Need stop_machine() primitive.
-
 source "block/Kconfig"
 
 config PREEMPT_NOTIFIERS
diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index fd643d8..2dd1f306 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -513,7 +513,7 @@ static int __init cpu_stop_init(void)
 }
 early_initcall(cpu_stop_init);
 
-#ifdef CONFIG_STOP_MACHINE
+#if defined(CONFIG_SMP) || defined(CONFIG_HOTPLUG_CPU)
 
 int __stop_machine(int (*fn)(void *), void *data, const struct cpumask *cpus)
 {
@@ -613,4 +613,4 @@ int stop_machine_from_inactive_cpu(int (*fn)(void *), void *data,
        return ret ?: done.ret;
 }
 
-#endif /* CONFIG_STOP_MACHINE */
+#endif /* CONFIG_SMP || CONFIG_HOTPLUG_CPU */

may be more apt.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1272322

FromAndy Lutomirski <luto@amacapital.net>
Date2015-11-18 17:00 +0100
Message-ID<qwdjl-4US-15@gated-at.bofh.it>
In reply to#1272261
On Wed, Nov 18, 2015 at 6:48 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Although
>
> diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
> index d2abbdb..ff4f029 100644
> --- a/include/linux/stop_machine.h
> +++ b/include/linux/stop_machine.h
> @@ -97,7 +97,7 @@ static inline int try_stop_cpus(const struct cpumask *cpumask,
>   * grabbing every spinlock (and more).  So the "read" side to such a
>   * lock is anything which disables preemption.
>   */
> -#if defined(CONFIG_STOP_MACHINE) && defined(CONFIG_SMP)
> +#if defined(CONFIG_SMP) || defined(CONFIG_HOTPLUG_CPU)

[...]


This seems much better.  Having a set of stop_machine functions around
that don't work depending on config seems dangerous.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272921

FromIngo Molnar <mingo@kernel.org>
Date2015-11-19 09:20 +0100
Message-ID<qwsBJ-6MQ-21@gated-at.bofh.it>
In reply to#1272322
* Andy Lutomirski <luto@amacapital.net> wrote:

> On Wed, Nov 18, 2015 at 6:48 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Although
> >
> > diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
> > index d2abbdb..ff4f029 100644
> > --- a/include/linux/stop_machine.h
> > +++ b/include/linux/stop_machine.h
> > @@ -97,7 +97,7 @@ static inline int try_stop_cpus(const struct cpumask *cpumask,
> >   * grabbing every spinlock (and more).  So the "read" side to such a
> >   * lock is anything which disables preemption.
> >   */
> > -#if defined(CONFIG_STOP_MACHINE) && defined(CONFIG_SMP)
> > +#if defined(CONFIG_SMP) || defined(CONFIG_HOTPLUG_CPU)
> 
> [...]
> 
> This seems much better.  Having a set of stop_machine functions around
> that don't work depending on config seems dangerous.

Agreed.

Acked-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272981 — [PATCH] kernel: Remove stop_machine() Kconfig dependency

FromChris Wilson <chris@chris-wilson.co.uk>
Date2015-11-19 11:10 +0100
Subject[PATCH] kernel: Remove stop_machine() Kconfig dependency
Message-ID<qwuk9-7U6-13@gated-at.bofh.it>
In reply to#1272921
Currently the full stop_machine() routine is only enabled on SMP if
module unloading is enabled, or if the CPUs are hotpluggable. This leads
to configurations where stop_machine() is broken as it will then only
run the callback on the local CPU with irqs disabled, and not stop the
other CPUs or run the callback on them. For example, this breaks MTRR
setup on x86 in certain configs since

commit ea8596bb2d8d37957f3e92db9511c50801689180
Author: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Date:   Thu Jul 18 20:47:53 2013 +0900

    kprobes/x86: Remove unused text_poke_smp() and text_poke_smp_batch() functions

as the MTRR is only established on the boot CPU.

This patch removes the Kconfig option for STOP_MACHINE and uses the SMP
and HOTPLUG_CPU config options to compile the correct stop_machine() for
the architecture, removing the false dependency on MODULE_UNLOAD in the
process.

Link: https://lkml.org/lkml/2014/10/8/124
References: https://bugs.freedesktop.org/show_bug.cgi?id=84794
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Preemptively-Acked-by: Ingo Molnar <mingo@kernel.org>
Cc:"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Pranith Kumar <bobby.prani@gmail.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Vladimir Davydov <vdavydov@parallels.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: H. Peter Anvin <hpa@linux.intel.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Iulia Manda <iulia.manda21@gmail.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Chuck Ebbert <cebbert.lkml@gmail.com>
---
 include/linux/stop_machine.h | 6 +++---
 init/Kconfig                 | 7 -------
 kernel/stop_machine.c        | 4 ++--
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
index 0adedca24c5b..0e1b1540597a 100644
--- a/include/linux/stop_machine.h
+++ b/include/linux/stop_machine.h
@@ -99,7 +99,7 @@ static inline int try_stop_cpus(const struct cpumask *cpumask,
  * grabbing every spinlock (and more).  So the "read" side to such a
  * lock is anything which disables preemption.
  */
-#if defined(CONFIG_STOP_MACHINE) && defined(CONFIG_SMP)
+#if defined(CONFIG_SMP) || defined(CONFIG_HOTPLUG_CPU)
 
 /**
  * stop_machine: freeze the machine on all CPUs and run this function
@@ -118,7 +118,7 @@ int stop_machine(cpu_stop_fn_t fn, void *data, const struct cpumask *cpus);
 
 int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 				   const struct cpumask *cpus);
-#else	 /* CONFIG_STOP_MACHINE && CONFIG_SMP */
+#else	/* CONFIG_SMP || CONFIG_HOTPLUG_CPU */
 
 static inline int stop_machine(cpu_stop_fn_t fn, void *data,
 				 const struct cpumask *cpus)
@@ -137,5 +137,5 @@ static inline int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 	return stop_machine(fn, data, cpus);
 }
 
-#endif	/* CONFIG_STOP_MACHINE && CONFIG_SMP */
+#endif	/* CONFIG_SMP || CONFIG_HOTPLUG_CPU */
 #endif	/* _LINUX_STOP_MACHINE */
diff --git a/init/Kconfig b/init/Kconfig
index c24b6f767bf0..235c7a2c0d20 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2030,13 +2030,6 @@ config INIT_ALL_POSSIBLE
 	  it was better to provide this option than to break all the archs
 	  and have several arch maintainers pursuing me down dark alleys.
 
-config STOP_MACHINE
-	bool
-	default y
-	depends on (SMP && MODULE_UNLOAD) || HOTPLUG_CPU
-	help
-	  Need stop_machine() primitive.
-
 source "block/Kconfig"
 
 config PREEMPT_NOTIFIERS
diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index 867bc20e1ef1..a3bbaee77c58 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -531,7 +531,7 @@ static int __init cpu_stop_init(void)
 }
 early_initcall(cpu_stop_init);
 
-#ifdef CONFIG_STOP_MACHINE
+#if defined(CONFIG_SMP) || defined(CONFIG_HOTPLUG_CPU)
 
 static int __stop_machine(cpu_stop_fn_t fn, void *data, const struct cpumask *cpus)
 {
@@ -631,4 +631,4 @@ int stop_machine_from_inactive_cpu(cpu_stop_fn_t fn, void *data,
 	return ret ?: done.ret;
 }
 
-#endif	/* CONFIG_STOP_MACHINE */
+#endif	/* CONFIG_SMP || CONFIG_HOTPLUG_CPU */
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272924

FromIngo Molnar <mingo@kernel.org>
Date2015-11-19 09:20 +0100
Message-ID<qwsBK-6MQ-29@gated-at.bofh.it>
In reply to#1272261
* Chris Wilson <chris@chris-wilson.co.uk> wrote:

> > > A bisection pointed to 
> > > 
> > > commit ea8596bb2d8d37957f3e92db9511c50801689180
> > > Author: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > > Date:   Thu Jul 18 20:47:53 2013 +0900
> > > 
> > >     kprobes/x86: Remove unused text_poke_smp() and text_poke_smp_batch() functions
> > > 
> > > of which the active ingredient was just
> > > 
> > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > index b32ebf9..f4001e0 100644
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -2334,7 +2334,6 @@ config HAVE_ATOMIC_IOMAP
> > >  
> > >  config HAVE_TEXT_POKE_SMP
> > >         bool
> > > -       select STOP_MACHINE if SMP

Ouch...

This is certainly an educative example of how pure 'code removal' patches can have 
unintended side effects.

Is there a full fix patch available, and is anyone pushing that to Linus?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web