From owner-freebsd-pf@FreeBSD.ORG Mon Mar 11 16:51:21 2013 Return-Path: Delivered-To: freebsd-pf@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 8F25C87B for ; Mon, 11 Mar 2013 16:51:21 +0000 (UTC) (envelope-from vegeta@tuxpowered.net) Received: from mail-bk0-x22d.google.com (mail-bk0-x22d.google.com [IPv6:2a00:1450:4008:c01::22d]) by mx1.freebsd.org (Postfix) with ESMTP id 1C4D8F84 for ; Mon, 11 Mar 2013 16:51:20 +0000 (UTC) Received: by mail-bk0-f45.google.com with SMTP id i18so1801592bkv.32 for ; Mon, 11 Mar 2013 09:51:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:from:to:subject:date:user-agent:cc:references :in-reply-to:mime-version:content-type:content-transfer-encoding :message-id:x-gm-message-state; bh=WXjJkv9LY0P3kftCOg/sWVzJR7yEN9eks/TjFAt1hsA=; b=YPXC/ME9LbwZnwC0UtAzJEPSMcnaPIssWaHXIWcWTj0N33sZMjHvU54wVvGdap4/fj rZZB1NiuvOW6hWMw3SAnVaKXod0go3Ynt1CNeaZUrvKlgJahaglPifGRVSOmeCzX6ehZ lec/69GSEHRuMvRltiUeh66AHTxihaFiYvh7rN4XH7LthBFwoinE3QqUrHRkdaOehXen KIXehpwa8u6mhcQHMxEqt89G70P1Jypt7VLrIKlToc8Cf/pGivVeSxkP+5CdIAG0l5U3 hJ6LYfhBpZxnQWU2soMteB82V2X3BpGn8fi7orpnSHPgG66PPa3bBJpFd1x7So3Gv4jM Wxug== X-Received: by 10.205.122.80 with SMTP id gf16mr4973231bkc.130.1363020680024; Mon, 11 Mar 2013 09:51:20 -0700 (PDT) Received: from zvezda.localnet ([212.48.107.10]) by mx.google.com with ESMTPS id v2sm4339726bkw.5.2013.03.11.09.51.19 (version=TLSv1 cipher=RC4-SHA bits=128/128); Mon, 11 Mar 2013 09:51:19 -0700 (PDT) From: Kajetan Staszkiewicz To: Ermal =?utf-8?q?Lu=C3=A7i?= Subject: Re: [patch] Source entries removing is awfully slow. Date: Mon, 11 Mar 2013 17:51:18 +0100 User-Agent: KMail/1.13.5 (Linux/3.6.6-vegeta.1; KDE/4.4.5; x86_64; ; ) References: <201303081419.17743.vegeta@tuxpowered.net> <201303111605.19518.vegeta@tuxpowered.net> In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Message-Id: <201303111751.18274.vegeta@tuxpowered.net> X-Gm-Message-State: ALoCoQmc5QXaIgxmksN3rzTt++CdVJCmQpkybFkJVWmfh28h2G+MeJ5E8Nad0iaL6+6jSaOwzcqD Cc: "freebsd-net@freebsd.org" , "freebsd-pf@freebsd.org" X-BeenThere: freebsd-pf@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "Technical discussion and general questions about packet filter \(pf\)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Mar 2013 16:51:21 -0000 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 > =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 | `------------------------^---------------------------------------'