Re: [PATCH 2/4] macvlan: cleanup rx statistics
From: Arnd Bergmann <arnd@arndb.de>
Date: 2009-11-24 09:29:21
Also in:
bridge, lkml
Subsystem:
networking drivers, the rest · Maintainers:
Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
On Tuesday 24 November 2009 08:45:14 Arnd Bergmann wrote:
On Tuesday 24 November 2009 08:15:53 Eric Dumazet wrote:quoted
Arnd Bergmann a écrit : I find following more readable, it probably generates more branches, but avoid dirtying rx_errors if it is in another cache line. if (likely(success)) { rx_stats->rx_packets++; rx_stats->rx_bytes += length; if (multicast) rx_stats->multicast++; } else { rx_stats->rx_errors++; }Given that the structure only has four members and alloc_percpu requests cache aligned data, it is rather likely to be in the same cache line. I'll have a look at what gcc generates on x86-64 for both versions and use the version you suggested unless it looks significantly more expensive.
Ok, that's what I got for trying to be clever. My version did not avoid any branches, just created more code. I'll fold this update into my patches then: --- macvlan: cleanups Use bool instead of int for flags, don't misoptimize rx counters, avoid accessing a NULL skb. Signed-off-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 3db96b9..ff5f0b0 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c@@ -119,19 +119,23 @@ static int macvlan_addr_busy(const struct macvlan_port *port, } static inline void macvlan_count_rx(const struct macvlan_dev *vlan, int length, - int success, int multicast) + bool success, bool multicast) { struct macvlan_rx_stats *rx_stats; rx_stats = per_cpu_ptr(vlan->rx_stats, smp_processor_id()); - rx_stats->rx_packets += success != 0; - rx_stats->rx_bytes += success ? length : 0; - rx_stats->multicast += success && multicast; - rx_stats->rx_errors += !success; + if (likely(success)) { + rx_stats->rx_packets++;; + rx_stats->rx_bytes += length; + if (multicast) + rx_stats->multicast++; + } else { + rx_stats->rx_errors++; + } } static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev, - const struct ethhdr *eth, int local) + const struct ethhdr *eth, bool local) { if (!skb) return NET_RX_DROP;
@@ -173,7 +177,7 @@ static void macvlan_broadcast(struct sk_buff *skb, err = macvlan_broadcast_one(nskb, vlan->dev, eth, mode == MACVLAN_MODE_BRIDGE); macvlan_count_rx(vlan, skb->len + ETH_HLEN, - likely(err == NET_RX_SUCCESS), 1); + err == NET_RX_SUCCESS, 1); } } }
@@ -186,6 +190,7 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb) const struct macvlan_dev *vlan; const struct macvlan_dev *src; struct net_device *dev; + int len; port = rcu_dereference(skb->dev->macvlan_port); if (port == NULL)
@@ -218,8 +223,11 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb) kfree_skb(skb); return NULL; } + len = skb->len + ETH_HLEN; skb = skb_share_check(skb, GFP_ATOMIC); - macvlan_count_rx(vlan, skb->len + ETH_HLEN, likely(skb != NULL), 0); + macvlan_count_rx(vlan, len, skb != NULL, 0); + if (!skb) + return NULL; skb->dev = dev; skb->pkt_type = PACKET_HOST;
@@ -248,7 +256,7 @@ static int macvlan_queue_xmit(struct sk_buff *skb, struct net_device *dev) int length = skb->len + ETH_HLEN; int ret = dev_forward_skb(dest->dev, skb); macvlan_count_rx(dest, length, - likely(ret == NET_RX_SUCCESS), 0); + ret == NET_RX_SUCCESS, 0); return NET_XMIT_SUCCESS; }