svn commit: r268570 - head/sys/kern

[Konstantin Belousov]

On Sun, Jul 13, 2014 at 10:01:16AM +0200, Mateusz Guzik wrote:
> On Sat, Jul 12, 2014 at 08:10:15PM +0300, Konstantin Belousov wrote:
> > On Sat, Jul 12, 2014 at 06:53:47PM +0200, Mateusz Guzik wrote:
> > > There can be only one 'struct file' for devctl and devclose is only
> > > called when it is about to be destroyed.
> > > 
> > > fd = open("/dev/devctl");
> > > close(dup(fd));
> > > 
> > > does not result in calling devclose.
> > > 
> > > If devclose is indeed reachable whlie fds are active this code needs
> > > serious help since devsoftc.inuse is of no use whatsoever.
> > > 
> > > There is no support for multiple readers in the sense that each event
> > > can be read only once, hence the restriction on open.
> > > 
> > > 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.
> > 
> 
> But this is again harmless as far as kernel consistency goes.
> 
> > 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.
> > 
> 
> This would be an option, but then what to do with events? Whoever
> happens to read one consumes it? Current behaviour of denying further
> opens seems safer since the process which opened can be sure nobody
> suddenly steals any. If it decides to 'share' device fd, well, it is its
> own problem.
> 
> Assignments in devclose are not going to be executed as long as
> there is an active fp, thus this is a nop from perspective of devctl
> users.
> 
> > 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.
> 
> 
> This was meant to be a cosmetic change, I'm not interested in working on
> this, sorry. I can revert the change if you want.
I do not see a point in reverting it.
I am more interested in (somebody) converting such places to use cdevpriv.

> 
> I plan to merge the following to stable/10:
> - r264114 Fix SIGIO delivery. Use fsetown() to handle file descriptor owner
> ioctl and use pgsigio() to send SIGIO.
> - r264310 Add kqueue support for devctl.
> 
> Do you have any objections?
> 
> If you don't want a revert of this patch, I'll MFC it as well.

I have no objections.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-all/attachments/20140713/c0c1c421/attachment.sig>