Skip to contentSkip to conversation

vlan: Precise RX stats accounting

PATCH Applied Eric Dumazet 2 participants 6 messages Nov 16, 2009 – Nov 18, 2009

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

PATCH 2/2 vlan: Precise RX stats accounting

With multi queue devices, its possible that several cpu call vlan RX routines simultaneously for the same vlan device.

We update RX stats counter without any locking, so we can get slightly wrong counters.

One possible fix is to use percpu counters, to get precise accounting and also get guarantee of no cache line ping pongs between cpus.

In case dynamic percpu allocation fails, we fallback to previous mode (sharing dev->stats counter)

Signed-off-by Eric Dumazet <[email protected]>
---
 net/8021q/vlan.c      |    1 
 net/8021q/vlan.h      |   10 +++++++
 net/8021q/vlan_core.c |   19 ++++++++++----
 net/8021q/vlan_dev.c  |   53 +++++++++++++++++++++++++++++++++++-----
 4 files changed, 72 insertions(+), 11 deletions(-)
net/8021q/vlan.c +1 -0
index 39f8d01..db6a9bc 100644
@@ -179,6 +179,7 @@ void unregister_vlan_dev(struct net_device *dev, struct list_head *head)
 		/* Free the group, after all cpu's are done. */
 		call_rcu(&grp->rcu, vlan_rcu_free);
 	}
+	free_percpu(vlan->vlan_rx_stats);
 
 	/* Get rid of the vlan's reference to real_dev */
 	dev_put(real_dev);
net/8021q/vlan.h +10 -0
index 68f9290..a211a55 100644
@@ -16,6 +16,15 @@ struct vlan_priority_tci_mapping {
 	struct vlan_priority_tci_mapping	*next;
 };
 
+
+/* percpu rx stats */
+struct vlan_rx_stats {
+	unsigned long rx_packets;
+	unsigned long rx_bytes;
+	unsigned long multicast;
+	unsigned long rx_errors;
+};
+
 /**
  *	struct vlan_dev_info - VLAN private device data
  *	@nr_ingress_mappings: number of ingress priority mappings

@@ -45,6 +54,7 @@ struct vlan_dev_info {
 	struct proc_dir_entry			*dent;
 	unsigned long				cnt_inc_headroom_on_tx;
 	unsigned long				cnt_encap_on_xmit;
+	struct vlan_rx_stats			*vlan_rx_stats;
 };
 
 static inline struct vlan_dev_info *vlan_dev_info(const struct net_device *dev)
net/8021q/vlan_core.c +14 -5
index 971d375..9b97ffb 100644
@@ -31,7 +31,7 @@ EXPORT_SYMBOL(__vlan_hwaccel_rx);
 int vlan_hwaccel_do_receive(struct sk_buff *skb)
 {
 	struct net_device *dev = skb->dev;
-	struct net_device_stats *stats;
+	struct vlan_rx_stats     *rx_stats = NULL;
 
 	skb->dev = vlan_dev_info(dev)->real_dev;
 	netif_nit_deliver(skb);

@@ -40,15 +40,24 @@ int vlan_hwaccel_do_receive(struct sk_buff *skb)
 	skb->priority = vlan_get_ingress_priority(dev, skb->vlan_tci);
 	skb->vlan_tci = 0;
 
-	stats = &dev->stats;
-	stats->rx_packets++;
-	stats->rx_bytes += skb->len;
+	if (likely(vlan_dev_info(dev)->vlan_rx_stats)) {
+		rx_stats = per_cpu_ptr(vlan_dev_info(dev)->vlan_rx_stats,
+				       smp_processor_id());
 
+		rx_stats->rx_packets++;
+		rx_stats->rx_bytes += skb->len;
+	} else {
+		dev->stats.rx_packets++;
+		dev->stats.rx_bytes += skb->len;
+	}
 	switch (skb->pkt_type) {
 	case PACKET_BROADCAST:
 		break;
 	case PACKET_MULTICAST:
-		stats->multicast++;
+		if (likely(rx_stats))
+			rx_stats->multicast++;
+		else
+			dev->stats.multicast++;
 		break;
 	case PACKET_OTHERHOST:
 		/* Our lower layer thinks this is not local, let's make sure.
net/8021q/vlan_dev.c +47 -6

100 lines not on this page · raw message

maintainer
#1 lore raw

From: Eric Dumazet <[email protected]>
Date: Mon, 16 Nov 2009 14:37:19 +0100

Quoted 2 lines

In case dynamic percpu allocation fails, we fallback to previous mode (sharing dev->stats counter)

Eric, please make allocation failure cause the vlan_setup() to fail and propagate -EFAULT back to the caller.

Thanks.

author
#2 lore raw

PATCH 1/1 vlan: Fix register_vlan_dev() error path

Quoted 2 lines · David Miller

Eric, please make allocation failure cause the vlan_setup() to fail and propagate -EFAULT back to the caller.

Sure ! (You meant -ENOMEM probably...)

While testing my new patch in OOM situations, I found following bug in vlan code, that gave me crashes or infinite loops.

[PATCH] vlan: Fix register_vlan_dev() error path

In case register_netdevice() returns an error, and a new vlan_group was allocated and inserted in vlan_group_hash[] we call vlan_group_free() without deleting group from hash table. Future lookups can give infinite loops or crashes.

We must delete the vlan_group using RCU safe procedure.

Signed-off-by Eric Dumazet <[email protected]>
---
 net/8021q/vlan.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)
net/8021q/vlan.c +5 -2
index 8836575..a29c5ab 100644
@@ -281,8 +281,11 @@ out_uninit_applicant:
 	if (ngrp)
 		vlan_gvrp_uninit_applicant(real_dev);
 out_free_group:
-	if (ngrp)
-		vlan_group_free(ngrp);
+	if (ngrp) {
+		hlist_del_rcu(&ngrp->hlist);
+		/* Free the group, after all cpu's are done. */
+		call_rcu(&ngrp->rcu, vlan_rcu_free);
+	}
 	return err;
 }
maintainer
#3 lore raw

Re: [PATCH] vlan: Fix register_vlan_dev() error path

From: Eric Dumazet <[email protected]>
Date: Tue, 17 Nov 2009 15:41:26 +0100

Quoted 6 lines
Quoted 2 lines · David Miller

Eric, please make allocation failure cause the vlan_setup() to fail and propagate -EFAULT back to the caller.

Sure ! (You meant -ENOMEM probably...)

Indeed.

Quoted 9 lines

[PATCH] vlan: Fix register_vlan_dev() error path

In case register_netdevice() returns an error, and a new vlan_group was allocated and inserted in vlan_group_hash[] we call vlan_group_free() without deleting group from hash table. Future lookups can give infinite loops or crashes.

We must delete the vlan_group using RCU safe procedure.

Signed-off-by Eric Dumazet <[email protected]>

Good catch, applied to net-2.6 and queued up for -stable.

author
#4 lore raw
In reply to David Miller
Quoted 4 lines · David Miller

Eric, please make allocation failure cause the vlan_setup() to fail and propagate -EFAULT back to the caller.

Given that vlan_setup(struct net_device *) is void and cannot return error status, unless using a global variable (ugly), or changing all setup() prototypes (oh well...)

One possibility is to perform the allocation in vlan_dev_init() (called from register_netdevice()), and freeing it in vlan_dev_uninit().

Thanks

[PATCH net-next-2.6 take2] vlan: Precise RX stats accounting

With multi queue devices, its possible that several cpus call vlan RX routines simultaneously for the same vlan device.

We update RX stats counter without any locking, so we can get slightly wrong counters.

One possible fix is to use percpu counters, to get precise accounting and also get guarantee of no cache line ping pongs between cpus.

Note: this adds 16 bytes (32 bytes on 64bit arches) of percpu data per vlan device.

Signed-off-by Eric Dumazet <[email protected]>
---
 net/8021q/vlan.h      |   17 ++++++++++++++
 net/8021q/vlan_core.c |   12 +++++-----
 net/8021q/vlan_dev.c  |   47 ++++++++++++++++++++++++++++++++++------
 3 files changed, 65 insertions(+), 11 deletions(-)
net/8021q/vlan.h +17 -0
index 68f9290..5685296 100644
@@ -16,6 +16,21 @@ struct vlan_priority_tci_mapping {
 	struct vlan_priority_tci_mapping	*next;
 };
 
+
+/**
+ *	struct vlan_rx_stats - VLAN percpu rx stats
+ *	@rx_packets: number of received packets
+ *	@rx_bytes: number of received bytes
+ *	@multicast: number of received multicast packets
+ *	@rx_errors: number of errors
+ */
+struct vlan_rx_stats {
+	unsigned long rx_packets;
+	unsigned long rx_bytes;
+	unsigned long multicast;
+	unsigned long rx_errors;
+};
+
 /**
  *	struct vlan_dev_info - VLAN private device data
  *	@nr_ingress_mappings: number of ingress priority mappings

@@ -29,6 +44,7 @@ struct vlan_priority_tci_mapping {
  *	@dent: proc dir entry
  *	@cnt_inc_headroom_on_tx: statistic - number of skb expansions on TX
  *	@cnt_encap_on_xmit: statistic - number of skb encapsulations on TX
+ *	@vlan_rx_stats: ptr to percpu rx stats
  */
 struct vlan_dev_info {
 	unsigned int				nr_ingress_mappings;

@@ -45,6 +61,7 @@ struct vlan_dev_info {
 	struct proc_dir_entry			*dent;
 	unsigned long				cnt_inc_headroom_on_tx;
 	unsigned long				cnt_encap_on_xmit;
+	struct vlan_rx_stats			*vlan_rx_stats;
 };
 
 static inline struct vlan_dev_info *vlan_dev_info(const struct net_device *dev)
net/8021q/vlan_core.c +7 -5
index 971d375..e75a2f3 100644
@@ -31,7 +31,7 @@ EXPORT_SYMBOL(__vlan_hwaccel_rx);
 int vlan_hwaccel_do_receive(struct sk_buff *skb)
 {
 	struct net_device *dev = skb->dev;
-	struct net_device_stats *stats;
+	struct vlan_rx_stats     *rx_stats;
 
 	skb->dev = vlan_dev_info(dev)->real_dev;
 	netif_nit_deliver(skb);

@@ -40,15 +40,17 @@ int vlan_hwaccel_do_receive(struct sk_buff *skb)
 	skb->priority = vlan_get_ingress_priority(dev, skb->vlan_tci);
 	skb->vlan_tci = 0;
 
-	stats = &dev->stats;
-	stats->rx_packets++;
-	stats->rx_bytes += skb->len;
+	rx_stats = per_cpu_ptr(vlan_dev_info(dev)->vlan_rx_stats,
+			       smp_processor_id());
+
+	rx_stats->rx_packets++;
+	rx_stats->rx_bytes += skb->len;
 
 	switch (skb->pkt_type) {
 	case PACKET_BROADCAST:
 		break;
 	case PACKET_MULTICAST:
-		stats->multicast++;
+		rx_stats->multicast++;
 		break;
 	case PACKET_OTHERHOST:
 		/* Our lower layer thinks this is not local, let's make sure.
net/8021q/vlan_dev.c +41 -6

101 lines not on this page · raw message

maintainer Applied
#5 lore raw

From: Eric Dumazet <[email protected]>
Date: Tue, 17 Nov 2009 15:53:09 +0100

Quoted 6 lines

Given that vlan_setup(struct net_device *) is void and cannot return error status, unless using a global variable (ugly), or changing all setup() prototypes (oh well...)

One possibility is to perform the allocation in vlan_dev_init() (called from register_netdevice()), and freeing it in vlan_dev_uninit().

That looks fine.

Quoted 1 line

[PATCH net-next-2.6 take2] vlan: Precise RX stats accounting

...

Quoted 1 line
Signed-off-by Eric Dumazet <[email protected]>

Applied.