Date: Mon, 11 Mar 2013 17:51:18 +0100 From: Kajetan Staszkiewicz <vegeta@tuxpowered.net> To: Ermal =?utf-8?q?Lu=C3=A7i?= <eri@freebsd.org> 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: <201303111751.18274.vegeta@tuxpowered.net> In-Reply-To: <CAPBZQG0EBCCYTbj_O_St9pfMyOAaW4=noVWxY8zRHLdWaj=_uw@mail.gmail.com> References: <201303081419.17743.vegeta@tuxpowered.net> <201303111605.19518.vegeta@tuxpowered.net> <CAPBZQG0EBCCYTbj_O_St9pfMyOAaW4=noVWxY8zRHLdWaj=_uw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Dnia poniedzia=C5=82ek, 11 marca 2013 o 16:27:33 Ermal Lu=C3=A7i napisa=C5=
=82(a):
> On Mon, Mar 11, 2013 at 4:05 PM, Kajetan Staszkiewicz
> <vegeta@tuxpowered.net
>=20
> > wrote:
> >=20
> > There are some things I find flawed in your patch:
> >=20
> > 1.
> >=20
> > +#if 0
> >=20
> > if (killed > 0)
> > =20
> > pf_purge_expired_src_nodes(1);
> >=20
> > +#endif
> >=20
> > 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=3Ds->nat_src_node=3DNULL =
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 t=
he
> > state
> > table anymore, so we achieve another performance improvement.
>=20
> Well probably just need to clear the rtaddr member of a source node.
rt_addr is a property of pf_state (or did you mean raddr of pf_src_node?), =
I do=20
not know if the rest of pf code allows it to be just cleared, and even if, =
it=20
would only create broken State entries not forwarding data to the server be=
hind=20
the loadbalancer (I *might* want to leave existing connections alive, depen=
ding=20
on usage of further mentioned pfctl's "-c" switch, but I always want to rem=
ove=20
Sources ASAP so no new connections use them). And of course I prefer to kee=
p=20
the default behavior unchanged.
> 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.
This was the original behavior of the code, I only reduced N to reasonable=
=20
values (and therefore time of operation).
> 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).
Yes, this is why I started working on the issue in the first place. My appr=
oach=20
introduces no additional overhead, only redurces the existing one.
> I would rather be keen to mark a src_node state as expired and let the
> purge_thread collect that?
What I observed was that without calling pf_purge_expired_src_nodes (your=20
patch), Sources were still displayed with 0 expiration time and if any clie=
nt=20
connected before they got purged, expiration time was bumped up and the Sou=
rce=20
entry was alive again and never purged anymore. This is definetely not the=
=20
behavior I'd expect.
>=20
> > 2.
> >=20
> > /* Handle state to src_node linkage */
> >=20
> > +#ifndef __FreeBSD__
> >=20
> > if (sn->states !=3D 0) {
> > =20
> > RB_FOREACH(s, pf_state_tree_id,
> >=20
> > #ifdef __FreeBSD__
> >=20
> > &V_tree_id) {
> >=20
> > #else
> >=20
> > &tree_id) {
> >=20
> > #endif
> >=20
> > if (s->src_node =3D=3D sn)
> > =20
> > s->src_node =3D NULL;
> > =20
> > if (s->nat_src_node =3D=3D sn)
> > =20
> > s->nat_src_node =3D NULL;
> > =20
> > }
> > sn->states =3D 0;
> > =20
> > }
> >=20
> > +#endif
> >=20
> > sn->expire =3D 1;
> > killed++;
> >=20
> > This removes a bit too much code, that is zeroing of source's state
> > counter.
> >=20
> > Please find the next version of the patch here:
> > http://vegeta.tuxpowered.net/download/link-states-to-src_node-3.patch
> >=20
> > 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.
>=20
> 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.
Variable for counting terminated states was added to pfioc_src_node_kill, n=
ot=20
to pf_state.
> 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?
Again, the counter is in pfioc_src_node_kill, not pf_src_node.
=2D-=20
| pozdrawiam / greetings | powered by Debian, CentOS and FreeBSD |
| Kajetan Staszkiewicz | jabber,email: vegeta()tuxpowered net |
| Vegeta | www: http://vegeta.tuxpowered.net |
`------------------------^---------------------------------------'
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201303111751.18274.vegeta>