Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
On 2024-06-19 at 00:03:30 +0530, K Prateek Nayak wrote:
> Hello Chenyu,
>
> On 6/18/2024 1:19 PM, Chen Yu wrote:
> > [..snip..]
> > > > > >
> > > > > > > Vincent [5] pointed out a case where the idle load kick will fail
> > > > > > > to
> > > > > > > run on an idle CPU since the IPI handler launching the ILB will
> > > > > > > check
> > > > > > > for need_resched(). In such cases, the idle CPU relies on
> > > > > > > newidle_balance() to pull tasks towards itself.
> > > > > >
> > > > > > Is this the need_resched() in _nohz_idle_balance() ? Should we
> > > > > > change
> > > > > > this to 'need_resched() && (rq->nr_running || rq->ttwu_pending)' or
> > > > > > something long those lines?
> > > > >
> > > > > It's not only this but also in do_idle() as well which exits the loop
> > > > > to look for tasks to schedule
> > > > >
> > > > > >
> > > > > > I mean, it's fairly trivial to figure out if there really is going
> > > > > > to be
> > > > > > work there.
> > > > > >
> > > > > > > Using an alternate flag instead of NEED_RESCHED to indicate a
> > > > > > > pending
> > > > > > > IPI was suggested as the correct approach to solve this problem
> > > > > > > on the
> > > > > > > same thread.
> > > > > >
> > > > > > So adding per-arch changes for this seems like something we
> > > > > > shouldn't
> > > > > > unless there really is no other sane options.
> > > > > >
> > > > > > That is, I really think we should start with something like the
> > > > > > below
> > > > > > and then fix any fallout from that.
> > > > >
> > > > > The main problem is that need_resched becomes somewhat meaningless
> > > > > because it doesn't only mean "I need to resched a task" and we have
> > > > > to add more tests around even for those not using polling
> > > > >
> > > > > >
> > > > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > > > > index 0935f9d4bb7b..cfa45338ae97 100644
> > > > > > --- a/kernel/sched/core.c
> > > > > > +++ b/kernel/sched/core.c
> > > > > > @@ -5799,7 +5800,7 @@ static inline struct task_struct *
> > > > > >__pick_next_task(struct rq *rq, struct task_struct *prev, struct
> > > > > > rq_flags *rf)
> > > > > >{
> > > > > > const struct sched_class *class;
> > > > > > - struct task_struct *p;
> > > > > > + struct task_struct *p = NULL;
> > > > > >
> > > > > > /*
> > > > > >* Optimization: we know that if all tasks are in the
> > > > > > fair class we can
> > > > > > @@ -5810,9 +5811,11 @@ __pick_next_task(struct rq *rq, struct
> > > > > > task_struct *prev, struct rq_flags *rf)
> > > > > > if (likely(!sched_class_above(prev->sched_class,
> > > > > > _sched_class) &&
> > > > > > rq->nr_running == rq->cfs.h_nr_running)) {
> > > > > >
> > > > > > - p = pick_next_task_fair(rq, prev, rf);
> > > > > > - if (unlikely(p == RETRY_TASK))
> > > > > > - goto restart;
> > > > > > + if (rq->nr_running) {
> > > > >
> > > > > How do you make the diff between a spurious need_resched() because of
> > > > > polling and a cpu becoming idle ? isn't rq->nr_running null in both
> > > > > cases ?
> > > > > In the later case, we need to call sched_balance_newidle() but not in
> > > > > the former
> > > > >
> > > >
> > > > Not sure if I understand correctly, if the goal of
> > > > smp_call_function_single() is to
> > > > kick the idle CPU and do not force it to launch the
> > > > schedule()->sched_balance_newidle(),
> > > > can we set the _TIF_POLLING_NRFLAG rather than _TIF_NEED_RESCHED in
> > > > set_nr_if_polling()?
> > > > I think writing any value to the monitor address would wakeup the idle
> > > > CPU. And _TIF_POLLING_NRFLAG
> > > > will be cleared once that idle CPU exit the idle loop, so we don't
> > > > introduce arch-wide flag.
> > > Although this might work for MWAIT, there is no way for the generic idle
> > > path to know if there is a pending interrupt within a TIF_POLLING_NRFLAG
> > > section. do_idle() sets TIF_POLLING_NRFLAG and relies on a bunch of
> > > need_resched() checks along the way to bail early until finally doing a
> > > current_clr_polling_and_test() before handing off to the cpuidle driver
> > > in call_cpuidle(). I believe this section will necessarily need the sender
> > > to indicate a pending interrupt via TIF_NEED_RESCHED flag to enable the
> > > early bail out before going into the cpuidle driver since this case cannot
> > > be considered the same as a break from MWAIT.
> > >
> >
> > I see, this is a good point. So you mean with only TIF_POLLING_NRFLAG there
> > is
> > possibility that the 'ipi kick CPU out of idle' is lost after the CPU enters
> > do_idle() and before finally entering the idle state. While setting
> > _TIF_NEED_RESCHED
> > could help the do_idle() loop to detect pending request easier.
>
> Yup, that is correct.
>
> > BTW, before the
> > commit b2a02fc43a1f ("smp: Optimize
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
Hello Chenyu,
On 6/18/2024 1:19 PM, Chen Yu wrote:
[..snip..]
Vincent [5] pointed out a case where the idle load kick will fail to
run on an idle CPU since the IPI handler launching the ILB will check
for need_resched(). In such cases, the idle CPU relies on
newidle_balance() to pull tasks towards itself.
Is this the need_resched() in _nohz_idle_balance() ? Should we change
this to 'need_resched() && (rq->nr_running || rq->ttwu_pending)' or
something long those lines?
It's not only this but also in do_idle() as well which exits the loop
to look for tasks to schedule
I mean, it's fairly trivial to figure out if there really is going to be
work there.
Using an alternate flag instead of NEED_RESCHED to indicate a pending
IPI was suggested as the correct approach to solve this problem on the
same thread.
So adding per-arch changes for this seems like something we shouldn't
unless there really is no other sane options.
That is, I really think we should start with something like the below
and then fix any fallout from that.
The main problem is that need_resched becomes somewhat meaningless
because it doesn't only mean "I need to resched a task" and we have
to add more tests around even for those not using polling
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 0935f9d4bb7b..cfa45338ae97 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5799,7 +5800,7 @@ static inline struct task_struct *
__pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags
*rf)
{
const struct sched_class *class;
- struct task_struct *p;
+ struct task_struct *p = NULL;
/*
* Optimization: we know that if all tasks are in the fair class we
can
@@ -5810,9 +5811,11 @@ __pick_next_task(struct rq *rq, struct task_struct
*prev, struct rq_flags *rf)
if (likely(!sched_class_above(prev->sched_class, _sched_class) &&
rq->nr_running == rq->cfs.h_nr_running)) {
- p = pick_next_task_fair(rq, prev, rf);
- if (unlikely(p == RETRY_TASK))
- goto restart;
+ if (rq->nr_running) {
How do you make the diff between a spurious need_resched() because of
polling and a cpu becoming idle ? isn't rq->nr_running null in both
cases ?
In the later case, we need to call sched_balance_newidle() but not in the former
Not sure if I understand correctly, if the goal of smp_call_function_single()
is to
kick the idle CPU and do not force it to launch the
schedule()->sched_balance_newidle(),
can we set the _TIF_POLLING_NRFLAG rather than _TIF_NEED_RESCHED in
set_nr_if_polling()?
I think writing any value to the monitor address would wakeup the idle CPU. And
_TIF_POLLING_NRFLAG
will be cleared once that idle CPU exit the idle loop, so we don't introduce
arch-wide flag.
Although this might work for MWAIT, there is no way for the generic idle
path to know if there is a pending interrupt within a TIF_POLLING_NRFLAG
section. do_idle() sets TIF_POLLING_NRFLAG and relies on a bunch of
need_resched() checks along the way to bail early until finally doing a
current_clr_polling_and_test() before handing off to the cpuidle driver
in call_cpuidle(). I believe this section will necessarily need the sender
to indicate a pending interrupt via TIF_NEED_RESCHED flag to enable the
early bail out before going into the cpuidle driver since this case cannot
be considered the same as a break from MWAIT.
I see, this is a good point. So you mean with only TIF_POLLING_NRFLAG there is
possibility that the 'ipi kick CPU out of idle' is lost after the CPU enters
do_idle() and before finally entering the idle state. While setting
_TIF_NEED_RESCHED
could help the do_idle() loop to detect pending request easier.
Yup, that is correct.
BTW, before the
commit b2a02fc43a1f ("smp: Optimize send_call_function_single_ipi()"), the
lost of ipi after entering do_idle() and before entering driver idle state
is also possible, right(the local irq is disabled)?
From what I understand, the IPI remains pending until the interrupts
are enabled again. Before the optimization, the interrupts would be
disabled all the way until the instruction that is used to put the CPU
to sleep which is what __sti_mwait() and native_safe_halt() does. The
CPU would have received the IPI then and broke out of idle before
Peter's optimization went in. There is an elaborate comment on this in
do_idle() function above the call to local_irq_disable(). In commit
edc8fc01f608 ("x86: Fix CPUIDLE_FLAG_IRQ_ENABLE leaking timer
reprogram") Peter describes a case of actually missing the break from
an interrupt as the driver enabled interrupts much earlier than
executing the sleep instruction.
Since the CPU was in TIF_POLLING_NRFLAG state, one could simply get away
by setting TIF_NEED_RESCHED and not sending an actual IPI which the
need_resched() checks in the idle path would catch and the
flush_smp_call_function_queue() on
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
On 2024-06-17 at 14:03:41 +0530, K Prateek Nayak wrote:
> Hello Chenyu,
>
> On 6/14/2024 10:01 PM, Chen Yu wrote:
> > On 2024-06-14 at 12:48:37 +0200, Vincent Guittot wrote:
> > > On Fri, 14 Jun 2024 at 11:28, Peter Zijlstra wrote:
> > > >
> > > > On Thu, Jun 13, 2024 at 06:15:59PM +, K Prateek Nayak wrote:
> > > > > Effects of call_function_single_prep_ipi()
> > > > > ==
> > > > >
> > > > > To pull a TIF_POLLING thread out of idle to process an IPI, the sender
> > > > > sets the TIF_NEED_RESCHED bit in the idle task's thread info in
> > > > > call_function_single_prep_ipi() and avoids sending an actual IPI to
> > > > > the
> > > > > target. As a result, the scheduler expects a task to be enqueued when
> > > > > exiting the idle path. This is not the case with non-polling idle
> > > > > states
> > > > > where the idle CPU exits the non-polling idle state to process the
> > > > > interrupt, and since need_resched() returns false, soon goes back to
> > > > > idle again.
> > > > >
> > > > > When TIF_NEED_RESCHED flag is set, do_idle() will call
> > > > > schedule_idle(),
> > > > > a large part of which runs with local IRQ disabled. In case of
> > > > > ipistorm,
> > > > > when measuring IPI throughput, this large IRQ disabled section delays
> > > > > processing of IPIs. Further auditing revealed that in absence of any
> > > > > runnable tasks, pick_next_task_fair(), which is called from the
> > > > > pick_next_task() fast path, will always call newidle_balance() in this
> > > > > scenario, further increasing the time spent in the IRQ disabled
> > > > > section.
> > > > >
> > > > > Following is the crude visualization of the problem with relevant
> > > > > functions expanded:
> > > > > --
> > > > > CPU0 CPU1
> > > > >
> > > > >do_idle() {
> > > > >
> > > > > __current_set_polling();
> > > > >...
> > > > >
> > > > > monitor(addr);
> > > > >if
> > > > > (!need_resched())
> > > > >
> > > > > mwait() {
> > > > >
> > > > > /* Waiting */
> > > > > smp_call_function_single(CPU1, func, wait = 1) {
> > > > > ...
> > > > >...
> > > > > ...
> > > > >set_nr_if_polling(CPU1) {
> > > > > ...
> > > > >/* Realizes CPU1 is polling */
> > > > > ...
> > > > >try_cmpxchg(addr,
> > > > > ...
> > > > >,
> > > > > ...
> > > > >val | _TIF_NEED_RESCHED);
> > > > > ...
> > > > >} /* Does not send an IPI */
> > > > > ...
> > > > >...
> > > > > } /* mwait exit due to write at addr */
> > > > >csd_lock_wait() { }
> > > > >/* Waiting */
> > > > > preempt_set_need_resched();
> > > > >...
> > > > > __current_clr_polling();
> > > > >...
> > > > > flush_smp_call_function_queue() {
> > > > >...
> > > > > func();
> > > > >} /* End of wait */ }
> > > > > }
> > > > > schedule_idle() {
> > > > >
> > > > > ...
> > > > >
> > > > > local_irq_disable();
> > > > > smp_call_function_single(CPU1, func, wait = 1) {
> > > > > ...
> > > > >...
> > > > > ...
> > > > >arch_send_call_function_single_ipi(CPU1);
> > > > > ...
> > > > >\
> > > > > ...
> > > > > \
> > > > > newidle_balance() {
> > > > > \
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
Hello Chenyu,
On 6/14/2024 10:01 PM, Chen Yu wrote:
On 2024-06-14 at 12:48:37 +0200, Vincent Guittot wrote:
On Fri, 14 Jun 2024 at 11:28, Peter Zijlstra wrote:
On Thu, Jun 13, 2024 at 06:15:59PM +, K Prateek Nayak wrote:
Effects of call_function_single_prep_ipi()
==
To pull a TIF_POLLING thread out of idle to process an IPI, the sender
sets the TIF_NEED_RESCHED bit in the idle task's thread info in
call_function_single_prep_ipi() and avoids sending an actual IPI to the
target. As a result, the scheduler expects a task to be enqueued when
exiting the idle path. This is not the case with non-polling idle states
where the idle CPU exits the non-polling idle state to process the
interrupt, and since need_resched() returns false, soon goes back to
idle again.
When TIF_NEED_RESCHED flag is set, do_idle() will call schedule_idle(),
a large part of which runs with local IRQ disabled. In case of ipistorm,
when measuring IPI throughput, this large IRQ disabled section delays
processing of IPIs. Further auditing revealed that in absence of any
runnable tasks, pick_next_task_fair(), which is called from the
pick_next_task() fast path, will always call newidle_balance() in this
scenario, further increasing the time spent in the IRQ disabled section.
Following is the crude visualization of the problem with relevant
functions expanded:
--
CPU0 CPU1
do_idle() {
__current_set_polling();
...
monitor(addr);
if
(!need_resched())
mwait() {
/*
Waiting */
smp_call_function_single(CPU1, func, wait = 1) {
...
...
...
set_nr_if_polling(CPU1) {
...
/* Realizes CPU1 is polling */
...
try_cmpxchg(addr,
...
,
...
val | _TIF_NEED_RESCHED);
...
} /* Does not send an IPI */
...
... } /*
mwait exit due to write at addr */
csd_lock_wait() { }
/* Waiting */
preempt_set_need_resched();
...
__current_clr_polling();
...
flush_smp_call_function_queue() {
... func();
} /* End of wait */ }
} schedule_idle() {
...
local_irq_disable();
smp_call_function_single(CPU1, func, wait = 1) { ...
... ...
arch_send_call_function_single_ipi(CPU1); ...
\ ...
\
newidle_balance() {
\
...
/* Delay */
...
\ }
\ ...
\-->
local_irq_enable();
/*
Processes the IPI */
--
Skipping newidle_balance()
==
In an earlier attempt to solve the challenge of the long IRQ disabled
section, newidle_balance() was skipped when a CPU waking up from idle
was found to have no runnable tasks, and was transitioning back to
idle [2]. Tim [3] and David [4] had pointed out that newidle_balance()
may be viable for CPUs that are idling with tick enabled, where the
newidle_balance() has the opportunity to pull tasks onto the idle CPU.
I don't think we should be relying on
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
Hello Vincent, Peter,
On 6/16/2024 8:27 PM, Vincent Guittot wrote:
On Sat, 15 Jun 2024 at 03:28, Peter Zijlstra wrote:
On Fri, Jun 14, 2024 at 12:48:37PM +0200, Vincent Guittot wrote:
On Fri, 14 Jun 2024 at 11:28, Peter Zijlstra wrote:
Vincent [5] pointed out a case where the idle load kick will fail to
run on an idle CPU since the IPI handler launching the ILB will check
for need_resched(). In such cases, the idle CPU relies on
newidle_balance() to pull tasks towards itself.
Is this the need_resched() in _nohz_idle_balance() ? Should we change
this to 'need_resched() && (rq->nr_running || rq->ttwu_pending)' or
something long those lines?
It's not only this but also in do_idle() as well which exits the loop
to look for tasks to schedule
Is that really a problem? Reading the initial email the problem seems to
be newidle balance, not hitting schedule. Schedule should be fairly
quick if there's nothing to do, no?
There are 2 problems:
- Because of NEED_RESCHED being set, we go through the full schedule
path for no reason and we finally do a sched_balance_newidle()
Peter's patch up in the thread seems to improve the above case by
speeding up the schedule() loop similar to the very first solution
I tried with
https://lore.kernel.org/lkml/20240119084548.2788-1-kprateek.na...@amd.com/
I do see same level of improvements (if not better) with Peter's
SM_IDLE solution:
==
Test : ipistorm (modified)
Units : Normalized runtime
Interpretation: Lower is better
Statistic : AMean
==
kernel: time [pct imp]
tip:sched/core1.00 [baseline]
tip:sched/core + revert 0.40 [60.26%]
tip:sched/core + TIF_NOTIFY_IPI 0.46 [54.88%]
tip:sched/core + SM_IDLE 0.38 [72.64%]
- Because of need_resched being set o wake up the cpu, we will not
kick the softirq to run the nohz idle load balance and get a chance to
pull a task on an idle CPU
However, this issues with need_resched() still remains. Any
need_resched() check within an interrupt context will return true if the
target CPU is perceived to be in a polling idle state by the sender as a
result of the optimization in commit b2a02fc43a1f ("smp: Optimize
send_call_function_single_ipi()").
If TIF_POLLING_NRFLAG is defined by an arch, do_idle() will set the
flag until the path hits call_cpuidle() where the flag is cleared just
before handing off the state entry to the cpuidle driver. An incoming
interrupt in this window will allow the idle path to bail early and
return before calling the driver specific routine since it'll be
indicated by TIF_NEED_RESCHED being set in the idle task's thread info.
Beyond that point, the cpuidle driver handles the idle entry.
I think an arch may define TIF_POLLING_NRFLAG just to utilize this
optimization in the generic idle path to answer Vincent's observation
on ARM32 having TIF_POLLING_NRFLAG.
I mean, it's fairly trivial to figure out if there really is going to be
work there.
Using an alternate flag instead of NEED_RESCHED to indicate a pending
IPI was suggested as the correct approach to solve this problem on the
same thread.
So adding per-arch changes for this seems like something we shouldn't
unless there really is no other sane options.
That is, I really think we should start with something like the below
and then fix any fallout from that.
The main problem is that need_resched becomes somewhat meaningless
because it doesn't only mean "I need to resched a task" and we have
to add more tests around even for those not using polling
True, however we already had some of that by having the wakeup list,
that made nr_running less 'reliable'.
The thing is, most architectures seem to have the TIF_POLLING_NRFLAG
bit, even if their main idle routine isn't actually using it, much of
Yes, I'm surprised that Arm arch has the TIF_POLLING_NRFLAG whereas it
has never been supported by the arch
the idle loop until it hits the arch idle will be having it set and will
thus tickle these cases *sometimes*.
[..snip..]
--
Thanks and Regards,
Prateek
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
Hello Russell, On 6/15/2024 7:56 PM, Russell King (Oracle) wrote: On Thu, Jun 13, 2024 at 06:15:59PM +, K Prateek Nayak wrote: o Dropping the ARM results since I never got my hands on the ARM64 system I used in my last testing. If I do manage to get my hands on it again, I'll rerun the experiments and share the results on the thread. To test the case where TIF_NOTIFY_IPI is not enabled for a particular architecture, I applied the series only until Patch 3 and tested the same on my x86 machine with a WARN_ON_ONCE() in do_idle() to check if tif_notify_ipi() ever return true and then repeated the same with Patch 4 applied. Confused. ARM (32-bit) or ARM64? You patch 32-bit ARM, but you don't touch 64-bit Arm. "ARM" on its own in the context above to me suggests 32-bit, since you refer to ARM64 later. In my first RFC posting, I had shared the results for ipistorm on an ARM64 server [1]. Vincent and Linus Walleij brought to my attention that ARM32 and ARM64 do not share the thread info flags and I probably saw a one-off behavior during my testing. Since then, it has been slightly challenging to get my hands on that machine again in a stable condition to see if there was any scenario that I might have missed but I tried a bunch of things on my x86 machine to confirm that an arch that does not define the TIF_NOTIFY_IPI would not hit these changes. Rest assured, Patch 5 is for ARM32 machines that currently define TIF_POLLING_NRFLAG [1] https://lore.kernel.org/lkml/20240220171457.703-6-kprateek.na...@amd.com/ -- Thanks and Regards, Prateek
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
On Sat, 15 Jun 2024 at 03:28, Peter Zijlstra wrote:
>
> On Fri, Jun 14, 2024 at 12:48:37PM +0200, Vincent Guittot wrote:
> > On Fri, 14 Jun 2024 at 11:28, Peter Zijlstra wrote:
>
> > > > Vincent [5] pointed out a case where the idle load kick will fail to
> > > > run on an idle CPU since the IPI handler launching the ILB will check
> > > > for need_resched(). In such cases, the idle CPU relies on
> > > > newidle_balance() to pull tasks towards itself.
> > >
> > > Is this the need_resched() in _nohz_idle_balance() ? Should we change
> > > this to 'need_resched() && (rq->nr_running || rq->ttwu_pending)' or
> > > something long those lines?
> >
> > It's not only this but also in do_idle() as well which exits the loop
> > to look for tasks to schedule
>
> Is that really a problem? Reading the initial email the problem seems to
> be newidle balance, not hitting schedule. Schedule should be fairly
> quick if there's nothing to do, no?
There are 2 problems:
- Because of NEED_RESCHED being set, we go through the full schedule
path for no reason and we finally do a sched_balance_newidle()
- Because of need_resched being set o wake up the cpu, we will not
kick the softirq to run the nohz idle load balance and get a chance to
pull a task on an idle CPU
>
> > > I mean, it's fairly trivial to figure out if there really is going to be
> > > work there.
> > >
> > > > Using an alternate flag instead of NEED_RESCHED to indicate a pending
> > > > IPI was suggested as the correct approach to solve this problem on the
> > > > same thread.
> > >
> > > So adding per-arch changes for this seems like something we shouldn't
> > > unless there really is no other sane options.
> > >
> > > That is, I really think we should start with something like the below
> > > and then fix any fallout from that.
> >
> > The main problem is that need_resched becomes somewhat meaningless
> > because it doesn't only mean "I need to resched a task" and we have
> > to add more tests around even for those not using polling
>
> True, however we already had some of that by having the wakeup list,
> that made nr_running less 'reliable'.
>
> The thing is, most architectures seem to have the TIF_POLLING_NRFLAG
> bit, even if their main idle routine isn't actually using it, much of
Yes, I'm surprised that Arm arch has the TIF_POLLING_NRFLAG whereas it
has never been supported by the arch
> the idle loop until it hits the arch idle will be having it set and will
> thus tickle these cases *sometimes*.
>
> > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > > index 0935f9d4bb7b..cfa45338ae97 100644
> > > --- a/kernel/sched/core.c
> > > +++ b/kernel/sched/core.c
> > > @@ -5799,7 +5800,7 @@ static inline struct task_struct *
> > > __pick_next_task(struct rq *rq, struct task_struct *prev, struct
> > > rq_flags *rf)
> > > {
> > > const struct sched_class *class;
> > > - struct task_struct *p;
> > > + struct task_struct *p = NULL;
> > >
> > > /*
> > > * Optimization: we know that if all tasks are in the fair class
> > > we can
> > > @@ -5810,9 +5811,11 @@ __pick_next_task(struct rq *rq, struct task_struct
> > > *prev, struct rq_flags *rf)
> > > if (likely(!sched_class_above(prev->sched_class,
> > > _sched_class) &&
> > >rq->nr_running == rq->cfs.h_nr_running)) {
> > >
> > > - p = pick_next_task_fair(rq, prev, rf);
> > > - if (unlikely(p == RETRY_TASK))
> > > - goto restart;
> > > + if (rq->nr_running) {
> >
> > How do you make the diff between a spurious need_resched() because of
> > polling and a cpu becoming idle ? isn't rq->nr_running null in both
> > cases ?
>
> Bah, true. It should also check current being idle, which then makes a
> mess of things again. Still, we shouldn't be calling newidle from idle,
> that's daft.
>
> I should probably not write code at 3am, but the below horror is what
> I came up with.
>
> ---
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 0935f9d4bb7b..cfe8d3350819 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6343,19 +6344,12 @@ pick_next_task(struct rq *rq, struct task_struct
> *prev, struct rq_flags *rf)
> * Constants for the sched_mode argument of __schedule().
> *
> * The mode argument allows RT enabled kernels to differentiate a
> - * preemption from blocking on an 'sleeping' spin/rwlock. Note that
> - * SM_MASK_PREEMPT for !RT has all bits set, which allows the compiler to
> - * optimize the AND operation out and just check for zero.
> + * preemption from blocking on an 'sleeping' spin/rwlock.
> */
> -#define SM_NONE0x0
> -#define SM_PREEMPT 0x1
> -#define SM_RTLOCK_WAIT 0x2
> -
> -#ifndef CONFIG_PREEMPT_RT
> -# define SM_MASK_PREEMPT (~0U)
> -#else
> -# define SM_MASK_PREEMPT SM_PREEMPT
> -#endif
> +#define SM_IDLE(-1)
> +#define SM_NONE
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
On Thu, Jun 13, 2024 at 06:15:59PM +, K Prateek Nayak wrote: > o Dropping the ARM results since I never got my hands on the ARM64 > system I used in my last testing. If I do manage to get my hands on it > again, I'll rerun the experiments and share the results on the thread. > To test the case where TIF_NOTIFY_IPI is not enabled for a particular > architecture, I applied the series only until Patch 3 and tested the > same on my x86 machine with a WARN_ON_ONCE() in do_idle() to check if > tif_notify_ipi() ever return true and then repeated the same with > Patch 4 applied. Confused. ARM (32-bit) or ARM64? You patch 32-bit ARM, but you don't touch 64-bit Arm. "ARM" on its own in the context above to me suggests 32-bit, since you refer to ARM64 later. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
On Sat, Jun 15, 2024 at 03:28:14AM +0200, Peter Zijlstra wrote: > On Fri, Jun 14, 2024 at 12:48:37PM +0200, Vincent Guittot wrote: > > The main problem is that need_resched becomes somewhat meaningless > > because it doesn't only mean "I need to resched a task" and we have > > to add more tests around even for those not using polling > > True, however we already had some of that by having the wakeup list, > that made nr_running less 'reliable'. Doesn't using !idle_cpu() instead of need_resched() in those balance paths already do the right thing? Checking need_resched() as an indicator of it getting work is already a bit an assumption. Also, Ingo, idle_cpu() and friends don't really belong in syscalls.c...
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
On Fri, Jun 14, 2024 at 12:48:37PM +0200, Vincent Guittot wrote: > The main problem is that need_resched becomes somewhat meaningless > because it doesn't only mean "I need to resched a task" and we have > to add more tests around even for those not using polling The converse problem is that you're adding a bunch of atomic ops that might be avoided. It might now need to set both the RESCHED and IPI flags -- and clear them again.
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
On Fri, Jun 14, 2024 at 12:48:37PM +0200, Vincent Guittot wrote:
> On Fri, 14 Jun 2024 at 11:28, Peter Zijlstra wrote:
> > > Vincent [5] pointed out a case where the idle load kick will fail to
> > > run on an idle CPU since the IPI handler launching the ILB will check
> > > for need_resched(). In such cases, the idle CPU relies on
> > > newidle_balance() to pull tasks towards itself.
> >
> > Is this the need_resched() in _nohz_idle_balance() ? Should we change
> > this to 'need_resched() && (rq->nr_running || rq->ttwu_pending)' or
> > something long those lines?
>
> It's not only this but also in do_idle() as well which exits the loop
> to look for tasks to schedule
Is that really a problem? Reading the initial email the problem seems to
be newidle balance, not hitting schedule. Schedule should be fairly
quick if there's nothing to do, no?
> > I mean, it's fairly trivial to figure out if there really is going to be
> > work there.
> >
> > > Using an alternate flag instead of NEED_RESCHED to indicate a pending
> > > IPI was suggested as the correct approach to solve this problem on the
> > > same thread.
> >
> > So adding per-arch changes for this seems like something we shouldn't
> > unless there really is no other sane options.
> >
> > That is, I really think we should start with something like the below
> > and then fix any fallout from that.
>
> The main problem is that need_resched becomes somewhat meaningless
> because it doesn't only mean "I need to resched a task" and we have
> to add more tests around even for those not using polling
True, however we already had some of that by having the wakeup list,
that made nr_running less 'reliable'.
The thing is, most architectures seem to have the TIF_POLLING_NRFLAG
bit, even if their main idle routine isn't actually using it, much of
the idle loop until it hits the arch idle will be having it set and will
thus tickle these cases *sometimes*.
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 0935f9d4bb7b..cfa45338ae97 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -5799,7 +5800,7 @@ static inline struct task_struct *
> > __pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags
> > *rf)
> > {
> > const struct sched_class *class;
> > - struct task_struct *p;
> > + struct task_struct *p = NULL;
> >
> > /*
> > * Optimization: we know that if all tasks are in the fair class we
> > can
> > @@ -5810,9 +5811,11 @@ __pick_next_task(struct rq *rq, struct task_struct
> > *prev, struct rq_flags *rf)
> > if (likely(!sched_class_above(prev->sched_class, _sched_class)
> > &&
> >rq->nr_running == rq->cfs.h_nr_running)) {
> >
> > - p = pick_next_task_fair(rq, prev, rf);
> > - if (unlikely(p == RETRY_TASK))
> > - goto restart;
> > + if (rq->nr_running) {
>
> How do you make the diff between a spurious need_resched() because of
> polling and a cpu becoming idle ? isn't rq->nr_running null in both
> cases ?
Bah, true. It should also check current being idle, which then makes a
mess of things again. Still, we shouldn't be calling newidle from idle,
that's daft.
I should probably not write code at 3am, but the below horror is what
I came up with.
---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 0935f9d4bb7b..cfe8d3350819 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -6343,19 +6344,12 @@ pick_next_task(struct rq *rq, struct task_struct *prev,
struct rq_flags *rf)
* Constants for the sched_mode argument of __schedule().
*
* The mode argument allows RT enabled kernels to differentiate a
- * preemption from blocking on an 'sleeping' spin/rwlock. Note that
- * SM_MASK_PREEMPT for !RT has all bits set, which allows the compiler to
- * optimize the AND operation out and just check for zero.
+ * preemption from blocking on an 'sleeping' spin/rwlock.
*/
-#define SM_NONE0x0
-#define SM_PREEMPT 0x1
-#define SM_RTLOCK_WAIT 0x2
-
-#ifndef CONFIG_PREEMPT_RT
-# define SM_MASK_PREEMPT (~0U)
-#else
-# define SM_MASK_PREEMPT SM_PREEMPT
-#endif
+#define SM_IDLE(-1)
+#define SM_NONE0
+#define SM_PREEMPT 1
+#define SM_RTLOCK_WAIT 2
/*
* __schedule() is the main scheduler function.
@@ -6396,11 +6390,12 @@ pick_next_task(struct rq *rq, struct task_struct *prev,
struct rq_flags *rf)
*
* WARNING: must be called with preemption disabled!
*/
-static void __sched notrace __schedule(unsigned int sched_mode)
+static void __sched notrace __schedule(int sched_mode)
{
struct task_struct *prev, *next;
unsigned long *switch_count;
unsigned long prev_state;
+ bool preempt = sched_mode > 0;
struct rq_flags rf;
struct rq *rq;
int cpu;
@@ -6409,13 +6404,13 @@ static void __sched
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
On 2024-06-14 at 12:48:37 +0200, Vincent Guittot wrote:
> On Fri, 14 Jun 2024 at 11:28, Peter Zijlstra wrote:
> >
> > On Thu, Jun 13, 2024 at 06:15:59PM +, K Prateek Nayak wrote:
> > > Effects of call_function_single_prep_ipi()
> > > ==
> > >
> > > To pull a TIF_POLLING thread out of idle to process an IPI, the sender
> > > sets the TIF_NEED_RESCHED bit in the idle task's thread info in
> > > call_function_single_prep_ipi() and avoids sending an actual IPI to the
> > > target. As a result, the scheduler expects a task to be enqueued when
> > > exiting the idle path. This is not the case with non-polling idle states
> > > where the idle CPU exits the non-polling idle state to process the
> > > interrupt, and since need_resched() returns false, soon goes back to
> > > idle again.
> > >
> > > When TIF_NEED_RESCHED flag is set, do_idle() will call schedule_idle(),
> > > a large part of which runs with local IRQ disabled. In case of ipistorm,
> > > when measuring IPI throughput, this large IRQ disabled section delays
> > > processing of IPIs. Further auditing revealed that in absence of any
> > > runnable tasks, pick_next_task_fair(), which is called from the
> > > pick_next_task() fast path, will always call newidle_balance() in this
> > > scenario, further increasing the time spent in the IRQ disabled section.
> > >
> > > Following is the crude visualization of the problem with relevant
> > > functions expanded:
> > > --
> > > CPU0 CPU1
> > >
> > > do_idle() {
> > >
> > > __current_set_polling();
> > > ...
> > >
> > > monitor(addr);
> > > if
> > > (!need_resched())
> > >
> > > mwait() {
> > > /*
> > > Waiting */
> > > smp_call_function_single(CPU1, func, wait = 1) {
> > > ...
> > > ...
> > > ...
> > > set_nr_if_polling(CPU1) {
> > > ...
> > > /* Realizes CPU1 is polling */
> > > ...
> > > try_cmpxchg(addr,
> > > ...
> > > ,
> > > ...
> > > val | _TIF_NEED_RESCHED);
> > > ...
> > > } /* Does not send an IPI */
> > > ...
> > > ... }
> > > /* mwait exit due to write at addr */
> > > csd_lock_wait() { }
> > > /* Waiting */
> > > preempt_set_need_resched();
> > > ...
> > > __current_clr_polling();
> > > ...
> > > flush_smp_call_function_queue() {
> > > ...
> > > func();
> > > } /* End of wait */ }
> > > }
> > > schedule_idle() {
> > > ...
> > >
> > > local_irq_disable();
> > > smp_call_function_single(CPU1, func, wait = 1) { ...
> > > ... ...
> > > arch_send_call_function_single_ipi(CPU1); ...
> > > \ ...
> > >\
> > > newidle_balance() {
> > > \
> > > ...
> > > /* Delay */
> > > ...
> > > \ }
> > >\ ...
> > > \-->
> > > local_irq_enable();
> > > /*
> > > Processes the IPI */
> > > --
> > >
> > >
> > > Skipping newidle_balance()
> > >
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
On Fri, 14 Jun 2024 at 11:28, Peter Zijlstra wrote:
>
> On Thu, Jun 13, 2024 at 06:15:59PM +, K Prateek Nayak wrote:
> > Effects of call_function_single_prep_ipi()
> > ==
> >
> > To pull a TIF_POLLING thread out of idle to process an IPI, the sender
> > sets the TIF_NEED_RESCHED bit in the idle task's thread info in
> > call_function_single_prep_ipi() and avoids sending an actual IPI to the
> > target. As a result, the scheduler expects a task to be enqueued when
> > exiting the idle path. This is not the case with non-polling idle states
> > where the idle CPU exits the non-polling idle state to process the
> > interrupt, and since need_resched() returns false, soon goes back to
> > idle again.
> >
> > When TIF_NEED_RESCHED flag is set, do_idle() will call schedule_idle(),
> > a large part of which runs with local IRQ disabled. In case of ipistorm,
> > when measuring IPI throughput, this large IRQ disabled section delays
> > processing of IPIs. Further auditing revealed that in absence of any
> > runnable tasks, pick_next_task_fair(), which is called from the
> > pick_next_task() fast path, will always call newidle_balance() in this
> > scenario, further increasing the time spent in the IRQ disabled section.
> >
> > Following is the crude visualization of the problem with relevant
> > functions expanded:
> > --
> > CPU0 CPU1
> >
> > do_idle() {
> >
> > __current_set_polling();
> > ...
> > monitor(addr);
> > if
> > (!need_resched())
> >
> > mwait() {
> > /*
> > Waiting */
> > smp_call_function_single(CPU1, func, wait = 1) {
> > ...
> > ...
> > ...
> > set_nr_if_polling(CPU1) {
> > ...
> > /* Realizes CPU1 is polling */
> > ...
> > try_cmpxchg(addr,
> > ...
> > ,
> > ...
> > val | _TIF_NEED_RESCHED);
> > ...
> > } /* Does not send an IPI */
> > ...
> > ... } /*
> > mwait exit due to write at addr */
> > csd_lock_wait() { }
> > /* Waiting */
> > preempt_set_need_resched();
> > ...
> > __current_clr_polling();
> > ...
> > flush_smp_call_function_queue() {
> > ...
> > func();
> > } /* End of wait */ }
> > }
> > schedule_idle() {
> > ...
> >
> > local_irq_disable();
> > smp_call_function_single(CPU1, func, wait = 1) { ...
> > ... ...
> > arch_send_call_function_single_ipi(CPU1); ...
> > \ ...
> >\
> > newidle_balance() {
> > \
> > ...
> > /* Delay */
> > ...
> > \ }
> >\ ...
> > \-->
> > local_irq_enable();
> > /*
> > Processes the IPI */
> > --
> >
> >
> > Skipping newidle_balance()
> > ==
> >
> > In an earlier attempt to solve the challenge of the long IRQ disabled
> > section, newidle_balance() was skipped when a CPU waking up from idle
> > was found to have no runnable tasks, and was transitioning back to
> > idle [2]. Tim
Re: [PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
On Thu, Jun 13, 2024 at 06:15:59PM +, K Prateek Nayak wrote:
> Effects of call_function_single_prep_ipi()
> ==
>
> To pull a TIF_POLLING thread out of idle to process an IPI, the sender
> sets the TIF_NEED_RESCHED bit in the idle task's thread info in
> call_function_single_prep_ipi() and avoids sending an actual IPI to the
> target. As a result, the scheduler expects a task to be enqueued when
> exiting the idle path. This is not the case with non-polling idle states
> where the idle CPU exits the non-polling idle state to process the
> interrupt, and since need_resched() returns false, soon goes back to
> idle again.
>
> When TIF_NEED_RESCHED flag is set, do_idle() will call schedule_idle(),
> a large part of which runs with local IRQ disabled. In case of ipistorm,
> when measuring IPI throughput, this large IRQ disabled section delays
> processing of IPIs. Further auditing revealed that in absence of any
> runnable tasks, pick_next_task_fair(), which is called from the
> pick_next_task() fast path, will always call newidle_balance() in this
> scenario, further increasing the time spent in the IRQ disabled section.
>
> Following is the crude visualization of the problem with relevant
> functions expanded:
> --
> CPU0 CPU1
>
> do_idle() {
>
> __current_set_polling();
> ...
> monitor(addr);
> if
> (!need_resched())
> mwait()
> {
> /*
> Waiting */
> smp_call_function_single(CPU1, func, wait = 1) {
> ...
> ...
> ...
> set_nr_if_polling(CPU1) {
> ...
> /* Realizes CPU1 is polling */
> ...
> try_cmpxchg(addr,
> ...
> ,
> ...
> val | _TIF_NEED_RESCHED);
> ...
> } /* Does not send an IPI */
> ...
> ... } /*
> mwait exit due to write at addr */
> csd_lock_wait() { }
> /* Waiting */
> preempt_set_need_resched();
> ...
> __current_clr_polling();
> ...
> flush_smp_call_function_queue() {
> ... func();
> } /* End of wait */ }
> } schedule_idle()
> {
> ...
>
> local_irq_disable();
> smp_call_function_single(CPU1, func, wait = 1) { ...
> ... ...
> arch_send_call_function_single_ipi(CPU1); ...
> \ ...
>\
> newidle_balance() {
> \
> ...
> /* Delay */
> ...
> \ }
>\ ...
> \-->
> local_irq_enable();
> /*
> Processes the IPI */
> --
>
>
> Skipping newidle_balance()
> ==
>
> In an earlier attempt to solve the challenge of the long IRQ disabled
> section, newidle_balance() was skipped when a CPU waking up from idle
> was found to have no runnable tasks, and was transitioning back to
> idle [2]. Tim [3] and David [4] had pointed out that newidle_balance()
> may be viable for CPUs that are idling with tick enabled, where the
> newidle_balance() has the opportunity to pull tasks onto the idle CPU.
I don't think we should be relying on this
[PATCH v2 00/14] Introducing TIF_NOTIFY_IPI flag
Hello everyone,
Before jumping into the issue, let me clarify the Cc list. Everyone have
been cc'ed on Patch 0 through Patch 3. Respective arch maintainers,
reviewers, and committers returned by scripts/get_maintainer.pl have
been cc'ed on the respective arch side changes. Scheduler and CPU Idle
maintainers and reviewers have been included for the entire series. If I
have missed anyone, please do add them. If you would like to be dropped
from the cc list, wholly or partially, for the future iterations, please
do let me know.
As long as the first three patches are applied in-order, the arch
specific enablement can be applied independently and out-of-order since
the TIF_NOTIFY_IPI flag is not used until Patch 3 and Patch 2 preps the
complete tree to handle a break out of TIF_POLLING_NRFLAG state with the
setting of either TIF_NOTIFY_IPI or TIF_NEED_RESCHED.
Quick changelog and addressing concerns from v1
===
v1: https://lore.kernel.org/lkml/20240220171457.703-1-kprateek.na...@amd.com/
v1..v2:
o Rebased the series on latest tip:sched/core at commit c793a62823d1
("sched/core: Drop spinlocks on contention iff kernel is preemptible")
Fixed a conflict with commit edc8fc01f608 ("x86: Fix
CPUIDLE_FLAG_IRQ_ENABLE leaking timer reprogram") that touched
mwait_idle_with_hints() in arch/x86/include/asm/mwait.h
o Dropping the ARM results since I never got my hands on the ARM64
system I used in my last testing. If I do manage to get my hands on it
again, I'll rerun the experiments and share the results on the thread.
To test the case where TIF_NOTIFY_IPI is not enabled for a particular
architecture, I applied the series only until Patch 3 and tested the
same on my x86 machine with a WARN_ON_ONCE() in do_idle() to check if
tif_notify_ipi() ever return true and then repeated the same with
Patch 4 applied.
o Updated benchmark results based on the latest base.
o Collected the Ack from Guo Ren for CSKY enablement.
o Dropped the RFC tag.
o Unfortunately, the series does not solve the issue highlighted by
Julia Lawall w.r.t. NUMA Balancing in [0] based on her testing of v1.
However, she did highlight a possible regression last time around
where compiling a single file took much longer with the series but I
could not reproduce it on my end. For sanity, I did rerun the same
experiment this time around and I could not see any difference.
Following are the numbers for
$ make clean
$ time make kernel/sched/core.o
---> tip:sched/core
-j1
real0m32.734s
user0m25.158s
sys 0m6.750s
-j256
real0m7.181s
user0m27.509s
sys 0m7.876s
--> tip:sched/core + TIF_NOTIFY_IPI
-j1
real0m32.408s
user0m24.826s
sys 0m6.767s
-j256
real0m7.187s
user0m27.556s
sys 0m7.602s
[0]
https://lore.kernel.org/lkml/alpine.DEB.2.22.394.2310032059060.3220@hadrien/
Individual patches have their own changelog to help with review.
With those details out of the way ...
Problem statement
=
When measuring IPI throughput using a modified version of Anton
Blanchard's ipistorm benchmark [1], configured to measure time taken to
perform a fixed number of smp_call_function_single() (with wait set to
1), an increase in benchmark time was observed between v5.7 and the
upstream release v6.7-rc6 (this was the latest upstream kernel at the
time of encountering the issue). The issue persists on v6.10-rc1 as
well.
Bisection pointed to commit b2a02fc43a1f ("smp: Optimize
send_call_function_single_ipi()") as the reason behind this increase in
runtime.
Experiments
===
Since the commit cannot be cleanly reverted on top of the current
tip:sched/core, the effects of the optimizations were reverted by:
1. Removing the check for call_function_single_prep_ipi() in
send_call_function_single_ipi(). With this change
send_call_function_single_ipi() always calls
arch_send_call_function_single_ipi()
2. Removing the call to flush_smp_call_function_queue() in do_idle()
since every smp_call_function, with (1.), would unconditionally send
an IPI to an idle CPU in TIF_POLLING mode.
Following is the diff of the above described changes which will be
henceforth referred to as the "revert":
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 31231925f1ec..735184d98c0f 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -332,11 +332,6 @@ static void do_idle(void)
*/
smp_mb__after_atomic();
- /*
-* RCU relies on this call to be done outside of an RCU read-side
-* critical section.
-*/
- flush_smp_call_function_queue();
schedule_idle();
if (unlikely(klp_patch_pending(current)))
diff --git a/kernel/smp.c b/kernel/smp.c
index f085ebcdf9e7..2ff100c41885 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -111,11 +111,9 @@ void
