Date: Wed, 9 Jul 2014 18:07:02 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: "Alexander V. Chernikov" <melifaro@FreeBSD.org> Cc: svn-src-projects@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r268440 - in projects/ipfw: sbin/ipfw sys/netinet sys/netpfil/ipfw Message-ID: <20140709140701.GJ87212@FreeBSD.org> In-Reply-To: <201407082311.s68NBGwb078468@svn.freebsd.org> References: <201407082311.s68NBGwb078468@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140709140701.GJ87212>