Date: Sat, 12 Jul 2014 20:10:15 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Mateusz Guzik <mjg@FreeBSD.org> Subject: Re: svn commit: r268570 - head/sys/kern Message-ID: <20140712171015.GT93733@kib.kiev.ua> In-Reply-To: <20140712165346.GA16884@dft-labs.eu> References: <201407121535.s6CFZ42f063120@svn.freebsd.org> <20140712161801.GS93733@kib.kiev.ua> <20140712165346.GA16884@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
--a7SarfeDDnAelrrD Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jul 12, 2014 at 06:53:47PM +0200, Mateusz Guzik wrote: > On Sat, Jul 12, 2014 at 07:18:01PM +0300, Konstantin Belousov wrote: > > On Sat, Jul 12, 2014 at 03:35:04PM +0000, Mateusz Guzik wrote: > > > Author: mjg > > > Date: Sat Jul 12 15:35:04 2014 > > > New Revision: 268570 > > > URL: http://svnweb.freebsd.org/changeset/base/268570 > > >=20 > > > Log: > > > Clear nonblock and async on devctl close instaed of open. > > > =20 > > > This is a purely cosmetic change. > > >=20 > > > Modified: > > > head/sys/kern/subr_bus.c > > >=20 > > > Modified: head/sys/kern/subr_bus.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=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D > > > --- head/sys/kern/subr_bus.c Sat Jul 12 15:19:30 2014 (r268569) > > > +++ head/sys/kern/subr_bus.c Sat Jul 12 15:35:04 2014 (r268570) > > > @@ -438,8 +438,6 @@ devopen(struct cdev *dev, int oflags, in > > > } > > > /* move to init */ > > > devsoftc.inuse =3D 1; > > > - devsoftc.nonblock =3D 0; > > > - devsoftc.async =3D 0; > > > mtx_unlock(&devsoftc.mtx); > > > return (0); > > > } > > > @@ -450,6 +448,8 @@ devclose(struct cdev *dev, int fflag, in > > > =20 > > > mtx_lock(&devsoftc.mtx); > > > devsoftc.inuse =3D 0; > > > + devsoftc.nonblock =3D 0; > > > + devsoftc.async =3D 0; > > > cv_broadcast(&devsoftc.cv); > > > funsetown(&devsoftc.sigio); > > > mtx_unlock(&devsoftc.mtx); > > This is not pure cosmetic. devctl does not track closes, so opens are > > not matched with close calls. Now any close clears nonblock and async, > > which is not how it was done before. Tracking close calls does not > > work reliably. > >=20 > > FWIW, I think that both old and new behaviour are bugs, and I am not > > sure why changing one for another (instead of fix). >=20 > I don't see that, but maybe I'm misreading the code. >=20 > There can be only one 'struct file' for devctl and devclose is only > called when it is about to be destroyed. >=20 > fd =3D open("/dev/devctl"); > close(dup(fd)); >=20 > does not result in calling devclose. >=20 > If devclose is indeed reachable whlie fds are active this code needs > serious help since devsoftc.inuse is of no use whatsoever. >=20 > There is no support for multiple readers in the sense that each event > can be read only once, hence the restriction on open. >=20 > On the other hand it is indeed possible to obtain multiple fds for > devctl which is harmless as far as consistency in the kernel goes. > Concurrent reads are serialized with a mutex and closes are invisible to > the device, except for the last one which destroys fp. Well, I argue that devsoftc.inuse is broken too. It was introduced in time when the only way of tracking the shared use of cdev was cloning. Note that it does not prevent multiple threads from simultaneously fall into the cdevsw methods; e.g. cv_wait_sig() in devread() drops devsoftc.mtx etc. IMO the right thing to do is to allow multiple opens and to keep non-blocking attribute and async bindings in the per-file structure. Then your change would be real nop. BTW, another, this time really big, user of the private (yes) cloning implementation is snd(4). The conversion of it to devfs_cdevpriv(9) would be also highly desirable. --a7SarfeDDnAelrrD Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJTwWv3AAoJEJDCuSvBvK1BKcEP+wYHWmSVzUnFJk81WrVgjch6 CKbCmceBa2UvzNc5tZ6NcNJxkWLZ7ISsFL0rMqpDbeJCOwbWddWty+RGwOmZ9mPS JXg6VTofyeBzsENGQoliEuyNIEU22lg7E17aENOmKQiLlFhVV1t7UBtysA+zYXio kommsAIA0gdQktt3KYGw7CmlwvQXfrFheiRHFMfLTX0tiotmrrfCd+8fuGbZgl6K ozGTgQ1PJqxXBiF4ttk9UDzTsp/Yv9YNLEDDfvjcTVQDvdt5weT6zvfa7SIYHYbk BPqxBLh3//YuvKF1s1C0g6q0/iGABh54eNa9hVc3JEkh7IKoVoztvKY2MWnj/2ps ci13zfF+S6JRXI1GOztvj5lnfwjvgmRy/o+8/c/DCR8zJSnialoIqjQI8kPZJYNp wnn/4CoF9Cc99naMuHWekqSZd9kilHVOy8NqeQMyJ8fviOcG6J/3YW0W5gAMc42C OcF7s/Aa99mIvNGgsCO7V/B75iAg13oWku8W/oftHksaIyzYZKeZIVGj1md9WCUf mSRbauFL3/doimjJO2w1T5w17UYDgzaSNwyqhcfwFBKaxnQcsheibZ9RSxVA1n8A 8gLd7rAemtssczB7FLQST2jWeT7OZLP6YK/xWDCHAZyKkSEjtzOudNsxSLfZ7qjh rL2scyCya01joojsXyQv =QQX7 -----END PGP SIGNATURE----- --a7SarfeDDnAelrrD--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140712171015.GT93733>