Hi!
Auto-loading only the required modules definetly helps a lot for low CPU
power systems like the Soekris net48xx.
We have seen a difference in sirq (Soft IRQ) load of 80% with conntrack
modules versus 12% without any iptables modules when the system is hit
with 50Mbps UDP traffic on the ethernet. Or with iperf UDP it's 30Mbps
vs 36Mbps (all modues/no conntrack modules), but here the iperf process
itself takes a lot of CPU time.
So, for small systems which do not use a firewall or which have a
firewall configuration without conntrack it would be really good to load
the modules on demand...
bruno
On 11/04/2013 10:27 AM, Takayuki Kaiso wrote:
> Thanks for the suggestions for each line, I failed to care for port
> forwarding (redirect) case.
>
> 1. autoload issues
> I basically agree to the simple way you said, "only load them when
> needed for the first time".
> then, "continuous running system, with low cpu power, dynamically change
> firewall configuration"
> will not be helped, but I hope it should be rare.
>
> 2. regarding the redundant lines of state->enable_conntrack = true,
> yes it looks redundant, but masquerade requires conntrack as its
> prerequisite, doesn't it ?
> so I think we need to enables the flag in both cases.
>
> thanks a lot.
> Takayuki Kaiso
>> Hi,
>>
>> in general I do consider this idea but I do not agree with the
>> implementation in your patch.
>>
>> In particular I think that the firewall init script is the wrong place
>> to unload kmods, actually I think a random service should never unload
>> kmods, at most only load them when needed for the first time.
>>
>> We can certainly change the netfilter kmod packages to not autoload on
>> boot anymore, then insmod the required ones from fw3 (similar to how
>> iptables did it).
>>
>> Furthermore the module dependencies should be calculated in the C code
>> directly, I do not like parsing the firewall config for a 2nd time in
>> shell, just to determine needed modules.
>>
>> A couple of additional comments are inline below.
>>
>> On 01.11.2013 06:35, Takayuki Kaiso wrote:
>>> Now all the firewall modules are loaded at boot time regardless of
>>> the user's firewall configuration.
>>> Then, performance overhead is so high for low cpu power boards (Ex.
>>> soekris net4826 with 266MHz)
>>> by this approach and should be better to load modules on demand
>>> base.
>>>
>>> This patch allow us to unload unnecessary modules especially for
>>> conntrack related ones.
>>> It can decrease the overhead for the config which does not enable
>>> NAT, for instance.
>>>
>>> Signed-off-by: Takayuki Kaiso <tka...@thinktube.com>
>>>
>>> ---
>>> package/firewall/files/firewall.init | 44 +++++++++
>>> .../patches/010-remove-unnecessary-chain.patch | 101
>>> +++++++++++++++++++++
>>> 2 files changed, 145 insertions(+)
>>>
>>> diff --git a/package/firewall/files/firewall.init
>>> b/package/firewall/files/firewall.init
>>> index 64e3a8c..6eda74f 100755
>>> --- a/package/firewall/files/firewall.init
>>> +++ b/package/firewall/files/firewall.init
>>> @@ -2,24 +2,68 @@
>>>
>>> START=19
>>>
>>> +CONNTRACK_ZONES=
>>> +CONNTRACK_MODS="nf_nat_irc nf_conntrack_irc nf_nat_ftp
>>> nf_conntrack_ftp ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4
>>> nf_defrag_ipv4
>>> +xt_conntrack"
>> There are more than just those, e.g. in the conntrack-extra packages, a
>> more generic solution is needed here.
>>
>>> +CONNTRACK_MODS_REVERSE=
>>> +
>>> +fw_zone_check_conntrack() {
>>> + config_get name $1 name
>>> + config_get_bool masq $1 masq "0"
>>> + config_get_bool conntrack $1 conntrack "0"
>>> + [ "$conntrack" = "1" -o "$masq" = "1" ] && append CONNTRACK_ZONES
>>> "$name"
>> Port forwards imply conntracking on a zone as well, this is not covered
>> here.
>>
>>> +}
>>> +
>>> +load_conntrack_modules() {
>>> + for i in $CONNTRACK_MODS; do
>>> + CONNTRACK_MODS_REVERSE="$i $CONNTRACK_MODS_REVERSE"
>>> + done
>>> + for i in $CONNTRACK_MODS_REVERSE; do
>>> + insmod $i 2>/dev/null;
>>> + done
>>> +}
>>> +
>>> +unload_conntrack_modules() {
>>> + for i in $CONNTRACK_MODS; do
>>> + rmmod $i 2>/dev/null;
>>> + done
>>> +}
>>> +
>>> +update_firewall_modules() {
>>> + config_load firewall
>>> + config_foreach fw_zone_check_conntrack zone
>>> + if [ -n "$CONNTRACK_ZONES" ]; then
>>> + echo "loading conntrack modules"
>>> + load_conntrack_modules
>>> + else
>>> + echo "Unloading conntrack modules"
>>> + unload_conntrack_modules
>>> + fi
>>> +}
>>> +
>>> boot() {
>>> # Be silent on boot, firewall might be started by hotplug already,
>>> # so don't complain in syslog.
>>> fw3 -q start
>>> + update_firewall_modules
>> Shouldn't that be done before starting the firewall?
>>> }
>>>
>>> start() {
>>> fw3 start
>>> + update_firewall_modules
>> Dito.
>>> }
>>>
>>> stop() {
>>> fw3 flush
>>> + update_firewall_modules
>> After a flush, the complete firewall is empty, one could argue that the
>> principle of least surprise is violated here by *loading* kmods under
>> certain circumstances.
>>> }
>>>
>>> restart() {
>>> fw3 restart
>>> + update_firewall_modules
>>> }
>>>
>>> reload() {
>>> fw3 reload
>>> + update_firewall_modules
>> Reload does not clear the entire ruleset, it simply rebuilds internal
>> chains. There is a high probability of live rules still relying on
>> conntrack modules.
>>> }
>>> diff --git
>>> a/package/firewall/patches/010-remove-unnecessary-chain.patch
>>> b/package/firewall/patches/010-remove-unnecessary-chain.patch
>>> new file mode 100644
>>> index 0000000..a2991e9
>>> --- /dev/null
>>> +++ b/package/firewall/patches/010-remove-unnecessary-chain.patch
>>> @@ -0,0 +1,101 @@
>>> +--- a/defaults.c
>>> ++++ b/defaults.c
>>> +@@ -138,6 +138,9 @@ fw3_print_default_chains(struct fw3_ipt_
>>> + if (handle->family == FW3_FAMILY_V6 && defs->disable_ipv6)
>>> + return;
>>> +
>>> ++ if ((handle->table == FW3_TABLE_NAT && !state->enable_nat_table))
>>> ++ return;
>>> ++
>>> + if (handle->table == FW3_TABLE_FILTER)
>>> + {
>>> + fw3_ipt_set_policy(handle, "INPUT", policy(defs->policy_input));
>>> +@@ -213,6 +216,9 @@ fw3_print_default_head_rules(struct fw3_
>>> + { 0, NULL },
>>> + };
>>> +
>>> ++ if ((handle->table == FW3_TABLE_NAT && !state->enable_nat_table))
>>> ++ return;
>>> ++
>>> + for (tr = rules; tr->chain; tr++)
>>> + {
>>> + if (tr->table != handle->table)
>>> +@@ -250,10 +256,13 @@ fw3_print_default_head_rules(struct fw3_
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(chains); i += 2)
>>> + {
>>> +- r = fw3_ipt_rule_new(handle);
>>> +- fw3_ipt_rule_extra(r, "-m conntrack --ctstate RELATED,ESTABLISHED");
>>> +- fw3_ipt_rule_target(r, "ACCEPT");
>>> +- fw3_ipt_rule_append(r, chains[i]);
>>> ++ if (state->enable_conntrack)
>>> ++ {
>>> ++ r = fw3_ipt_rule_new(handle);
>>> ++ fw3_ipt_rule_extra(r, "-m conntrack --ctstate RELATED,ESTABLISHED");
>>> ++ fw3_ipt_rule_target(r, "ACCEPT");
>>> ++ fw3_ipt_rule_append(r, chains[i]);
>>> ++ }
>>> +
>>> + if (defs->drop_invalid)
>>> + {
>>> +--- a/options.h
>>> ++++ b/options.h
>>> +@@ -461,6 +461,8 @@ struct fw3_state
>>> +
>>> + bool disable_ipsets;
>>> + bool statefile;
>>> ++ bool enable_nat_table;
>>> ++ bool enable_conntrack;
>>> + };
>>> +
>>> + struct fw3_chain_spec {
>>> +--- a/zones.c
>>> ++++ b/zones.c
>>> +@@ -213,6 +213,13 @@ fw3_load_zones(struct fw3_state *state,
>>> + {
>>> + setbit(zone->flags[0], FW3_FLAG_SNAT);
>>> + zone->conntrack = true;
>>> ++ state->enable_nat_table = true;
>>> ++ state->enable_conntrack = true;
>> This...
>>
>>> ++ }
>>> ++
>>> ++ if (zone->conntrack)
>>> ++ {
>>> ++ state->enable_conntrack = true;
>> ... and this is redundant, only the 2nd enable_conntrack assignment
>> seems to be needed.
>>
>>> + }
>>> +
>>> + if (zone->custom_chains)
>>> +@@ -256,6 +263,9 @@ print_zone_chain(struct fw3_ipt_handle *
>>> + if (!fw3_is_family(zone, handle->family))
>>> + return;
>>> +
>>> ++ if ((handle->table == FW3_TABLE_NAT && !state->enable_nat_table))
>>> ++ return;
>>> ++
>>> + info(" * Zone '%s'", zone->name);
>>> +
>>> + set(zone->flags, handle->family, handle->table);
>>> +@@ -332,6 +342,9 @@ print_interface_rule(struct fw3_ipt_hand
>>> + "forward",
>>> + };
>>> +
>>> ++ if ((handle->table == FW3_TABLE_NAT && !state->enable_nat_table))
>>> ++ return;
>>> ++
>>> + #define jump_target(t) \
>>> + ((t == FW3_FLAG_REJECT) ? "reject" : fw3_flag_names[t])
>>> +
>>> +--- a/main.c
>>> ++++ b/main.c
>>> +@@ -50,6 +50,11 @@ build_state(bool runtime)
>>> + error("Out of memory");
>>> +
>>> + memset(state, 0, sizeof(*state));
>>> ++
>>> ++ /* set default status */
>>> ++ state->enable_nat_table = false;
>>> ++ state->enable_conntrack = false;
>> This is redundant and defaults to false anyway due to the memset before.
>>
>>> ++
>>> + state->uci = uci_alloc_context();
>>> +
>>> + if (!state->uci)
>> You also need to enable support for the nat table if at least one
>> redirect is referencing the zone.
>>
>>
>> Regards,
>> Jow
>>
>>
>>
>> _______________________________________________
>> openwrt-devel mailing list
>> openwrt-devel@lists.openwrt.org
>> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
>
>
_______________________________________________
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
