Date: Mon, 25 Apr 2016 16:51:24 +0100
From: James Hogan <james.hogan@imgtec.com>
To: Martin Schwidefsky <schwidefsky@de.ibm.com>
CC: Thomas Gleixner <tglx@linutronix.de>, <linux-kernel@vger.kernel.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>
Subject: Re: [PATCH] clockevents: Retry programming min delta up to 10 times
Message-ID: <20160425155124.GA22522@jhogan-linux.le.imgtec.org>
References: <1461321611-6159-1-git-send-email-james.hogan@imgtec.com>
 <20160425154858.7695e109@mschwide>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="k1lZvvs/B4yU6o8G"
Content-Disposition: inline
In-Reply-To: <20160425154858.7695e109@mschwide>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3852
Lines: 102

--k1lZvvs/B4yU6o8G
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 25, 2016 at 03:48:58PM +0200, Martin Schwidefsky wrote:
> On Fri, 22 Apr 2016 11:40:11 +0100
> James Hogan <james.hogan@imgtec.com> wrote:
>=20
> > Under virtualisation it is possible to get unexpected latency during a
> > clockevent device's set_next_event() callback which can make it return
> > -ETIME even for a delta based on min_delta_ns.
>=20
> Do you have an example for this behavior?

The place where I've observed it is arch/mips/kernel/cevt-r4k.c, which
returns -ETIME when the delay is too short for it to be able to set it
and read back the timer.

I've also recently (Friday afternoon) seen a report of it apparently
happening with the MIPS GIC clockevent driver too
(drivers/clocksource/mips-gic-timer.c) which has similar logic, probably
copied from cevt-r4k, and this patch appeared to help (I still need to
confirm that one). That wasn't with virtualisation, but was on a
multithreaded core being stress tested, a case when its also hard to
find a guaranteed min delta.

> I would call that a BUG in the implementation of the clockevent
> device, no?

Several drivers seem to do that. I'm open to alternatives. Do you think
the driver should retry itself when it detects this race may have been
hit?

>=20
> > The clockevents_program_min_delta() implementation for
> > CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST=3Dn doesn't handle retries when t=
his
> > happens, nor does clockevents_program_event() or its callers when force
> > is true (for example hrtimer_reprogram()). This can result in hangs
> > until the clock event device does a full period.
>=20
> Is that because some clockevent devices can not program the minimum delta
> in some corner cases?

yes.

I think it actually ended up causing an arithmetic overflow somewhere in
ktime_get() (I'd have to dig through my notes to find specifics)
which resulted in __iter_div_u64_rem() being given an excessively large
dividend, which effectively hung the CPU.

Thanks
James

>=20
> > It isn't appropriate to use MIN_ADJUST in this case as occasional
> > hypervisor induced high latency will cause min_delta_ns to quickly
> > increase to the maximum.
>=20
> I agree, the whole minimum delta adjustment is quite broken on a virtuali=
zed
> system. On s390 we have seen the rise of the min_delta_ns to the maximum
> value due to a busy hypervisor.
>=20
> > Instead, borrow the retry pattern from the MIN_ADJUST case, but without
> > making adjustments. We retry up to 10 times before giving up.
>=20
> That will add a few unnecessary instruction for architectures that have a
> sane set_next_event function, namely those that always returns 0. Should
> not be too bad though.=20
>=20
> --=20
> blue skies,
>    Martin.
>=20
> "Reality continues to ruin my life." - Calvin.
>=20

--k1lZvvs/B4yU6o8G
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: Digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJXHjz8AAoJEGwLaZPeOHZ6pUcP/jXBKMMttaRHhYBbY0yZxZ3T
Td/S8/DfImv1KgRUhcjokZH1Fi7QvysTsLLaM5H1xhYSPVi0DpQNvwIxyR8mHYrj
kfI+WedYXKcJ8s/gGraZX292Tcj9FiMwiLBiR29Lki0AzbBBseg6EC0d2w6a120f
V9YJufrXQHLYOYDS7FCvH73GgZVX6vZfuCSx+0jG5Kco1nKGoc2tYMdnnsTgz5Bp
Qlap2baKgvFiFm5FY1UmMI+TsbrfRRY6mErYLGf7OHnYLWijksSKN4/5gPrSohvt
JGiZNDTOM2cUraFLvUjshNNsGrTXO7b6+LGFM8rItZFOg9HlaqaHbZ8U18BJC1yi
RUb8qln5ed159hdh3UCh6FFitypj6mfVKVejtn5YFrjNQ0tnENEtcwINGVEDL92h
UNMhffvQGprBi/K7cii/q4SsJhbN3EviPi3blNdLYvEKreJSqd65lQ5M3Huiz6/T
laytswZ9Sdps0aY4HLUxI9yiH/DPGHSuXSF/1ACJxC4gz58IbbUTYkrgwdyYCEW+
uMMYks7oorm1rI2yh0QElUM5epTy1Yei+UsXRVwUlpqJGvi9SolvHGznH8QOBJmM
clgn5YOguZ/cCROKXXmkW55SfjsZm6AKqBLh30uVc2Ql/1FZatZYtXEkrGQ7vmxq
iL4kd9/cSgI52v5U1sLe
=G5eC
-----END PGP SIGNATURE-----

--k1lZvvs/B4yU6o8G--