On 19 Feb 2019, at 22:53, Andreas Longwitz wrote:
Kristof Provost wrote:
Because fetching a counter is a rather expansive function we
should use
counter_u64_fetch() in pf_state_expires() only when necessary. A
"rdr
pass" rule should not cause more effort than separate "rdr" and
"pass"
rules. For rules with adaptive timeout values the call of
counter_u64_fetch() should be accepted, but otherwise not.
For a small gain in performance especially for "rdr pass" rules I
suggest something like
--- pf.c.orig 2019-02-18 17:49:22.944751000 +0100
+++ pf.c 2019-02-18 17:55:07.396163000 +0100
@@ -1558,7 +1558,7 @@
if (!timeout)
timeout = V_pf_default_rule.timeout[state->timeout];
start = state->rule.ptr->timeout[PFTM_ADAPTIVE_START];
- if (start) {
+ if (start && state->rule.ptr != &V_pf_default_rule) {
end = state->rule.ptr->timeout[PFTM_ADAPTIVE_END];
states = counter_u64_fetch(state->rule.ptr->states_cur);
} else {
I think that looks correct. Do you have any performance measurements
on
this?
No
Although presumably it only really matters in cases where there’s
no
explicit catch-all rule, so I do wonder if it’s worth it.
Sorry, but I do not understand this argument.
From manpage:
The adaptive timeout values can be defined both globally and for
each rule. When used on a per-rule basis, the values relate to the
number of states created by the rule, otherwise to the total number
of states.
This handling of adaptive timeouts is done in pf_state_expires():
start = state->rule.ptr->timeout[PFTM_ADAPTIVE_START];
if (start) {
end = state->rule.ptr->timeout[PFTM_ADAPTIVE_END];
states = counter_u64_fetch(state->rule.ptr->states_cur);
} else {
start = V_pf_default_rule.timeout[PFTM_ADAPTIVE_START];
end = V_pf_default_rule.timeout[PFTM_ADAPTIVE_END];
states = V_pf_status.states;
}
The following calculation needs three values: start, end and states.
1. Normal rules "pass .." without adaptive setting meaning "start = 0"
runs in the else-section of the code snippet and therefore takes
"start" and "end" from the global default settings and sets
"states"
to pf_status.states (= total number of states).
2. Special rules like
"pass .. keep state (adaptive.start 500 adaptive.end 1000)"
have start != 0, run in the if-section of the code snippet and take
"start" and "end" from the rule and set "states" to the number of
states created by their rule using counter_u64_fetch().
Thats all ok, but there is a third case without special handling in
the
above code snippet:
3. All "rdr/nat pass .." statements use together the pf_default_rule.
Therefore we have "start != 0" in this case and we run the
if-section of the code snippet but we better should run the
else-section in this case and do not fetch the counter of the
pf_default_rule but take the total number of states.
Thats what the patch does.
Thank you, that makes sense. I’d missed the third case.
The patch is in my queue and should get committed soon. Your explanation
makes a great commit message.
Regards,
Kristof
_______________________________________________
freebsd-pf@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-pf
To unsubscribe, send any mail to "freebsd-pf-unsubscr...@freebsd.org"
