From owner-freebsd-pf@FreeBSD.ORG Mon Mar 11 15:27:36 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 A9DC7A5D; Mon, 11 Mar 2013 15:27:36 +0000 (UTC) (envelope-from ermal.luci@gmail.com) Received: from mail-qe0-f51.google.com (mail-qe0-f51.google.com [209.85.128.51]) by mx1.freebsd.org (Postfix) with ESMTP id 468D4A47; Mon, 11 Mar 2013 15:27:35 +0000 (UTC) Received: by mail-qe0-f51.google.com with SMTP id nd7so2325310qeb.24 for ; Mon, 11 Mar 2013 08:27:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:x-received:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=mJaK4Fg2x2PZ9Tq04oPGZOgMA3o+ot3/feS5TJ+sB0M=; b=EFOvUgDYK27zjJJHQhsKNuvEYDY4bUymODKafmtN01FlvtQnlNK+LogfbJBOhe61Ic z6rUy708j0aWy+LLpF6FZLTazgDGqVFtHAx5wKI7cQgqxTi2uZb2bDuL0PiK4qB7d4+H aUKQ/HmrUmv4coS+/n2hwVLqzbENWNRI1Kgczi/qT6qcaqejtjgzZpmZTcdGJZwnSg/e C3/GEAxoWdlVn/EWL2g6MWw/+/pv0AM72luqvtWB4TOFE9uVdUKJdP+nhTu1wh3tVzqf WuZquHcAYIZ5caecS0t7DAzgHCjAYaATwvya/FUmAn7UC6vAhhS7QKup4ZI/FTDdexhD 2PcA== MIME-Version: 1.0 X-Received: by 10.229.69.24 with SMTP id x24mr4030045qci.16.1363015653965; Mon, 11 Mar 2013 08:27:33 -0700 (PDT) Sender: ermal.luci@gmail.com Received: by 10.49.27.197 with HTTP; Mon, 11 Mar 2013 08:27:33 -0700 (PDT) In-Reply-To: <201303111605.19518.vegeta@tuxpowered.net> References: <201303081419.17743.vegeta@tuxpowered.net> <201303091437.51945.vegeta@tuxpowered.net> <201303111605.19518.vegeta@tuxpowered.net> Date: Mon, 11 Mar 2013 16:27:33 +0100 X-Google-Sender-Auth: u_ysQLfDXzv2yicH4zsprTPBp9c Message-ID: Subject: Re: [patch] Source entries removing is awfully slow. From: =?ISO-8859-1?Q?Ermal_Lu=E7i?= To: Kajetan Staszkiewicz Content-Type: text/plain; charset=ISO-8859-1 X-Content-Filtered-By: Mailman/MimeDel 2.1.14 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 15:27:36 -0000 On Mon, Mar 11, 2013 at 4:05 PM, Kajetan Staszkiewicz 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