On Tuesday 24 November 2009 08:15:53 Eric Dumazet wrote:
> Arnd Bergmann a écrit :
> >
> > +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
> > +{
> > + 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?
> > - 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 <><
_______________________________________________
Bridge mailing list
Bridge@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/bridge
