timeout storms make the box unresponsive

timeout storms make the box unresponsive

[Balazs Scheidler]

Hi,

while trying to reproduce the oops in replace_in_hashes, I compiled a kernel
with netfilter debug enabled. From time-to-time the box simply freezes for a
minute or two, and then continues working as if nothing had happened.

I understand that a kernel with assertions enabled will be slower, but I did
not see this behaviour with the previous kernel and the same load. I've
pressed SysRq+ShowPC and fed that through a ksymoops to see what's going on.
It turned out that ip_nat_cleanup_conntrack() was working (or waiting for
ip_nat_lock).

My test setup is an SMP box (dual Athlon MP 1800+), and I'm downloading
files from a custom webserver using 7 client PCs, with apachebench, each
starting 500 concurrent threads. As there are many connections (each HTTP
session downloads 1k data without keepalive) many connections end at the
same time, thus many of them exit the TIME_WAIT state at the same time. (I
have about 100k entries in /proc/net/ip_conntrack) 

When the box is not frozen, it is otherwise completely responsive, and is
processing 30k interrupts/sec happily.

Maybe we should consider removing expired entries from the NAT hash at a
single go, instead of the current lock-remove-unlock for each entry. Or
create a cleanup thread which cleans up conntrack entries regularly, but not
in the timer interrupt context.

-- 
Bazsi
PGP info: KeyID 9AF8D0A9 Fingerprint CD27 CFB0 802C 0944 9CFD 804E C82C 8EB1

Re: timeout storms make the box unresponsive

[Harald Welte]

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

On Fri, Jul 25, 2003 at 08:56:44PM +0200, Balazs Scheidler wrote:

> Maybe we should consider removing expired entries from the NAT hash at a
> single go, instead of the current lock-remove-unlock for each entry. Or
> create a cleanup thread which cleans up conntrack entries regularly, but not
> in the timer interrupt context.

Rusty, Jamal and I were talking about this conntrack cleanup yesterday
(we're currently all at OLS).  The conclusion was that a cleanup kernel thread 
is what we want to have.

This can be combined with the ip_conntrack timer change.  Instead of
resetting the timer inside struct ip_conntrack every time, we just write
jiffies+timeout into a field.  

And then there is a kernel thread that cleans up all expired entries in
regular intervals.

If I understood correctly, Rusty already has some old patches in this
area, and he is most likely to put an update of these patches out in the
next weeks before the netfilter developer workshop.

If this is too late and somebody else wants to work on this, please
coordinate with Rusty to not duplicate any work.  

> Bazsi
> PGP info: KeyID 9AF8D0A9 Fingerprint CD27 CFB0 802C 0944 9CFD 804E C82C 8EB1
> 

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: timeout storms make the box unresponsive

[Jozsef Kadlecsik]

On Fri, 25 Jul 2003, Harald Welte wrote:

> On Fri, Jul 25, 2003 at 08:56:44PM +0200, Balazs Scheidler wrote:
>
> > Maybe we should consider removing expired entries from the NAT hash at a
> > single go, instead of the current lock-remove-unlock for each entry. Or
> > create a cleanup thread which cleans up conntrack entries regularly, but not
> > in the timer interrupt context.
>
> Rusty, Jamal and I were talking about this conntrack cleanup yesterday
> (we're currently all at OLS).  The conclusion was that a cleanup kernel thread
> is what we want to have.
[...]
> If I understood correctly, Rusty already has some old patches in this
> area, and he is most likely to put an update of these patches out in the
> next weeks before the netfilter developer workshop.

Maybe the reworked per bucket locking patch is a good start for a
cleanup kernel thread. I have uploaded all my pending patches to
http://www.kfki.hu/~kadlec/sw/netfilter/2.5/:

01_logging.patch	2.4 logging patch ported to 2.5
02_proc.patch		ditto
03_locking.patch	per bucket locking patch with a couple of
			optimizations
04_tcp_window.patch	TCP window tracking patch port
05_dos.patch		protection against a conntrack attack
			by an advisory hotlist

[Harald, the user home pages on kashyyyk are unavailable from outside.]

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: timeout storms make the box unresponsive

[Patrick Schaaf]

Hi Jozsef,

> 03_locking.patch	per bucket locking patch with a couple of
> 			optimizations

some questions, during first pass reading over the patch;

1) regarding the 'ifdef CONFIG_IP_NF_NAT_NEEDED' gymnastics, reading
   hash_conntrack(), am I correct that this is the "non-NAT symmetry"
   optimization I proposed a long time ago, i.e. if both direction's
   tuples are mirrors of each other, hash/chain them only once?
   If yes, I feel that warrants breaking out into a separate patch.

2) assuming I am correct wrt the goal in 1), I object to the seperate
   "sorting" of IP and port in hash_conntrack(). Although it is unlikely,
   consider a situation where between two specific IP addresses,
   two independant connections run from (IPA,portA)<->(IPB,portB)
   and (IPA,portB)<->(IPB,portA). The code in your patch would
   throw both into the same hash chain, without need. Better:

   	if (IPa < IPb) {
		/* fine, do nothing */
	} else if (IPa > IPb) {
		swap IPs _and_ ports
	} else /* IPa == IPb */ if (portA > port B) {
		swap ports (IPs are identical, swap is noop)
	}

3) there is much activity related to managing expectations and
   helper stuff. Is that directly related to the conntrack locking
   changes themselves?  If not, I would also advocate breaking those
   into a separate patch. I never looked closely into the expectation
   stuff, so it is nothing but confusing (to me) to see it in this patch.

In any case, thanks for your work. Feel free to ignore my pleas, if you
and others are content with the aggregated changes.

best regards
  Patrick

Re: timeout storms make the box unresponsive

[Jozsef Kadlecsik]

Hi Patrick,

[...back from holiday]

On Sun, 27 Jul 2003, Patrick Schaaf wrote:

> > 03_locking.patch	per bucket locking patch with a couple of
> > 			optimizations
>
> some questions, during first pass reading over the patch;
>
> 1) regarding the 'ifdef CONFIG_IP_NF_NAT_NEEDED' gymnastics, reading
>    hash_conntrack(), am I correct that this is the "non-NAT symmetry"
>    optimization I proposed a long time ago, i.e. if both direction's
>    tuples are mirrors of each other, hash/chain them only once?
>    If yes, I feel that warrants breaking out into a separate patch.

Yes, that's the implementation of your proposal. I was simply too lazy to
create a separate patch...

> 2) assuming I am correct wrt the goal in 1), I object to the seperate
>    "sorting" of IP and port in hash_conntrack(). Although it is unlikely,
>    consider a situation where between two specific IP addresses,
>    two independant connections run from (IPA,portA)<->(IPB,portB)
>    and (IPA,portB)<->(IPB,portA). The code in your patch would
>    throw both into the same hash chain, without need. Better:
>
>    	if (IPa < IPb) {
> 		/* fine, do nothing */
> 	} else if (IPa > IPb) {
> 		swap IPs _and_ ports
> 	} else /* IPa == IPb */ if (portA > port B) {
> 		swap ports (IPs are identical, swap is noop)
> 	}

Good point, thank you the suggestion!

> 3) there is much activity related to managing expectations and
>    helper stuff. Is that directly related to the conntrack locking
>    changes themselves?  If not, I would also advocate breaking those
>    into a separate patch. I never looked closely into the expectation
>    stuff, so it is nothing but confusing (to me) to see it in this patch.

That'd require a lot of work. I don't think it would worth the effort.

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary