svn commit: r268440 - in projects/ipfw: sbin/ipfw sys/netinet sys/netpfil/ipfw

[Gleb Smirnoff]

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.