Re: PATCH: extra conntrack stats

[Rusty Russell <rusty@rustcorp.com.au>]

In message <1051743903.8213.188.camel@tux.rsn.bth.se> you write:
> On Wed, 2003-04-30 at 13:19, Martin Josefsson wrote:
> - Switch to using hlists for the hashtable

OK.  Please implement hlist_for_each_entry(), though, a-la
list_for_each_entry.

> - Rearrange struct ip_conntrack to be more cachefriendly if possible

Leave 'til last, I think, since that structure will be changing
anyway.  Most important is that next ptr and tuple are at the start
(ie. only one cacheline per hash chain entry).

> - Add prefetching in list-searching

Can be done, but it's a micro-optimization and you'd want to check
carefully that recent gcc's don't do this anyway.

> - Turn protocol_list into an array

Or hardcode the three builtins and use the list for others.

> - Switch to a better hashfunction

Yes!  See comments below on secret hashing...

> - Remove pointless timer-updating

We could revisit this altogether: go for one timer which sweeps the
hash chains and an "expiry" date on each one.  You end up with some
icky deletion issues, but...

> - Rework locking to be finer grained, start with per bucket spinlocks
>   (goal: RCU?)

Well, might as well go straight to RCU for the infrastructure
locking.  

It's a little tricky.  RCU can be used for the read side, but since
writes are not uncommon (I always guesstimated 1 in 10), we still
probably want per-chain locks.  Hmm, actually, since that bloats the
hash, let's start with one lock and see how it goes.

The protection of the conntrack objects themselves should become a
lock per conntrack I think.  We currently use the timer lock as a form
of synchronization: if we have a conntrack.lock we should use that.

> - Remove tcp_lock if using per bucket spinlocks, otherwise move it into
>   the entries

Agreed: use conntrack.lock.

> - Remove pointless rehashing of tuples

Hmmm, does this happen?

> - Rework overload support (early_drop)

OK, you've probably seen my "hash with secret key" scheme.
Unfortunately, it relies on being able to grab the network brlock to
stop all activity while it redoes the hash.  But Dave is removing the
brlock, so this becomes more tricky (ie. readers have to be aware that
there could be two hash tables, ick).

It might still be worth it though (this same trick would allow us to
resize the hash through /proc).  Needs more thought.

> - Avoid as many memory-writes as possible, no need to dirty cachelines if
>   we don't have to

Well, yes, but it's usually secondary after correctness.

> - Eliminate listhelp.h and lockhelp.h by request from hch

Yeah, list.h is more sophisticated now, and we have lock debugging

> - Try to shrink struct ip_conntrack

Ignoring NAT for the moment, and using a 32-bit arch:

	struct nf_conntrack ct_general;
8 bytes: hard to shrink.

	struct ip_conntrack_tuple_hash tuplehash[IP_CT_DIR_MAX];
2 x 28 bytes: we can get rid of one.

	unsigned long status;
8 bytes. Needs to be ulong for bitops.

	struct timer_list timeout;
48 bytes: could be one ulong (time for expiry).

	struct list_head sibling_list;
8 bytes.  Hard to remove without getting tricky...
	
	unsigned int expecting;
4 bytes. Maybe could merge this with upper bits of status, maybe...

	struct ip_conntrack_expect *master;
4 bytes. Needed.

	struct ip_conntrack_helper *helper;
4 bytes.  Needed.

	struct nf_ct_info infos[IP_CT_NUMBER];
7 * 4 bytes.  We could eliminate this by adding an skb field to hold
the state.

	union ip_conntrack_proto proto;
8 bytes, will get bigger with tcp tracking.

	union ip_conntrack_help help;
16 bytes, could use flags in status for ftp's seq_aft_nl_set and cut
to 8 bytes.

So, we're about 192 bytes.  We could get to 80 bytes.

Cheers,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.