From owner-svn-src-projects@FreeBSD.ORG Wed Jul 9 14:07:11 2014 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id E6BDCD54; Wed, 9 Jul 2014 14:07:11 +0000 (UTC) Received: from cell.glebius.int.ru (glebius.int.ru [81.19.69.10]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "cell.glebius.int.ru", Issuer "cell.glebius.int.ru" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 5282B219D; Wed, 9 Jul 2014 14:07:10 +0000 (UTC) Received: from cell.glebius.int.ru (localhost [127.0.0.1]) by cell.glebius.int.ru (8.14.9/8.14.9) with ESMTP id s69E72BH015328 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Wed, 9 Jul 2014 18:07:02 +0400 (MSK) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebius.int.ru (8.14.9/8.14.9/Submit) id s69E72r2015327; Wed, 9 Jul 2014 18:07:02 +0400 (MSK) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebius.int.ru: glebius set sender to glebius@FreeBSD.org using -f Date: Wed, 9 Jul 2014 18:07:02 +0400 From: Gleb Smirnoff To: "Alexander V. Chernikov" Subject: Re: svn commit: r268440 - in projects/ipfw: sbin/ipfw sys/netinet sys/netpfil/ipfw Message-ID: <20140709140701.GJ87212@FreeBSD.org> References: <201407082311.s68NBGwb078468@svn.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201407082311.s68NBGwb078468@svn.freebsd.org> User-Agent: Mutt/1.5.23 (2014-03-12) Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Jul 2014 14:07:12 -0000 On Tue, Jul 08, 2014 at 11:11:16PM +0000, Alexander V. Chernikov wrote: A> * Switch kernel to use per-cpu counters for rules. ... A> ============================================================================== A> --- projects/ipfw/sys/netpfil/ipfw/ip_fw_private.h Tue Jul 8 23:07:09 2014 (r268439) A> +++ projects/ipfw/sys/netpfil/ipfw/ip_fw_private.h Tue Jul 8 23:11:15 2014 (r268440) A> @@ -220,6 +220,44 @@ VNET_DECLARE(unsigned int, fw_tables_set A> A> struct tables_config; A> A> +#ifdef _KERNEL A> +typedef struct ip_fw_cntr { A> + uint64_t pcnt; /* Packet counter */ A> + uint64_t bcnt; /* Byte counter */ A> + uint64_t timestamp; /* tv_sec of last match */ A> +} ip_fw_cntr; So, you decided to pack three counters into one structure instead of using standard counter_u64_alloc() three times. And you create a separate UMA_ZONE_PCPU zone for that. I already expressed concerns about this plan, but if you wish so... let it be. But further code gets uglier: A> +struct ip_fw { A> + uint16_t act_ofs; /* offset of action in 32-bit units */ A> + uint16_t cmd_len; /* # of 32-bit words in cmd */ A> + uint16_t rulenum; /* rule number */ A> + uint8_t set; /* rule set (0..31) */ A> + uint8_t flags; /* currently unused */ A> + counter_u64_t cntr; /* Pointer to rule counters */ A> + uint32_t timestamp; /* tv_sec of last match */ A> + uint32_t id; /* rule id */ A> + struct ip_fw *x_next; /* linked list of rules */ A> + struct ip_fw *next_rule; /* ptr to next [skipto] rule */ A> + A> + ipfw_insn cmd[1]; /* storage for commands */ A> +}; Here we got 'counter_u64_t cntr' that actually points not to a counter(9) allocation, but to 'struct ip_fw_cntr' from UMA_ZONE_PCPU zone. A> +#define IPFW_INC_RULE_COUNTER(_cntr, _bytes) do { \ A> + counter_u64_add((_cntr)->cntr, 1); \ A> + counter_u64_add((_cntr)->cntr + 1, _bytes); \ A> + if ((_cntr)->timestamp != time_uptime) \ A> + (_cntr)->timestamp = time_uptime; \ A> + } while (0) And you update second counter in the structure using (_cntr)->cntr + 1 that relies on internal structure alignment. Regarding timestamp: doing comparison and then assignment on a percpu memory isn't safe against scheduler preemption. I suppose the race is harmless here. Why didn't you made field in the 'struct ip_fw' of type 'struct ip_fw_cntr *'? Then you could use zpcpu_get() to access private to this CPU ip_fw_cntr and use it. The code would be free from type mismatches and structure alignment requirements. -- Totus tuus, Glebius.