Re: [PATCH][RFC] Signal-per-fd for RT signals

Re: [PATCH][RFC] Signal-per-fd for RT signals

[Dan Kegel]

Vitaly Luban <vitaly@luban.org> wrote:
> Attached patch is an implementation of "signal-per-fd" 
> enhancement to kernel RT signal mechanism, AFAIK first 
> proposed by A. Chandra and D. Mosberger ...
> which should dramatically increase linux based network 
> servers scalability. 
> [ Patch lives at http://www.luban.org/GPL/gpl.html ]

I have been using variations on this patch while trying
to benchmark an FTP server at a load of 10000 simultaneous
sessions (at 1 kilobyte/sec each), and noticed a few issues:

1. If a SIGINT comes in, t->files may be null, so where
   send_signal() says
         if( (info->si_fd < files->max_fds) &&
   it should say
         if( files && (info->si_fd < files->max_fds) &&
   otherwise there will be a null pointer oops.

2. If a signal has come in, and a reference to it is left
   in filp->f_infoptr, and for some reason the signal is
   removed from the queue without going through collect_signal(),
   a stale pointer may be left in filp->f_infoptr, which could
   cause a wild pointer oops.  There are two places this can happen:
   a. if send_signal() returns -EAGAIN because we're out of memory or queue space
   b. if user sets the signal handler to SIG_IGN, triggering a call 
   to rm_sig_from_queue()

I have seen the above problems in the field in my version of the patch, 
and written and tested fixes for them.  (Ah, the joys of ksymoops.)

3. Any reference to t->files probably needs to be protected by
   acquiring t->files->file_lock, else when the file table is
   expanded, any filp in use will become stale.

I have seen this problem in my version of the patch, but have not yet tackled it.
Is there any good guidance out there for how the various spinlocks
interact?  Documentation/spinlocks.txt and Documentation/DocBook/kernel-locking.tmpl 
are the best I've seen so far, but they don't get into specifics about, say,
files->file_lock and task->sigmask_lock.  Guess I'll just have to read the source.

Also, while I have verified that the patch significantly reduces 
reliable signal queue usage, I have not yet been able to measure
a reduction in CPU time in a real app.  Presumably the benefits
are in response time, which I am not set up to measure yet.

This is my first excursion into the kernel, so please be gentle.
- Dan

Re: [PATCH][RFC] Signal-per-fd for RT signals

[Vitaly Luban]

Thanks Dan,

I'll modify the patch shortly according to your information.
In fact, 2.4.6 patch is modified already according to (1)

Though I myself have not seen this effects, despite of heavy
use of modified kernel, this seems logical.

For the case of lack queue space, just to prevent it, the
queue size should always be set equal to max file descriptors
number in system. Both parameters accessible via /proc, and all
my tests are done under this setting.

Unfortunately, as we are close to release time, I do not have
enough time to complete long promised testing and patch refinement,
but, hopefully, this will happen in october.

I'll try to add changes to deal with cases (2) and (3) shortly.

Thanks again.

Vitaly.

Dan Kegel wrote:

> Vitaly Luban <vitaly@luban.org> wrote:
> > [ Patch lives at http://www.luban.org/GPL/gpl.html ]
>
> I have been using variations on this patch while trying
> to benchmark an FTP server at a load of 10000 simultaneous
> sessions (at 1 kilobyte/sec each), and noticed a few issues:
>
> 1. If a SIGINT comes in, t->files may be null, so where
>    send_signal() says
>          if( (info->si_fd < files->max_fds) &&
>    it should say
>          if( files && (info->si_fd < files->max_fds) &&
>    otherwise there will be a null pointer oops.
>
> 2. If a signal has come in, and a reference to it is left
>    in filp->f_infoptr, and for some reason the signal is
>    removed from the queue without going through collect_signal(),
>    a stale pointer may be left in filp->f_infoptr, which could
>    cause a wild pointer oops.  There are two places this can happen:
>    a. if send_signal() returns -EAGAIN because we're out of memory or queue space
>    b. if user sets the signal handler to SIG_IGN, triggering a call
>    to rm_sig_from_queue()
>
> I have seen the above problems in the field in my version of the patch,
> and written and tested fixes for them.  (Ah, the joys of ksymoops.)
>
> 3. Any reference to t->files probably needs to be protected by
>    acquiring t->files->file_lock, else when the file table is
>    expanded, any filp in use will become stale.
>
> I have seen this problem in my version of the patch, but have not yet tackled it.
> Is there any good guidance out there for how the various spinlocks
> interact?  Documentation/spinlocks.txt and Documentation/DocBook/kernel-locking.tmpl
> are the best I've seen so far, but they don't get into specifics about, say,
> files->file_lock and task->sigmask_lock.  Guess I'll just have to read the source.
>
> Also, while I have verified that the patch significantly reduces
> reliable signal queue usage, I have not yet been able to measure
> a reduction in CPU time in a real app.  Presumably the benefits
> are in response time, which I am not set up to measure yet.
>
> This is my first excursion into the kernel, so please be gentle.
> - Dan

Re: [PATCH][RFC] Signal-per-fd for RT signals

[Dan Kegel]

Vitaly Luban wrote:
> Thanks Dan,
> 
> I'll modify the patch shortly according to your information.
> In fact, 2.4.6 patch is modified already according to (1)

http://www.luban.org/GPL/one-sig-perfd-2.4.6.patch still has the bug.
Perhaps you fixed a different problem?  (cf. my patch below.)
 
> Though I myself have not seen this effects, despite of heavy
> use of modified kernel, this seems logical.

If you like, I can set up a test case.

> For the case of lack queue space, just to prevent it, the
> queue size should always be set equal to max file descriptors
> number in system. Both parameters accessible via /proc, and all
> my tests are done under this setting.

I was aiming at optimizing the normal F_ASYNC, where it is
normal and expected to receive a SIGIO, which is why I ran into
it and you didn't.
 
> Unfortunately, as we are close to release time, I do not have
> enough time to complete long promised testing and patch refinement,
> but, hopefully, this will happen in october.
> 
> I'll try to add changes to deal with cases (2) and (3) shortly.
>
> Thanks again.
> 
> Vitaly.

Here's one possible oops traceback for case 3.  This happened when
testing with about 4500 ftp sessions.  Might not correspond
exactly to your code, since I was doing some Funky Stuff.
(My patch is at http://www.kegel.com/linux/onefd-dank.patch 
 It differs from yours in that I keep poll data, not pointers,
 in struct file, overload an existing struct file field to
 avoid the space penalty, and didn't bother protecting the old
 interface used by phhttpd and x15.  Not for public consumption.)

<send_signal+69/160>      <==== crash on wild pointer here
<deliver_signal+17/80>
<send_sig_info+86/b0>
<send_sigio_to_task+c5/e0>
<ip_queue_xmit+334/470>
<tcp_rcv_established+79e/7e0>
<tcp_v4_send_check+6e/b0>
<send_sigio+58/b0>
<__kill_fasync+59/70>
<sock_wake_async+72/80>
<sock_def_readable+5d/70>
<tcp_rcv_established+399/7e0>
<tcp_v4_do_rcv+3b/120>
<tcp_v4_rcv+3e4/660>
<ip_local_deliver+fa/170>
<ip_rcv+304/380>
<tulip_interrupt+618/7d0>
<net_rx_action+17b/290>
<net_tx_action+62/140>
<do_softirq+7b/e0>
<do_IRQ+dd/f0>
<ret_from_intr+0/7>
<vfs_rename_other+28/2b0>
<expand_fd_array+d6/160>
<get_unused_fd+f2/160>
<sock_map_fd+c/1c0>
<sock_create+f3/120>
<sys_socket+2d/50>
<sys_socketcall+62/200>
<system_call+33/38>

- Dan
 
> Dan Kegel wrote:
> 
> > Vitaly Luban <vitaly@luban.org> wrote:
> > > [ Patch lives at http://www.luban.org/GPL/gpl.html ]
> >
> > I have been using variations on this patch while trying
> > to benchmark an FTP server at a load of 10000 simultaneous
> > sessions (at 1 kilobyte/sec each), and noticed a few issues:
> >
> > 1. If a SIGINT comes in, t->files may be null, so where
> >    send_signal() says
> >          if( (info->si_fd < files->max_fds) &&
> >    it should say
> >          if( files && (info->si_fd < files->max_fds) &&
> >    otherwise there will be a null pointer oops.
> >
> > 2. If a signal has come in, and a reference to it is left
> >    in filp->f_infoptr, and for some reason the signal is
> >    removed from the queue without going through collect_signal(),
> >    a stale pointer may be left in filp->f_infoptr, which could
> >    cause a wild pointer oops.  There are two places this can happen:
> >    a. if send_signal() returns -EAGAIN because we're out of memory or queue space
> >    b. if user sets the signal handler to SIG_IGN, triggering a call
> >    to rm_sig_from_queue()
> >
> > I have seen the above problems in the field in my version of the patch,
> > and written and tested fixes for them.  (Ah, the joys of ksymoops.)
> >
> > 3. Any reference to t->files probably needs to be protected by
> >    acquiring t->files->file_lock, else when the file table is
> >    expanded, any filp in use will become stale.
> >
> > I have seen this problem in my version of the patch, but have not yet tackled it.
> > Is there any good guidance out there for how the various spinlocks
> > interact?  Documentation/spinlocks.txt and Documentation/DocBook/kernel-locking.tmpl
> > are the best I've seen so far, but they don't get into specifics about, say,
> > files->file_lock and task->sigmask_lock.  Guess I'll just have to read the source.
> >
> > Also, while I have verified that the patch significantly reduces
> > reliable signal queue usage, I have not yet been able to measure
> > a reduction in CPU time in a real app.  Presumably the benefits
> > are in response time, which I am not set up to measure yet.
> >
> > This is my first excursion into the kernel, so please be gentle.
> > - Dan

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

[Dan Kegel]

I'm playing with a patch that collapses duplicate sigio signals.
It refers to task->files during signal generation,
so it crashes on a uniprocessor system when an interrupt that
causes a sigio comes in while the process was busy expanding its
file table, to wit:

> <send_signal+69/160>      <==== crash on wild pointer here
> <deliver_signal+17/80>
> <send_sig_info+86/b0>
> <send_sigio_to_task+c5/e0>
> <ip_queue_xmit+334/470>
> <tcp_rcv_established+79e/7e0>
> <tcp_v4_send_check+6e/b0>
> <send_sigio+58/b0>
> <__kill_fasync+59/70>
> <sock_wake_async+72/80>
> <sock_def_readable+5d/70>
> <tcp_rcv_established+399/7e0>
> <tcp_v4_do_rcv+3b/120>
> <tcp_v4_rcv+3e4/660>
> <ip_local_deliver+fa/170>
> <ip_rcv+304/380>
> <tulip_interrupt+618/7d0>
> <net_rx_action+17b/290>
> <net_tx_action+62/140>
> <do_softirq+7b/e0>
> <do_IRQ+dd/f0>
> <ret_from_intr+0/7>
> <vfs_rename_other+28/2b0>
> <expand_fd_array+d6/160>
> <get_unused_fd+f2/160>
> <sock_map_fd+c/1c0>
> <sock_create+f3/120>
> <sys_socket+2d/50>
> <sys_socketcall+62/200>
> <system_call+33/38>

Documentation/DocBook/kernel-locking seems to say spin_lock_bh()
is the way to address conflicts like this between a softirq and
the process.

The code referring to task->files is sprinkled into kernel/signal.c
by the patch, in routines where task->sigmask_lock is already
acquired with spin_lock_irqsave() or spin_lock_irq() before entry.

Question: is it safe to acquire task->files->file_lock with spin_lock_bh()
inside these routines, or am I risking deadlock?  Is it an issue that
I might be holding the lock while traversing a list thousands of
entries long?  For example, is adding file_lock locks to rm_sig_from_queue() as
follows safe, efficient, and proper?

/*
 * Remove signal sig from t->pending.
 * Returns 1 if sig was found.
 *
 * All callers must be holding t->sigmask_lock.
 */
static int rm_sig_from_queue(int sig, struct task_struct *t)
{
    struct sigqueue *q, **pp;
    struct sigpending *s = &t->pending;
    struct files_struct *files = t->files;

    if (!sigismember(&s->signal, sig))
        return 0;

    sigdelset(&s->signal, sig);

    pp = &s->head;

    if (files)
        spin_lock_bh(files->file_lock);    /* ??? */
    while ((q = *pp) != NULL) {
        if (q->info.si_signo == sig) {
            /* Must clear f_revents for FASYNC sigs. dank 9/2001 */
            struct file *filp;
            if (files
            &&  (q->info.si_fd < files->max_fds)
            &&  ((filp = files->fd[ q->info.si_fd ]) != 0)
            &&  (filp->f_flags & FASYNC)
            &&  (filp->f_owner.signum == sig))
                filp->f_revents = 0;

            if ((*pp = q->next) == NULL)
                s->tail = pp;
            kmem_cache_free(sigqueue_cachep,q);
            atomic_dec(&nr_queued_signals);
            continue;
        }
        pp = &q->next;
    }
    if (files)
         spin_unlock_bh(files->file_lock);   /* ??? */
    return 1;
}

Thanks,
Dan

p.s. The bug was introduced by Luban's one-signal-per-fd patch; I ran into 
it while running my variant of his patch, http://www.kegel.com/linux/onefd-dank.patch
My variant makes the following changes:
* it fixes an oops related to 'task->files' being null during SIGINT
* it fixes two oopses related to signal queue overflow
* 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 perfectly willing to write patches for phhttpd and x15 to use the
  new interface in the unlikely event that everyone agrees my interface change 
  is good.

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

[Vitaly Luban]

Dan Kegel wrote:

> so it crashes on a uniprocessor system when an interrupt that
> causes a sigio comes in while the process was busy expanding its
> file table, to wit:

I'm checking that. My variant of patch will be posted tomorrow, after I'll
finish testing it. It will be against 2.4.9 and addresses all crashes you've
mentioned.


>
> p.s. The bug was introduced by Luban's one-signal-per-fd patch; I ran into
> it while running my variant of his patch, http://www.kegel.com/linux/onefd-dank.patch
> My variant makes the following changes:
> * it fixes an oops related to 'task->files' being null during SIGINT
> * it fixes two oopses related to signal queue overflow
> * 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,

Keeping pointer allows for additional sanity check to make sure I'm not screwed,
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. 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.

Thanks,
    Vitaly.

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

[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

Re: [PATCH][RFC] Signal-per-fd for RT signals

[Vitaly Luban]

[-- Attachment #1: Type: text/plain, Size: 847 bytes --]

Dan Kegel wrote:

> 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.

Well, when I have an app, that deals primarily with network I/O, and, at the same time
has some file I/O, it's only logical to have all I/O handling within the same event
loop, and if loop is RT-signals based...

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

Could you please try attached one? It's mostly untested, but my home site
will be down next week.

And thank you for your efforts also :)
I'm looking forward to see a test case, all I could come up with happily
runs on the old version.

Thanks again,
    Vitaly.


[-- Attachment #2: one-sig-perfd-2.4.9.patch.gz --]
[-- Type: application/x-gzip, Size: 2289 bytes --]

Re: [PATCH][RFC] Signal-per-fd for RT signals

[Dan Kegel]

Vitaly Luban wrote:
> 
> Dan Kegel wrote:
> 
> > 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.
> 
> Well, when I have an app, that deals primarily with network I/O, and, at the
> same time has some file I/O, it's only logical to have all I/O handling within
> the same event loop, and if loop is RT-signals based...

I agree that having a single event source and event loop is attractive,
and want Linux to support it.  But my proposal doesn't get in the way of
that at all.  Let's say you use my patch to pick up network readiness events,
and have aio_{read,write}() send realtime signals when disk I/O is complete.
You can distinguish them nicely by using separate signal numbers, or you
can distinguish them based on the value of si_code (which will be SI_ASYNC
for the completion notifications, and SI_SIGIO or something like that for the
readiness notifications).
No problem, and you still have a unified event queue.

> > Thanks again for creating and maintaining your patch!  I look forward to
> > stress-testing the next version.
> 
> Could you please try attached one? It's mostly untested, but my home site
> will be down next week.
> 
> And thank you for your efforts also :)
> I'm looking forward to see a test case, all I could come up with happily
> runs on the old version.

OK, I'll see if I can whip together a test case tomorrow.  (No promises --
my wife is starting to wonder if I'll ever emerge from my office.)

- Dan

Re: [PATCH][RFC] Signal-per-fd for RT signals; write_lock_bh(file_lock)?

[Dan Kegel]

Vitaly Luban wrote:
> Could you please try attached one? It's mostly untested, but my home site
> will be down next week.

I just had time to read your new patch; have not yet run it.
I think there may be some problems in send_signal:

1) might still have null pointer dereference.  If files is NULL, following section
+        if( newsignal && q )
+            filep->f_infoptr = q;
+        write_unlock( &files->file_lock);
may crash due to NULL filep and unlocking an already unlocked lock.
BTW, you might be able to use a read lock there.

2) You're using a kind of spinlock that compiles to a no-op on single
processor, but the crash (see the old oops traceback in
http://marc.theaimsgroup.com/?l=linux-kernel&m=100055931808169&w=2 )
happens on a single processor, so your locking shouldn't help.
The conflict is between a bottom half and normal.  Thus write_lock, besides
not helping on uniprocessor, might cause a deadlock on smp.  
Might have to convert the write_lock in get_unused_fd() and friends to be a
write_lock_bh() instead to prevent the bottom half from running until the normal
part is done with the file lock!

This bottom-half-vs-normal issue worries me greatly.  It may take
a lot of thought to fix this.  Then again, maybe I'm just confused;
I've never dealt with kernel spinlocks before, and anyone who reads
my posts regularly knows that I'm wrong most of the time.  Anyone
who actually *understands* this stuff, please speak up with corrections...

3) you still save info in the file even if you don't end up queuing
a signal; your changes in send_signal should be in the default case
of the switch, just to keep things in a good state.

4) collect_signal references the file table, so it needs to lock it;
probably read_lock (but maybe write_lock, I dunno), and probably
_bh, too, to avoid deadlock.

> I'm looking forward to see a test case, all I could come up with happily
> runs on the old version.

I'll try to put it together today.  
- Dan