Re: spin_lock_bh() usage check, please (was: [PATCH][RFC] Signal-per-fd

[Dan Kegel]

Vitaly Luban wrote:
> Dan Kegel wrote: ...
> > My variant makes the following changes: ..
> > * it keeps poll data, not pointers, in struct file.  This saves space
> >   and makes the consequence of screwing up less severe.
> > * it overloads an existing struct file field to avoid the space penalty;
> >   it doesn't bloat struct file.
> > * it assumes that it's better to keep the kernel uncluttered, so it
> >   changes the meaning of siginfo.si_code rather than introduces new ioctl's.
> >   (I hear the Austin draft of the Posix single unix spec is deleting all mention
> >    of SIGIO, so it looks like we're free to 'improve' that interface freely
> >    once Austin becomes the law of the land :-)
> 
> I'm reluctant to make such changes, like f_error field overloading because
> a) it may backbite in future if you'll use this mechanism for all I/O and not
> only sockets, since you are interfering with it's legitimate usage, in short, 
> it makes patch more dependent of possible kernel code changes
> b) I want the default kernel behavior to be exactly the same, as if without this
> patch, thus additional fcntl to control this feature. If you do not activate it,
> you don't get it,

You're absolutely right.  My patch is aggressive.  It does things that would
require buy-in by a lot of people.  Your patch is much safer.

But I doubt very much that SIGIO style readiness notification will ever
be used with files.  aio_{read,write} style completion notification is
much more appropriate for file I/O, and my proposal (if I make it) will not
affect that.

> Keeping pointer allows for additional sanity check to make sure I'm not screwed,
That's a good thing, yes.

> and cheap update events information in siginfo.
> Keeping interests strips you of this ability. And also, you have to have
> additional method of gettong events information in user space, because you 
> are not updating siginfo, and cannot rely on it anymore. 

I disagree there.  My patch's updates are just as cheap as yours, and
programs which use si_band instead of si_code are potentially unaffected by my
patch; it's possible to write programs that work identically with or without
my patch.  (My Poller_sigio.cc is such a program.)

> So, to utilize signal per fd feature you have to modify
> event loop and not file descriptor setup, which I'd also try to avoid.

Agreed.  It is messy to have to go out and 'fix' existing programs to 
be compatible with a proposed interface change like this.  You are absolutely
right to avoid the kind of change I made.

On the other hand, my change might in the long run yield both a simpler
kernel and simpler userland programs.  That's the only reason I am playing
with this approch.  I'm not seriously proposing it as yet.  I only posted it
so you could see how I addressed the first two kinds of oops'es; the fixes
should be similar in your patch.

Thanks again for creating and maintaining your patch!  I look forward to
stress-testing the next version.
- Dan