Mailing List Archive: Linux: Kernel

[PATCH 2/4] macvlan: cleanup rx statistics

Index | Next | Previous | View Threaded

arnd at arndb

Nov 23, 2009, 4:56 PM

Post #1 of 5 (141 views)
Permalink

[PATCH 2/4] macvlan: cleanup rx statistics

We have very similar code for rx statistics in
two places in the macvlan driver, with a third
one being added in the next patch.

Consolidate them into one function to improve
overall readability of the driver.

Signed-off-by: Arnd Bergmann <arnd [at] arndb>
---
drivers/net/macvlan.c | 63 +++++++++++++++++++++++++-----------------------
1 files changed, 33 insertions(+), 30 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index ae2b5c7..a0dea23 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -116,42 +116,53 @@ static int macvlan_addr_busy(const struct macvlan_port *port,
return 0;
}

+static inline void macvlan_count_rx(const struct macvlan_dev *vlan, int length,
+ int success, int 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;
+}
+
+static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,
+ const struct ethhdr *eth)
+{
+ if (!skb)
+ return NET_RX_DROP;
+
+ skb->dev = dev;
+ if (!compare_ether_addr_64bits(eth->h_dest,
+ dev->broadcast))
+ skb->pkt_type = PACKET_BROADCAST;
+ else
+ skb->pkt_type = PACKET_MULTICAST;
+
+ return netif_rx(skb);
+}
+
static void macvlan_broadcast(struct sk_buff *skb,
const struct macvlan_port *port)
{
const struct ethhdr *eth = eth_hdr(skb);
const struct macvlan_dev *vlan;
struct hlist_node *n;
- struct net_device *dev;
struct sk_buff *nskb;
unsigned int i;
- struct macvlan_rx_stats *rx_stats;
+ int err;

if (skb->protocol == htons(ETH_P_PAUSE))
return;

for (i = 0; i < MACVLAN_HASH_SIZE; i++) {
hlist_for_each_entry_rcu(vlan, n, &port->vlan_hash[i], hlist) {
- dev = vlan->dev;
- rx_stats = per_cpu_ptr(vlan->rx_stats, smp_processor_id());
-
nskb = skb_clone(skb, GFP_ATOMIC);
- if (nskb == NULL) {
- rx_stats->rx_errors++;
- continue;
- }
-
- rx_stats->rx_bytes += skb->len + ETH_HLEN;
- rx_stats->rx_packets++;
- rx_stats->multicast++;
-
- nskb->dev = dev;
- if (!compare_ether_addr_64bits(eth->h_dest, dev->broadcast))
- nskb->pkt_type = PACKET_BROADCAST;
- else
- nskb->pkt_type = PACKET_MULTICAST;
-
- netif_rx(nskb);
+ err = macvlan_broadcast_one(nskb, vlan->dev, eth);
+ macvlan_count_rx(vlan, skb->len + ETH_HLEN,
+ likely(err == NET_RX_SUCCESS), 1);
}
}
}
@@ -163,7 +174,6 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
const struct macvlan_port *port;
const struct macvlan_dev *vlan;
struct net_device *dev;
- struct macvlan_rx_stats *rx_stats;

port = rcu_dereference(skb->dev->macvlan_port);
if (port == NULL)
@@ -183,15 +193,8 @@ static struct sk_buff *macvlan_handle_frame(struct sk_buff *skb)
kfree_skb(skb);
return NULL;
}
- 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);

skb->dev = dev;
skb->pkt_type = PACKET_HOST;
--
1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

eric.dumazet at gmail

Nov 24, 2009, 12:15 AM

Post #2 of 5 (135 views)
Permalink

Re: [PATCH 2/4] macvlan: cleanup rx statistics [In reply to]

Arnd Bergmann a écrit :
> We have very similar code for rx statistics in
> two places in the macvlan driver, with a third
> one being added in the next patch.
>
> Consolidate them into one function to improve
> overall readability of the driver.
>
> Signed-off-by: Arnd Bergmann <arnd [at] arndb>
> ---
> drivers/net/macvlan.c | 63 +++++++++++++++++++++++++-----------------------
> 1 files changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index ae2b5c7..a0dea23 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -116,42 +116,53 @@ static int macvlan_addr_busy(const struct macvlan_port *port,
> return 0;
> }
>
> +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

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

> - 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);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

arnd at arndb

Nov 24, 2009, 12:45 AM

Post #3 of 5 (138 views)
Permalink

Re: [PATCH 2/4] macvlan: cleanup rx statistics [In reply to]

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 <><
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

arnd at arndb

Nov 24, 2009, 1:28 AM

Post #4 of 5 (131 views)
Permalink

Re: [PATCH 2/4] macvlan: cleanup rx statistics [In reply to]

On Tuesday 24 November 2009 08:45:14 Arnd Bergmann wrote:
> On Tuesday 24 November 2009 08:15:53 Eric Dumazet wrote:
> > 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 [at] arndb>

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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

kaber at trash

Nov 24, 2009, 2:41 AM

Post #5 of 5 (136 views)
Permalink

Re: [PATCH 2/4] macvlan: cleanup rx statistics [In reply to]

Arnd Bergmann wrote:
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index ae2b5c7..a0dea23 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -116,42 +116,53 @@ static int macvlan_addr_busy(const struct macvlan_port *port,
> return 0;
> }
>
> +static inline void macvlan_count_rx(const struct macvlan_dev *vlan, int length,

Please use unsigned int for length values.

Regarding Eric's comments, I also think it would be more readable to use
if (success) {
...
} else {
...
}

> + int success, int 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;
> +}
> +
> +static int macvlan_broadcast_one(struct sk_buff *skb, struct net_device *dev,
> + const struct ethhdr *eth)
> +{
> + if (!skb)
> + return NET_RX_DROP;
> +
> + skb->dev = dev;
> + if (!compare_ether_addr_64bits(eth->h_dest,
> + dev->broadcast))

This would fit on one line without reducing readability.

> + skb->pkt_type = PACKET_BROADCAST;
> + else
> + skb->pkt_type = PACKET_MULTICAST;
> +
> + return netif_rx(skb);
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads