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

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

[Vitaly Luban]

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

Hi,

I'm sorry, the previous message slipped out w/o subj.

Attached patch is an implementation of "signal-per-fd"
enhancement to kernel RT signal mechanism, AFAIK first
proposed by A. Chandra and D. Mosberger :

http://www.hpl.hp.com/techreports/2000/HPL-2000-174.html

which should dramatically increase linux based network
servers scalability.

Patch is made against 2.4.4 tree.

This patch allows to set signal-per-fd mode per each
file descriptor by introducing new fcntls "F_SETAUXFL"
and "F_GETAUXFL" with one possible argument "O_ONESIGFD"
to F_SETAUXFL defined.

When set, no additional siginfo will be queued for an
RT signal, generated by event on file descriptor, while
there is already one queued, though event report field
in already queued struct siginfo - "si_band" is updated.

I'd also like to hear an opinion on the signal filtering
capability. I.e, it's relatively easy to filter signals
upon an interest mask, supplied by the same F_SETAUXFL in
the form of POLL_... This will bring functionality of RT
signals event notification on the level with 'select' or
'poll' one, while more efficient and scalable. If there's
an interest in such a feature, I'd be eager to publish a
patch.

Thanks,
    Vitaly.

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

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

[Gerold Jury]

Vitaly Luban wrote:
> 
> Hi,
> 
<snip/>
> the form of POLL_... This will bring functionality of RT
> signals event notification on the level with 'select' or
> 'poll' one, while more efficient and scalable. If there's
> an interest in such a feature, I'd be eager to publish a
> patch.
> 
> Thanks,
>     Vitaly.
> 
I have been waiting for this patch since 2.4.0.

The SIGIO signal is a nightmare when it arrives :
  The machine is already under high load and has to stop
  using the most efficient way to handle it.

The filter changes would be the cream on top of this patch.
Do not hurry, but please not for long.

best Regards

Gerold

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

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