Extend mwait idle to optimize away CAL and RES interrupts to an idle CPU -v1

* Do we need some accounting for these wakeups exported for powertop?

* We can also eliminate TS_POLLING flag in favor of this. But, that will have a lot more touchpoints and better done as a standlone change.

+DECLARE_PER_CPU(atomic_t *, idle_task_ti_flags);

Quoted 2 lines · Venkatesh Pallipadi

* We can also eliminate TS_POLLING flag in favor of this. But, that will have a lot more touchpoints and better done as a standlone change.

Should most definitely be done for this series to be acceptble - as a preparatory patch in the series, with the feature at the end of the series.

Quoted 5 lines · Ingo Molnar

Quoted 2 lines · Venkatesh Pallipadi

* We can also eliminate TS_POLLING flag in favor of this. But, that will have a lot more touchpoints and better done as a standlone change.

Quoted 3 lines

Should most definitely be done for this series to be acceptble - as a preparatory patch in the series, with the feature at the end of the series.

I don't think you can, TS_POLLING still works for idle=poll, whereas this new stuff requires monitor/mwait.

Quoted 1 line · Venkatesh Pallipadi

* Do we need some accounting for these wakeups exported for powertop?

If then tracepoints.

Quoted 2 lines

* We can also eliminate TS_POLLING flag in favor of this. But, that will have a lot more touchpoints and better done as a standlone change.

Should most definitely be done for this series to be acceptble - as a preparatory patch in the series, with the feature at the end of the series.

Quoted 1 line

+DECLARE_PER_CPU(atomic_t *, idle_task_ti_flags);

That's ugly, we should access the idle task's ti flags directly.

To have efficient percpu access to the idle threads another clean-up is needed: we should turn idle_thread_array into a full-structure PER_CPU area.

For that we need a small variant of fork_idle(), which does not dup the init thread - pretty trivial.

fork_idle() should also make sure it does not schedule the child
thread: thus we'd also be able to further simplify smpboot.c and get rid of all that extremely ugly 'struct create_idle' gymnastics in smpboot.c.

Thanks,

       Ingo

Quoted 1 line · Venkatesh Pallipadi

* Do we need some accounting for these wakeups exported for powertop?

If then tracepoints.

Quoted 2 lines

* We can also eliminate TS_POLLING flag in favor of this. But, that will have a lot more touchpoints and better done as a standlone change.

Should most definitely be done for this series to be acceptble - as a preparatory patch in the series, with the feature at the end of the series.

Quoted 1 line

+DECLARE_PER_CPU(atomic_t *, idle_task_ti_flags);

That's ugly, we should access the idle task's ti flags directly.

To have efficient percpu access to the idle threads another clean-up is needed: we should turn idle_thread_array into a full-structure PER_CPU area.

For that we need a small variant of fork_idle(), which does not dup the init thread - pretty trivial.

fork_idle() should also make sure it does not schedule the child
thread: thus we'd also be able to further simplify smpboot.c and get rid of all that extremely ugly 'struct create_idle' gymnastics in smpboot.c.

Thanks,

       Ingo

Quoted 5 lines

fork_idle() should also make sure it does not schedule the child
thread: thus we'd also be able to further simplify smpboot.c and get rid of all that extremely ugly 'struct create_idle' gymnastics in smpboot.c.

But not this. I am not sure where fork_idle results in resched of the child. As I saw it, fork_idle calls init_idle and that sets the affinity of idle_task to target CPU. So, reschedule should not be a problem. What am I missing here?

Also, I tried this silly test patch (Cut and paste... Sorry) and it seemed to work fine both with and without CPU hotplug.

Thanks,
Venki

---

arch/x86/kernel/smpboot.c +6 -6

index 66d250c..36b80ef 100644
@@ -686,7 +686,7 @@ static int __cpuinit do_boot_cpu(int apicid, int cpu)
                .done   = COMPLETION_INITIALIZER_ONSTACK(c_idle.done),
        };
 
-       INIT_WORK_ONSTACK(&c_idle.work, do_fork_idle);
+       // INIT_WORK_ONSTACK(&c_idle.work, do_fork_idle);
 
        alternatives_smp_switch(1);
 

@@ -703,12 +703,13 @@ static int __cpuinit do_boot_cpu(int apicid, int cpu)
                goto do_rest;
        }
 
-       schedule_work(&c_idle.work);
-       wait_for_completion(&c_idle.done);
+       // schedule_work(&c_idle.work);
+       // wait_for_completion(&c_idle.done);
+       c_idle.idle = fork_idle(cpu);
 
        if (IS_ERR(c_idle.idle)) {
                printk("failed fork for CPU %d\n", cpu);
-               destroy_work_on_stack(&c_idle.work);
+               // destroy_work_on_stack(&c_idle.work);
                return PTR_ERR(c_idle.idle);
        }
 

@@ -831,7 +832,7 @@ do_rest:
                smpboot_restore_warm_reset_vector();
        }
 
-       destroy_work_on_stack(&c_idle.work);
+       // destroy_work_on_stack(&c_idle.work);
        return boot_error;
 }
 
---

Quoted 5 lines

Thanks,

Ingo

Quoted 10 lines · Venki Pallipadi

Quoted 5 lines

fork_idle() should also make sure it does not schedule the child
thread: thus we'd also be able to further simplify smpboot.c and get rid of all that extremely ugly 'struct create_idle' gymnastics in smpboot.c.

But not this. I am not sure where fork_idle results in resched of the child. As I saw it, fork_idle calls init_idle and that sets the affinity of idle_task to target CPU. So, reschedule should not be a problem. What am I missing here?

I think Ingo is referring to the fact that we can't use kthread_create() here and hence we were relying on fork_idle().

Quoted 3 lines

Also, I tried this silly test patch (Cut and paste... Sorry) and it seemed to work fine both with and without CPU hotplug.

I don't think we can do this today, as we need to make sure we have the correct current context. With dynamic cpu hotplug, current context can be any process and hence we were depending on the schedule_work() context.

thanks,
suresh

Quoted 49 lines

Thanks,
Venki

---

arch/x86/kernel/smpboot.c +6 -6

index 66d250c..36b80ef 100644
@@ -686,7 +686,7 @@ static int __cpuinit do_boot_cpu(int apicid, int cpu)
                 .done   = COMPLETION_INITIALIZER_ONSTACK(c_idle.done),
         };
 
-       INIT_WORK_ONSTACK(&c_idle.work, do_fork_idle);
+       // INIT_WORK_ONSTACK(&c_idle.work, do_fork_idle);
 
         alternatives_smp_switch(1);
 

@@ -703,12 +703,13 @@ static int __cpuinit do_boot_cpu(int apicid, int cpu)
                 goto do_rest;
         }
 
-       schedule_work(&c_idle.work);
-       wait_for_completion(&c_idle.done);
+       // schedule_work(&c_idle.work);
+       // wait_for_completion(&c_idle.done);
+       c_idle.idle = fork_idle(cpu);
 
         if (IS_ERR(c_idle.idle)) {
                 printk("failed fork for CPU %d\n", cpu);
-               destroy_work_on_stack(&c_idle.work);
+               // destroy_work_on_stack(&c_idle.work);
                 return PTR_ERR(c_idle.idle);
         }
 

@@ -831,7 +832,7 @@ do_rest:
                 smpboot_restore_warm_reset_vector();
         }
 
-       destroy_work_on_stack(&c_idle.work);
+       // destroy_work_on_stack(&c_idle.work);
         return boot_error;
  }
 
---

Quoted 5 lines

Thanks,

       Ingo

Quoted 15 lines · Suresh Siddha

Quoted 10 lines · Venki Pallipadi

Quoted 5 lines

fork_idle() should also make sure it does not schedule the child
thread: thus we'd also be able to further simplify smpboot.c and get rid of all that extremely ugly 'struct create_idle' gymnastics in smpboot.c.

But not this. I am not sure where fork_idle results in resched of the child. As I saw it, fork_idle calls init_idle and that sets the affinity of idle_task to target CPU. So, reschedule should not be a problem. What am I missing here?

I think Ingo is referring to the fact that we can't use kthread_create() here and hence we were relying on fork_idle().

Quoted 9 lines

Quoted 3 lines

Also, I tried this silly test patch (Cut and paste... Sorry) and it seemed to work fine both with and without CPU hotplug.

I don't think we can do this today, as we need to make sure we have the correct current context. With dynamic cpu hotplug, current context can be any process and hence we were depending on the schedule_work() context.

schedule_work() is only done at boot time. In case of dynamic cpu hotplug, we skip the whole fork_idle as we already have the task struct and just do init_idle().

Quoted 26 lines · Venki Pallipadi

Quoted 24 lines · Suresh Siddha

Quoted 10 lines · Venki Pallipadi

Quoted 5 lines

fork_idle() should also make sure it does not schedule the child
thread: thus we'd also be able to further simplify smpboot.c and get rid of all that extremely ugly 'struct create_idle' gymnastics in smpboot.c.

But not this. I am not sure where fork_idle results in resched of the child. As I saw it, fork_idle calls init_idle and that sets the affinity of idle_task to target CPU. So, reschedule should not be a problem. What am I missing here?

I think Ingo is referring to the fact that we can't use kthread_create() here and hence we were relying on fork_idle().

Quoted 3 lines

Also, I tried this silly test patch (Cut and paste... Sorry) and it seemed to work fine both with and without CPU hotplug.

I don't think we can do this today, as we need to make sure we have the correct current context. With dynamic cpu hotplug, current context can be any process and hence we were depending on the schedule_work() context.

Quoted 4 lines

schedule_work() is only done at boot time. In case of dynamic cpu hotplug, we skip the whole fork_idle as we already have the task struct and just do init_idle().

What happens if we boot with "maxcpus=" and later online the remaining cpu's?

same issue with the physical cpu-online case too right?

thanks,
suresh

Quoted 31 lines · Suresh Siddha

Quoted 29 lines · Venki Pallipadi

Quoted 15 lines · Suresh Siddha

Quoted 10 lines · Venki Pallipadi

Quoted 5 lines

fork_idle() should also make sure it does not schedule the child
thread: thus we'd also be able to further simplify smpboot.c and get rid of all that extremely ugly 'struct create_idle' gymnastics in smpboot.c.

But not this. I am not sure where fork_idle results in resched of the child. As I saw it, fork_idle calls init_idle and that sets the affinity of idle_task to target CPU. So, reschedule should not be a problem. What am I missing here?

I think Ingo is referring to the fact that we can't use kthread_create() here and hence we were relying on fork_idle().

Quoted 8 lines

Quoted 3 lines

Also, I tried this silly test patch (Cut and paste... Sorry) and it seemed to work fine both with and without CPU hotplug.

I don't think we can do this today, as we need to make sure we have the correct current context. With dynamic cpu hotplug, current context can be any process and hence we were depending on the schedule_work() context.

schedule_work() is only done at boot time. In case of dynamic cpu hotplug, we skip the whole fork_idle as we already have the task struct and just do init_idle().

Quoted 2 lines

What happens if we boot with "maxcpus=" and later online the remaining cpu's?

Yes. This case will be problematic. So, we still need struct create_idle work stuff even after percpu idle_task. Or was Ingo's suggestion to do something along the lines of - init any CPU's idle task from CPU 0's idle task?

Quoted 4 lines · Venki Pallipadi

Yes. This case will be problematic. So, we still need struct create_idle work stuff even after percpu idle_task. Or was Ingo's suggestion to do something along the lines of - init any CPU's idle task from CPU 0's idle task?

If the percpu area of possible CPUs is already allocated at this point then we should probably do that.

If not then what is the problem with doing fork_idle() from the hotplug process context? Where does the assymetry in the dynamic case come from?

Cleanup. Instead of reimplementing fork_idle on wq and idle task caching, use fork_idle_from_wq().

Signed-off-by Venkatesh Pallipadi <[email protected]>

Cleanup. Instead of reimplementing fork_idle on wq and idle task caching, use fork_idle_from_wq().

Signed-off-by Venkatesh Pallipadi <[email protected]>

---
 arch/s390/kernel/smp.c |   38 +++++---------------------------------
 1 files changed, 5 insertions(+), 33 deletions(-)

I looked at avoiding the wq stuff. But, there is no easy/clean way to do if as we have number of routines within copy_process assume current being the parent.

But, we can move most of the complication out of arch code into generic code and share it across multiple archs. Here is the patch for that.

We can easily add x86 percpu idle task on top of this cleanup (except for mini-circus to switch idle_task of CPU 0).

Sorry. The patches for ia64, mips, powerpc and s390 are untested and are in "should work" category.

Overall diffstat looks like

---
 arch/ia64/kernel/smpboot.c |   49 ++---------------------------
 arch/mips/kernel/smp.c     |   47 +---------------------------
 arch/powerpc/kernel/smp.c  |   63 ++++----------------------------------
 arch/s390/kernel/smp.c     |   38 +++--------------------
 arch/x86/kernel/smpboot.c  |   74 ++++++---------------------------------------
 include/linux/sched.h      |    1 
 kernel/fork.c              |   48 +++++++++++++++++++++++++++++
 7 files changed, 79 insertions(+), 241 deletions(-)

Changes since previous versions:

Moved the changes into arch specific code as per PeterZ suggestion

 include/linux/sched.h               |    2 +
 kernel/sched/core.c                 |   13 ++++++

arch/x86/include/asm/ipiless_poke.h new file +35 -0

new file mode 100644
index 0000000..58670c7
@@ -0,0 +1,82 @@
+#ifndef _ASM_X86_IPILESS_POKE_H
+#define _ASM_X86_IPILESS_POKE_H
+
+#include <linux/sched.h>
+#include <asm/thread_info.h>
+
+#ifdef CONFIG_SMP
+
+DECLARE_PER_CPU(atomic_t *, idle_task_ti_flags);
+
+/*
+ * Use 2 bits in idle_task's thead info flags:
+ * TIF_IPILESS_IDLE marks enter to and exit from idle states with ipiless
+ * wakeup capability.
+ * TIF_IPI_PENDING set by IPI source CPU if it finds that the IPI target CPU
+ * is in TIF_IPILESS_IDLE state (and TIF_IPI_PENDING is not already set).
+ * Setting of TIF_IPI_PENDING bit brings the target CPU out of idle state.
+ */
+
+static inline void ipiless_idle_enter(void)
+{
+	set_thread_flag(TIF_IPILESS_IDLE);
+}
+
+static inline void ipiless_idle_exit(void)
+{
+	clear_thread_flag(TIF_IPILESS_IDLE);
+}
+
+static inline int is_ipi_pending(void)
+{
+	return unlikely(test_thread_flag(TIF_IPI_PENDING));
+}
+
+static inline int need_wakeup(void)

arch/x86/include/asm/ipiless_poke.h new file +17 -0

+		return 1;
+
+	return 0;
+}
+
+#else
+static inline void ipiless_pending_work(void) { }
+static inline void ipiless_idle_enter(void) { }
+static inline void ipiless_idle_exit(void) { }
+
+static inline int need_wakeup(void)
+{
+	return need_resched();
+}
+#endif
+
+#endif /* _ASM_X86_IPILESS_POKE_H */

arch/x86/kernel/smp.c +1 -0

index a8ff227..e66a4c8 100644
@@ -27,6 +27,7 @@
 #include <asm/mtrr.h>
 #include <asm/tlbflush.h>
 #include <asm/mmu_context.h>
+#include <asm/ipiless_poke.h>
 #include <asm/proto.h>
 #include <asm/apic.h>
 #include <asm/nmi.h>

kernel/sched/core.c +0 -0

index 5255c9d..1558316 100644

kernel/sched/core.c +4 -0

 void scheduler_ipi(void)
 {
 	if (llist_empty(&this_rq()->wake_list) && !got_nohz_idle_kick())

@@ -4827,6 +4835,10 @@ void __cpuinit init_idle_bootup_task(struct task_struct *idle)
 	idle->sched_class = &idle_sched_class;
 }
 
+void __cpuinit __weak thread_idle_state_setup(struct task_struct *idle, int cpu)
+{
+}
+
 /**
  * init_idle - set up an idle thread for a given CPU
  * @idle: task in question

Quoted 3 lines · Venkatesh Pallipadi

Changes since previous versions:

  Moved the changes into arch specific code as per PeterZ suggestion

You failed:

Quoted 56 lines

 include/linux/sched.h               |    2 + kernel/sched/core.c                 |   13 ++++++

arch/x86/include/asm/ipiless_poke.h new file +13 -0

new file mode 100644
index 0000000..58670c7
@@ -0,0 +1,82 @@
+#ifndef _ASM_X86_IPILESS_POKE_H
+#define _ASM_X86_IPILESS_POKE_H
+
+#include <linux/sched.h>
+#include <asm/thread_info.h>
+
+#ifdef CONFIG_SMP
+
+DECLARE_PER_CPU(atomic_t *, idle_task_ti_flags);
+
+/*
+ * Use 2 bits in idle_task's thead info flags:
+ * TIF_IPILESS_IDLE marks enter to and exit from idle states with

ipiless
+ * wakeup capability.
+ * TIF_IPI_PENDING set by IPI source CPU if it finds that the IPI target CPU
+ * is in TIF_IPILESS_IDLE state (and TIF_IPI_PENDING is not already set).
+ * Setting of TIF_IPI_PENDING bit brings the target CPU out of idle state.
+ */

arch/x86/include/asm/ipiless_poke.h new file +22 -0

+
+static inline void ipiless_idle_enter(void)
+{
+     set_thread_flag(TIF_IPILESS_IDLE);
+}
+
+static inline void ipiless_idle_exit(void)
+{
+     clear_thread_flag(TIF_IPILESS_IDLE);
+}
+
+static inline int is_ipi_pending(void)
+{
+     return unlikely(test_thread_flag(TIF_IPI_PENDING));
+}
+
+static inline int need_wakeup(void)
+{
+     return need_resched() || is_ipi_pending();
+}
+
+static inline void ipiless_pending_work(void)

Quoted 5 lines

+{
+     if (is_ipi_pending()) {
+             clear_thread_flag(TIF_IPI_PENDING);
+             local_bh_disable();
+             local_irq_disable();

That local_bh_disable() is completely superfluous, disabling IRQs already kills bh.

Quoted 3 lines

+             generic_smp_call_function_single_interrupt();
+             __scheduler_ipi();

Why not scheduler_ipi()?

Also, you could keep a pending vector bitmask in a per-cpu variable to avoid having to call all handlers. not sure that's worth it for just two, but something to keep in mind for if/when this starts expanding.

Quoted 4 lines

+             local_irq_enable();
+             local_bh_enable();
+     }
+}

Why doesn't ipiless_idle_exit() call this? That would keep it isolated to just those idle routines that actually use mwait instead of bothering the generic idle paths with this.

arch/x86/include/asm/ipiless_poke.h line 56

Quoted 5 lines

arch/x86/include/asm/ipiless_poke.h new file +4 -0

+static inline int ipiless_magic_poke(int cpu)
+{
+     int val;
+     atomic_t *idle_flag = per_cpu(idle_task_ti_flags, cpu);

What's this atomic_t nonsense about? thread_info::flags is __u32, casting it to atomic_t is complete rubbish.

Quoted 2 lines

+
+     val = atomic_read(idle_flag);

The __u32 version would look like: val = ACCESS_ONCE(*idle_flag);

arch/x86/include/asm/ipiless_poke.h line 65

Quoted 7 lines

+     if (unlikely(val & _TIF_IPI_PENDING))
+             return 1;

arch/x86/include/asm/ipiless_poke.h new file +5 -0

+
+     if (!(val & _TIF_IPILESS_IDLE))
+             return 0;
+
+     if (val == atomic_cmpxchg(idle_flag, val, val | _TIF_IPI_PENDING))

The __u32 version would look like:

 if (val == cmpxchg(idle_flag, val, val | _TIF_IPI_PENDING))

Bonus win for being shorter!

Quoted 35 lines

+             return 1;

arch/x86/include/asm/ipiless_poke.h new file +16 -0

+
+     return 0;
+}
+
+#else
+static inline void ipiless_pending_work(void) { }
+static inline void ipiless_idle_enter(void) { }
+static inline void ipiless_idle_exit(void) { }
+
+static inline int need_wakeup(void)
+{
+     return need_resched();
+}
+#endif
+
+#endif /* _ASM_X86_IPILESS_POKE_H */

arch/x86/kernel/smp.c +1 -0

index a8ff227..e66a4c8 100644
@@ -27,6 +27,7 @@
  #include <asm/mtrr.h>
  #include <asm/tlbflush.h>
  #include <asm/mmu_context.h>
+#include <asm/ipiless_poke.h>
  #include <asm/proto.h>
  #include <asm/apic.h>
  #include <asm/nmi.h>

@@ -109,6 +110,14 @@
   *   about nothing of note with C stepping upwards.
   */
 

arch/x86/kernel/smp.c line 119

Quoted 9 lines

arch/x86/kernel/smp.c +7 -0

+DEFINE_PER_CPU(atomic_t *, idle_task_ti_flags);
+
+void __cpuinit thread_idle_state_setup(struct task_struct *idle, int
 cpu)
+{
+     per_cpu(idle_task_ti_flags, cpu) =
+                             (atomic_t
 *)(&(task_thread_info(idle)->flags));
+}

As Ingo already pointed out, its the architecture that actually sets up the idle threads, so putting callbacks into the generic code to find them is kinda backwards.

Again, *yuck* at that atomic_t business.

Quoted 7 lines

kernel/sched/core.c +0 -0

index 5255c9d..1558316 100644

kernel/sched/core.c static void sched_ttwu_pending(void) · line 1460

Quoted 10 lines

kernel/sched/core.c +7 -0

@@ -1451,6 +1451,14 @@ static void sched_ttwu_pending(void)
       raw_spin_unlock(&rq->lock);
  }
 
+void __scheduler_ipi(void)
+{
+     if (llist_empty(&this_rq()->wake_list))
+             return;
+
+     sched_ttwu_pending();
+}

FAIL!! It should be 100% identical to a normal IPI, that calls scheduler_ipi() so this should too.

Quoted 19 lines

kernel/sched/core.c +4 -0

  void scheduler_ipi(void)
  {
       if (llist_empty(&this_rq()->wake_list) && !got_nohz_idle_kick())

@@ -4827,6 +4835,10 @@ void __cpuinit init_idle_bootup_task(struct
 task_struct *idle)
       idle->sched_class = &idle_sched_class;
  }
 
+void __cpuinit __weak thread_idle_state_setup(struct task_struct *idle,
 int cpu)
+{
+}
+
  /**

  * init_idle - set up an idle thread for a given CPU
  * @idle: task in question

kernel/sched/core.c +0 -0

@@ -4869,6 +4881,7 @@ void __cpuinit init_idle(struct task_struct *idle,
 int cpu)
 

kernel/sched/core.c void __cpuinit init_idle(struct task_struct *idle, int cpu) · line 4884

Quoted 3 lines

kernel/sched/core.c +1 -0

       /* Set the preempt count _outside_ the spinlocks! */
       task_thread_info(idle)->preempt_count = 0;
+     thread_idle_state_setup(idle, cpu);

I suggest you put this in smpboot.c someplace ;-)

Quoted 1 line · Peter Zijlstra

Why not scheduler_ipi()?

Was trying to avoid irq_enter/exit. As the work here is done in idle thread context, I though we could avoid enter/exit.

Quoted 5 lines · Venki Pallipadi

Quoted 1 line · Peter Zijlstra

Why not scheduler_ipi()?

Was trying to avoid irq_enter/exit. As the work here is done in idle thread context, I though we could avoid enter/exit.

It seems we could not.
At least RCU need it, see commit c5d753a55, otherwise we will get warning like 'RCU used illegally from extended quiescent state!'

This remind me that we should have moved the irq_enter()/irq_exit() to each arch's related irq handler.
see: http://marc.info/?l=linux-kernel&m=130709505700821&w=2

So Peter, is there someone alread on it? or it still worth doing now?

Quoted 5 lines · Yong Zhang

This remind me that we should have moved the irq_enter()/irq_exit() to each arch's related irq handler.
see: http://marc.info/?l=linux-kernel&m=130709505700821&w=2

So Peter, is there someone alread on it? or it still worth doing now?

trouble was that some people didn't feel comfortable adding that overhead to plain resched IPIs that didn't need to do anything.

So I
more or less let it sit for a while.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 6 lines · Peter Zijlstra

Quoted 4 lines · Yong Zhang

This remind me that we should have moved the irq_enter()/irq_exit() to each arch's related irq handler.
see: http://marc.info/?l=linux-kernel&m=130709505700821&w=2

Quoted 4 lines

Quoted 1 line

So Peter, is there someone alread on it? or it still worth doing now?

trouble was that some people didn't feel comfortable adding that overhead to plain resched IPIs that didn't need to do anything.

Any ideas on where the plain resched IPIs come from?

Quoted 7 lines · Yong Zhang

Quoted 6 lines · Peter Zijlstra

Quoted 4 lines · Yong Zhang

This remind me that we should have moved the irq_enter()/irq_exit() to each arch's related irq handler.
see: http://marc.info/?l=linux-kernel&m=130709505700821&w=2

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

So Peter, is there someone alread on it? or it still worth doing now?

trouble was that some people didn't feel comfortable adding that overhead to plain resched IPIs that didn't need to do anything.

Any ideas on where the plain resched IPIs come from?

remote wakeups that don't queue I guess.

IIRC I looked at it a while back and while not in the majority there were still a few (I only added counters, didn't look where they came from).

However since 518cd623 there'd be a lot more I guess..

Feel free to investigate. Also, some non-sched users of the resched ipi exist, eg. KVM uses it to kick a remote vcpu out from guest mode.

Quoted 5 lines · Venki Pallipadi

Quoted 1 line · Peter Zijlstra

Why not scheduler_ipi()?

Was trying to avoid irq_enter/exit. As the work here is done in idle thread context, I though we could avoid enter/exit.

It seems we could not.
At least RCU need it, see commit c5d753a55, otherwise we will get warning like 'RCU used illegally from extended quiescent state!'

Quoted 10 lines · Yong Zhang

Quoted 5 lines · Venki Pallipadi

Quoted 1 line · Peter Zijlstra

Why not scheduler_ipi()?

Was trying to avoid irq_enter/exit. As the work here is done in idle thread context, I though we could avoid enter/exit.

It seems we could not.
At least RCU need it, see commit c5d753a55, otherwise we will get warning like 'RCU used illegally from extended quiescent state!'

If the use is tracing, then Steven Rostedt's patchset plus use of his _rcuidle() tracing variants handles this:

	https://lkml.org/lkml/2012/2/7/231

If this is instead algorithmic use of RCU, a set of patches I have queued up for 3.4 will be required.

Quoted 15 lines · Paul E. McKenney

Quoted 10 lines · Yong Zhang

Quoted 5 lines · Venki Pallipadi

Quoted 1 line · Peter Zijlstra

Why not scheduler_ipi()?

Was trying to avoid irq_enter/exit. As the work here is done in idle thread context, I though we could avoid enter/exit.

It seems we could not.
At least RCU need it, see commit c5d753a55, otherwise we will get warning like 'RCU used illegally from extended quiescent state!'

If the use is tracing, then Steven Rostedt's patchset plus use of his _rcuidle() tracing variants handles this:

Quoted 4 lines

	https://lkml.org/lkml/2012/2/7/231

If this is instead algorithmic use of RCU, a set of patches I have queued up for 3.4 will be required.

scheduler_ipi() doing more than tracing. Will look at your patches :)

Quoted 18 lines · Yong Zhang

Quoted 17 lines · Paul E. McKenney

Quoted 10 lines · Yong Zhang

Quoted 5 lines · Venki Pallipadi

Quoted 1 line · Peter Zijlstra

Why not scheduler_ipi()?

Was trying to avoid irq_enter/exit. As the work here is done in idle thread context, I though we could avoid enter/exit.

It seems we could not.
At least RCU need it, see commit c5d753a55, otherwise we will get warning like 'RCU used illegally from extended quiescent state!'

If the use is tracing, then Steven Rostedt's patchset plus use of his _rcuidle() tracing variants handles this:

	https://lkml.org/lkml/2012/2/7/231

Quoted 4 lines

Quoted 2 lines

If this is instead algorithmic use of RCU, a set of patches I have queued up for 3.4 will be required.

scheduler_ipi() doing more than tracing. Will look at your patches :)

Ah! The key question is whether or not the code in question is called both from idle and from non-idle.

This will be easiest if the code is
called only from idle, in which case you should only need this one:

	https://lkml.org/lkml/2012/2/3/498

Quoted 22 lines · Paul E. McKenney

Quoted 21 lines · Yong Zhang

Quoted 19 lines · Paul E. McKenney

Quoted 10 lines · Yong Zhang

Quoted 5 lines · Venki Pallipadi

Quoted 1 line · Peter Zijlstra

Why not scheduler_ipi()?

Was trying to avoid irq_enter/exit. As the work here is done in idle thread context, I though we could avoid enter/exit.

It seems we could not.
At least RCU need it, see commit c5d753a55, otherwise we will get warning like 'RCU used illegally from extended quiescent state!'

If the use is tracing, then Steven Rostedt's patchset plus use of his _rcuidle() tracing variants handles this:

	https://lkml.org/lkml/2012/2/7/231

If this is instead algorithmic use of RCU, a set of patches I have queued up for 3.4 will be required.

Quoted 4 lines

Quoted 1 line

scheduler_ipi() doing more than tracing. Will look at your patches :)

Ah! The key question is whether or not the code in question is called both from idle and from non-idle.

In fact before this patch from Venki, the only call site of scheduler_ipi() is resched irq handler. Then Venki introduce __scheduler_ipi()(which avoid irq_enter()/irq_exit()) into cpu_idle(). So the answer is yes.

But when I was testing this patch, I didn't see explicit warning on illegal rcu usage. The reason maybe 1) there are no much rcu dereference in scheduler_ipi(), but we indeed do tracing in it; 2) rq->lock provide some kind of protection.
Maybe I'm overstraining, but it is potential danger.

But anyway, it's not an issue anymore since Venki removed __scheduler_ipi() in his latest version.

Quoted 4 lines

This will be easiest if the code is
called only from idle, in which case you should only need this one:

	https://lkml.org/lkml/2012/2/3/498

Hmm... Yeah, RCU_NONIDLE() could survive IMHO :)

Quoted 23 lines · Yong Zhang

Quoted 22 lines · Paul E. McKenney

Quoted 21 lines · Yong Zhang

Quoted 19 lines · Paul E. McKenney

Quoted 10 lines · Yong Zhang

Quoted 5 lines · Venki Pallipadi

Quoted 1 line · Peter Zijlstra

Why not scheduler_ipi()?

Was trying to avoid irq_enter/exit. As the work here is done in idle thread context, I though we could avoid enter/exit.

It seems we could not.
At least RCU need it, see commit c5d753a55, otherwise we will get warning like 'RCU used illegally from extended quiescent state!'

If the use is tracing, then Steven Rostedt's patchset plus use of his _rcuidle() tracing variants handles this:

	https://lkml.org/lkml/2012/2/7/231

If this is instead algorithmic use of RCU, a set of patches I have queued up for 3.4 will be required.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

scheduler_ipi() doing more than tracing. Will look at your patches :)

Ah! The key question is whether or not the code in question is called both from idle and from non-idle.

In fact before this patch from Venki, the only call site of scheduler_ipi() is resched irq handler. Then Venki introduce __scheduler_ipi()(which avoid irq_enter()/irq_exit()) into cpu_idle(). So the answer is yes.

Ah, that explains why I didn't see it in my testing. ;-)

Quoted 5 lines

But when I was testing this patch, I didn't see explicit warning on illegal rcu usage. The reason maybe 1) there are no much rcu dereference in scheduler_ipi(), but we indeed do tracing in it; 2) rq->lock provide some kind of protection.
Maybe I'm overstraining, but it is potential danger.

Did you have CONFIG_PROVE_RCU=y when testing?

zgrep PROVE /proc/config.gz

Quoted 3 lines

But anyway, it's not an issue anymore since Venki removed __scheduler_ipi() in his latest version.

OK.

Quoted 6 lines

Quoted 4 lines

This will be easiest if the code is
called only from idle, in which case you should only need this one:

	https://lkml.org/lkml/2012/2/3/498

Hmm... Yeah, RCU_NONIDLE() could survive IMHO :)

Seems like it might be needed sooner rather than later...

Quoted 5 lines

arch/x86/include/asm/ipiless_poke.h line 45

Quoted 2 lines

arch/x86/include/asm/ipiless_poke.h new file +2 -0

+             local_bh_disable();
+             local_irq_disable();

That local_bh_disable() is completely superfluous, disabling IRQs already kills bh.

The reason for local_bh_disable/enable was to check for potential softirqs after these interrupt.

Quoted 3 lines

Why doesn't ipiless_idle_exit() call this? That would keep it isolated to just those idle routines that actually use mwait instead of bothering the generic idle paths with this.

ipiless_idle_exit is called in the inner most part of idle entry exit. In mwait case we may not even have interrupts enabled at that time and there is code that does idle residency timing etc which will get impacted if we start doing the pending ipi work there. Doing it at higher level along with things like enter-exit tickless felt nicer.

Quoted 3 lines · Venki Pallipadi

Quoted 3 lines

Quoted 2 lines

+             local_bh_disable();
+             local_irq_disable();

Quoted 5 lines

Quoted 2 lines

That local_bh_disable() is completely superfluous, disabling IRQs already kills bh.

The reason for local_bh_disable/enable was to check for potential softirqs after these interrupt.

Why is that needed? We never wrap local_irq_disable() in bh muck?

Quoted 4 lines

Quoted 3 lines

Why doesn't ipiless_idle_exit() call this? That would keep it isolated to just those idle routines that actually use mwait instead of bothering the generic idle paths with this.

Quoted 5 lines

ipiless_idle_exit is called in the inner most part of idle entry exit. In mwait case we may not even have interrupts enabled at that time and there is code that does idle residency timing etc which will get impacted if we start doing the pending ipi work there. Doing it at higher level along with things like enter-exit tickless felt nicer.

But but but, an actual interrupt can be handled before the instruction after mwait, so why would this be a problem?