Patrick McHardy wrote: > Holger Eitzenberger wrote: >> are you talking about this particular patch or the end result after >> applying all of the NFCT patches? >> >> Also note that IMO this particular patch turned a non-working NFCT >> solution into some better-working solution. I just suggest to >> comment on the final NFCT code and not on this one. Thanks. > > I agree, this code has bitrotten for too long, I'm happy about > any progress. Any objections should be clearly stated please. I know, this code need some spins but there are several stuff that it's better if we fix it now than later: * CT_EVENTS is a duplicated flags, already exists NFCT_ALL_CT_GROUPS * This patch arbitrarily disables loopback logging, this must be an option * Default hashtable size reduced to 512, why? * nfnl_recv_msgs looks like a duplicated of nfnl_catch (or nfct_catch). nfct_msg_type is not required if you use the libnetfilter_conntrack API properly. do_nfct_msg should use the libnetfilter_conntrack framework which would simplify it. * This patch checks if every conntrack exists in the kernel every N seconds to handle overruns. Instead, why doesn't it wait for ENOBUFS in the recv buffer and, then try to resync to kernel? * Where is the NLMSG_OVERRUN flag used in the netlink code? * ct_hash_find_seq is O(n). Overruns sometimes happen because the CPU reaches 100% consumption, so if it can't backoff, this function won't help that much in those cases. I like that this patch kills the preallocation code which made this thing more complex. Unfortunately it is mixed with many other things. Still the netlink handling need some spins IMO. An observation, the asynchronous nature of the ulogd timers keep this hard since the timer callback can be called while accessing whatever section of code. I think that the way to go is to use select and implement time-slicing. -- "Los honestos son inadaptados sociales" -- Les Luthiers - To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
