Date: Wed, 6 Feb 2013 17:58:30 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Ian Lepore <ian@FreeBSD.org> Cc: "freebsd-hackers@freebsd.org" <freebsd-hackers@FreeBSD.org> Subject: Re: Request for review, time_pps_fetch() enhancement Message-ID: <20130206155830.GX2522@kib.kiev.ua> In-Reply-To: <1360125698.93359.566.camel@revolution.hippie.lan> References: <1360125698.93359.566.camel@revolution.hippie.lan>
next in thread | previous in thread | raw e-mail | index | archive | help
--tcpvmIcflXmDwk6w
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Tue, Feb 05, 2013 at 09:41:38PM -0700, Ian Lepore wrote:
> I'd like feedback on the attached patch, which adds support to our
> time_pps_fetch() implementation for the blocking behaviors described in
> section 3.4.3 of RFC 2783. The existing implementation can only return
> the most recently captured data without blocking. These changes add the
> ability to block (forever or with timeout) until a new event occurs.
>=20
> -- Ian
>=20
> Index: sys/kern/kern_tc.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- sys/kern/kern_tc.c (revision 246337)
> +++ sys/kern/kern_tc.c (working copy)
> @@ -1446,6 +1446,50 @@
> * RFC 2783 PPS-API implementation.
> */
> =20
> +static int
> +pps_fetch(struct pps_fetch_args *fapi, struct pps_state *pps)
> +{
> + int err, timo;
> + pps_seq_t aseq, cseq;
> + struct timeval tv;
> +
> + if (fapi->tsformat && fapi->tsformat !=3D PPS_TSFMT_TSPEC)
> + return (EINVAL);
> +
> + /*
> + * If no timeout is requested, immediately return whatever values were
> + * most recently captured. If timeout seconds is -1, that's a request
> + * to block without a timeout. WITNESS won't let us sleep forever
> + * without a lock (we really don't need a lock), so just repeatedly
> + * sleep a long time.
> + */
Regarding no need for the lock, it would just move the implementation into
the low quality one, for the case when one timestamp capture is lost
and caller of time_pps_fetch() sleeps until next pps event is generated.
I understand the desire to avoid lock, esp. in the pps_event() called
=66rom the arbitrary driver context. But the race is also real.
> + if (fapi->timeout.tv_sec || fapi->timeout.tv_nsec) {
> + if (fapi->timeout.tv_sec =3D=3D -1)
> + timo =3D 0x7fffffff;
> + else {
> + tv.tv_sec =3D fapi->timeout.tv_sec;
> + tv.tv_usec =3D fapi->timeout.tv_nsec / 1000;
> + timo =3D tvtohz(&tv);
> + }
> + aseq =3D pps->ppsinfo.assert_sequence;
> + cseq =3D pps->ppsinfo.clear_sequence;
> + while (aseq =3D=3D pps->ppsinfo.assert_sequence &&
> + cseq =3D=3D pps->ppsinfo.clear_sequence) {
Note that compilers are allowed to optimize these accesses even over
the sequential point, which is the tsleep() call. Only accesses to
volatile objects are forbidden to be rearranged.
I suggest to add volatile casts to pps in the loop condition.
> + err =3D tsleep(pps, PCATCH, "ppsfch", timo);
> + if (err =3D=3D EWOULDBLOCK && fapi->timeout.tv_sec =3D=3D -1) {
> + continue;
> + } else if (err !=3D 0) {
> + return (err);
> + }
> + }
> + }
> +
> + pps->ppsinfo.current_mode =3D pps->ppsparam.mode;
> + fapi->pps_info_buf =3D pps->ppsinfo;
> +
> + return (0);
> +}
> +
> int
> pps_ioctl(u_long cmd, caddr_t data, struct pps_state *pps)
> {
> @@ -1485,13 +1529,7 @@
> return (0);
> case PPS_IOC_FETCH:
> fapi =3D (struct pps_fetch_args *)data;
> - if (fapi->tsformat && fapi->tsformat !=3D PPS_TSFMT_TSPEC)
> - return (EINVAL);
> - if (fapi->timeout.tv_sec || fapi->timeout.tv_nsec)
> - return (EOPNOTSUPP);
> - pps->ppsinfo.current_mode =3D pps->ppsparam.mode;
> - fapi->pps_info_buf =3D pps->ppsinfo;
> - return (0);
> + return (pps_fetch(fapi, pps));
> #ifdef FFCLOCK
> case PPS_IOC_FETCH_FFCOUNTER:
> fapi_ffc =3D (struct pps_fetch_ffc_args *)data;
> @@ -1540,7 +1578,7 @@
> void
> pps_init(struct pps_state *pps)
> {
> - pps->ppscap |=3D PPS_TSFMT_TSPEC;
> + pps->ppscap |=3D PPS_TSFMT_TSPEC | PPS_CANWAIT;
> if (pps->ppscap & PPS_CAPTUREASSERT)
> pps->ppscap |=3D PPS_OFFSETASSERT;
> if (pps->ppscap & PPS_CAPTURECLEAR)
> @@ -1680,6 +1718,9 @@
> hardpps(tsp, ts.tv_nsec + 1000000000 * ts.tv_sec);
> }
> #endif
> +
> + /* Wakeup anyone sleeping in pps_fetch(). */
> + wakeup(pps);
> }
> =20
> /*
> _______________________________________________
> freebsd-hackers@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
> To unsubscribe, send any mail to "freebsd-hackers-unsubscribe@freebsd.org"
--tcpvmIcflXmDwk6w
Content-Type: application/pgp-signature
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)
iQIcBAEBAgAGBQJREn2lAAoJEJDCuSvBvK1Bz7YP/R5+JihMt78WpNfaMSMlPHyE
Kl5GEmAZVojEt8kBp0/uGHzR7VyN9U18lTgQlrC/BJhAlJZlC7vJE856+VS+L5zG
Cz3D4+Psx+5IeUBeL1Vq52fJ3BcRWz3dN9XRyK66vFxS0/xWqv2+F6VBMN/pWXI3
1vJmcvcanhLF17hdSF7HwLPweqVyVLf3k43SR/MxVCWkX+Yga9hz0RXfWSJAH4hz
zClNLKHsTFJfoHeSLAEJkDXoBoC/JbpWywVjhlmecnldNZKjvE+d5l2Egz39uNyn
mRus+gUKbYdYEffadBL49WrGveV3MRNnJNN8QnuB+/9Dt+/x8zkqSqDYSBxoA9Ac
b2gYhMw8ytn+oMfGVlqzO9rw4/EBfGAQrkUrLDl9orSYj+P4+Fm8p8Kb6pmzWfLN
e7za4VZrPE3fTQecDlZH9ZUmY3Gq9HAdM2SyNeuZiOJYoNEghXch/VXAHQOUpJCC
n55ipGFTgzRxfaIE810uu9NkSvlx2iAVJJw48qYjABCejvmNbg59WiBL7IyF/5Ka
j/wH2ZnuhLfj4OhSfqVXuMzMSizkGLDNB6VtXmEmvehKhSZ+7VDjBmcaLCcbkgsg
NdahVIxLoJtPDW4dv1LxmBonMPvji/nZZUz9Lc1MXJTLs9/BnCHrnVir+PDkgyHl
sIC4C6JKgoqjq41k/Flm
=9TbN
-----END PGP SIGNATURE-----
--tcpvmIcflXmDwk6w--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130206155830.GX2522>