Date: Mon, 11 Mar 2013 16:27:33 +0100 From: =?ISO-8859-1?Q?Ermal_Lu=E7i?= <eri@freebsd.org> To: Kajetan Staszkiewicz <vegeta@tuxpowered.net> Cc: "freebsd-net@freebsd.org" <freebsd-net@freebsd.org>, "freebsd-pf@freebsd.org" <freebsd-pf@freebsd.org> Subject: Re: [patch] Source entries removing is awfully slow. Message-ID: <CAPBZQG0EBCCYTbj_O_St9pfMyOAaW4=noVWxY8zRHLdWaj=_uw@mail.gmail.com> In-Reply-To: <201303111605.19518.vegeta@tuxpowered.net> References: <201303081419.17743.vegeta@tuxpowered.net> <201303091437.51945.vegeta@tuxpowered.net> <CAPBZQG0EyUb=MZFfFzesxQvA38CPBubjd7izt3OHyqpbMOMarA@mail.gmail.com> <201303111605.19518.vegeta@tuxpowered.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Mar 11, 2013 at 4:05 PM, Kajetan Staszkiewicz <vegeta@tuxpowered.net
> wrote:
> There are some things I find flawed in your patch:
>
> 1.
>
> +#if 0
> if (killed > 0)
> pf_purge_expired_src_nodes(1);
> +#endif
>
> This means that after using `pfctl -K` the src nodes are still around until
> purged and any new states created will still use them and bump their expire
> timer. This also changes behavior from DIOCCLRSRCNODES, which also
> performs the
> purge immediately. You also moved s->src_node=s->nat_src_node=NULL code to
> inside of pf_purge_expired_src_nodes, therefore I believe it should be
> called
> immediately. If detaching state from source is done in
> pf_purge_expired_src_nodes, DIOCCLRSRCNODES does not have to traverse the
> state
> table anymore, so we achieve another performance improvement.
>
Well probably just need to clear the rtaddr member of a source node.
Its an optimization to keep the source node there until next purge since
you would avoid a memory allocation.
It seems to me a better behaviour rather than having to loop N amount of
states during an ioctl which is supposed to be fast.
You need to take into consideration that an ioctl freezes the whole
operation of network in this case because you are modifying
the core of pf(4).
I would rather be keen to mark a src_node state as expired and let the
purge_thread collect that?
>
> 2.
> /* Handle state to src_node linkage */
> +#ifndef __FreeBSD__
> if (sn->states != 0) {
> RB_FOREACH(s, pf_state_tree_id,
> #ifdef __FreeBSD__
> &V_tree_id) {
> #else
> &tree_id) {
> #endif
> if (s->src_node == sn)
> s->src_node = NULL;
> if (s->nat_src_node == sn)
> s->nat_src_node = NULL;
> }
> sn->states = 0;
> }
> +#endif
> sn->expire = 1;
> killed++;
>
> This removes a bit too much code, that is zeroing of source's state
> counter.
>
> Please find the next version of the patch here:
> http://vegeta.tuxpowered.net/download/link-states-to-src_node-3.patch
>
> This one also takes care of removing states linked to found sources if
> pfctl is
> given extra -c parameter (that can stand for "clear", I could not find any
> other free pfctl parameter better matching). Thanks to this parameter, the
> default behavior is not changed.
>
>
Its ok for the extra -c, there is even -S free just to be consistent with
the flush and vieweing options probably better use that?!
The addition of 2 extra members to pf_state structure i am reluctant a bit
since its just for reporting the number of states killed
and it really is not a useful information compared to the increased size of
the structure that consumes RAM memory.
Also pf_src_node structure if the 2 options for display are not added can
use the member that is used to report
the number of states killed to request killing linked states.
What do you think?
> --
> | pozdrawiam / greetings | powered by Debian, CentOS and FreeBSD |
> | Kajetan Staszkiewicz | jabber,email: vegeta()tuxpowered net |
> | Vegeta | www: http://vegeta.tuxpowered.net |
> `------------------------^---------------------------------------'
>
--
Ermal
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPBZQG0EBCCYTbj_O_St9pfMyOAaW4=noVWxY8zRHLdWaj=_uw>