On Tuesday 24 November 2009 08:15:53 Eric Dumazet wrote:

Arnd Bergmann a écrit :

quoted

 
+static inline void macvlan_count_rx(const struct macvlan_dev *vlan, int length,
+			     int success, int multicast)

success and multicast should be declared as bool

ok

quoted

+{
+	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;
+}
+

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.

Since we're into micro-optimization territory, do you think it should
be marked inline or not?
 

quoted

-	rx_stats = per_cpu_ptr(vlan->rx_stats, smp_processor_id());
 	skb = skb_share_check(skb, GFP_ATOMIC);
-	if (skb == NULL) {
-		rx_stats->rx_errors++;
-		return NULL;
-	}
-
-	rx_stats->rx_bytes += skb->len + ETH_HLEN;
-	rx_stats->rx_packets++;
+	macvlan_count_rx(vlan, skb->len + ETH_HLEN, likely(skb != NULL), 0);

its not _likely_ that skb != NULL, its a fact :)

-> macvlan_count_rx(vlan, skb->len + ETH_HLEN, true, false);

I don't understand. Note how I removed the check for NULL above and
the skb pointer may be the result of a failing skb_clone().

Looking at this again, I actually introduced a bug by calling netif_rx
on a possibly NULL skb, I'll fix that.

Thanks!

	Arnd <><