Re: [PATCH v3] cpuidle: poll_state: Add time limit to poll_idle()

Rik van Riel
2018-03-25T21:45:31Z

[-- Attachment #1: Type: text/plain, Size: 3269 bytes --]

On Sun, 2018-03-25 at 23:34 +0200, Rafael J. Wysocki wrote:
> On Sunday, March 25, 2018 10:15:52 PM CEST Rik van Riel wrote:
> >
> > --=-e8yLbs0aoH4SrxOskwwl
> > Content-Type: text/plain; charset="UTF-8"
> > Content-Transfer-Encoding: quoted-printable
> >
> > On Thu, 2018-03-22 at 18:09 +0100, Rafael J. Wysocki wrote:
> > > =20
> > > +++ linux-pm/drivers/cpuidle/poll_state.c
> > > @@ -10,6 +10,7 @@
> > > #include <linux/sched/idle.h>
> > > =20
> > > #define POLL_IDLE_TIME_LIMIT (TICK_NSEC / 16)
> > > +#define POLL_IDLE_COUNT 1000
> > > =20
> > > static int __cpuidle poll_idle(struct cpuidle_device *dev,
> > > struct cpuidle_driver *drv, int
> > > index)
> > > @@ -18,9 +19,14 @@ static int __cpuidle poll_idle(struct cp
> > > =20
> > > local_irq_enable();
> > > if (!current_set_polling_and_test()) {
> > > + unsigned int loop_count =3D 0;
> > > +
> > > while (!need_resched()) {
> > > cpu_relax();
> > > + if (loop_count++ < POLL_IDLE_COUNT)
> > > + continue;
> > > =20
> > > + loop_count =3D 0;
> > > if (local_clock() - time_start >
> > > POLL_IDLE_TIME_LIMIT)
> > > break;
> > > }
> >
> > OK, I am still seeing a performance
> > degradation with the above, though
> > not throughout the entire workload.
> >
> > It appears that making the idle loop
> > do anything besides cpu_relax() for
> > a significant amount of time slows
> > things down.
>
> I see.
>
> > I plan to try two more things:
> >
> > 1) Disable polling on SMT systems, with
> > the idea that putting one thread to
> > sleep with monitor/mwait in C1 will
> > allow the other thread to run faster.
>
> Sounds plausible.
>
> > 2) Insert more cpu_relax() calls into the
> > main loop, so the CPU core spends more
> > of its time in cpu_relax() and less
> > time doing other things:
>
> Well, maybe it's a matter of doing cpu_relax() between any other bits
> of
> significant computation in there:

That sounds like a plausible thing to try.
Let me kick off a test with that variant, too.

> drivers/cpuidle/poll_state.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/cpuidle/poll_state.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/poll_state.c
> +++ linux-pm/drivers/cpuidle/poll_state.c
> @@ -10,6 +10,7 @@
> #include <linux/sched/idle.h>
>
> #define POLL_IDLE_TIME_LIMIT (TICK_NSEC / 16)
> +#define POLL_IDLE_COUNT 200
>
> static int __cpuidle poll_idle(struct cpuidle_device *dev,
> struct cpuidle_driver *drv, int
> index)
> @@ -18,11 +19,21 @@ static int __cpuidle poll_idle(struct cp
>
> local_irq_enable();
> if (!current_set_polling_and_test()) {
> + unsigned int loop_count = 0;
> +
> while (!need_resched()) {
> cpu_relax();
> -
> + if (loop_count++ < POLL_IDLE_COUNT) {
> + cpu_relax();
> + continue;
> + }
> + cpu_relax();
> + loop_count = 0;
> + cpu_relax();
> if (local_clock() - time_start >
> POLL_IDLE_TIME_LIMIT)
> break;
> +
> + cpu_relax();
> }
> }
> current_clr_polling();
>
>
--
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]