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. Regards Andreas _______________________________________________ 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"