Skip to contentSkip to conversation

net: rocker: Add basic netdev counters

PATCH In review David Ahern 3 participants 4 messages Jan 14, 2015 – Jan 16, 2015

Subsystems (3)
Networking Network drivers Rocker driver
lore
Share & tools
lore

David Ahern

author
#0 lore raw

PATCH 1/1 net: rocker: Add basic netdev counters

Add packet and byte counters for RX and TX paths.

$ ifconfig eth1
eth1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
        inet6 fe80::5054:ff:fe12:3501  prefixlen 64  scopeid 0x20<link>
        ether 52:54:00:12:35:01  txqueuelen 1000  (Ethernet)
        RX packets 63  bytes 15813 (15.4 KiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 79  bytes 17991 (17.5 KiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
Signed-off-by David Ahern <[email protected]>
Cc Scott Feldman <[email protected]>
Cc Jiri Pirko <[email protected]>
---
 drivers/net/ethernet/rocker/rocker.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/net/ethernet/rocker/rocker.c +8 -0
index 2f398fa4b9e6..9743279d9121 100644
@@ -3557,6 +3557,9 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (!desc_info)
 		netif_stop_queue(dev);
 
+	dev->stats.tx_packets++;
+	dev->stats.tx_bytes += skb->len;
+
 	return NETDEV_TX_OK;
 
 unmap_frags:

@@ -3565,6 +3568,8 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev)
 	rocker_tlv_nest_cancel(desc_info, frags);
 out:
 	dev_kfree_skb(skb);
+	dev->stats.tx_dropped++;
+
 	return NETDEV_TX_OK;
 }
 

@@ -3890,6 +3895,9 @@ static int rocker_port_rx_proc(struct rocker *rocker,
 	skb->protocol = eth_type_trans(skb, rocker_port->dev);
 	netif_receive_skb(skb);
 
+	rocker_port->dev->stats.rx_packets++;
+	rocker_port->dev->stats.rx_bytes += skb->len;
+
 	return rocker_dma_rx_ring_skb_alloc(rocker, rocker_port, desc_info);
 }
Signature · 1 line
1.9.3 (Apple Git-50)
#1 lore raw
Quoted 23 lines · David Ahern

Add packet and byte counters for RX and TX paths.

$ ifconfig eth1
eth1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
        inet6 fe80::5054:ff:fe12:3501  prefixlen 64  scopeid 0x20<link>
        ether 52:54:00:12:35:01  txqueuelen 1000  (Ethernet)
        RX packets 63  bytes 15813 (15.4 KiB)
        RX errors 0  dropped 0  overruns 0  frame 0
        TX packets 79  bytes 17991 (17.5 KiB)
        TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
Signed-off-by David Ahern <[email protected]>
Cc Scott Feldman <[email protected]>
Cc Jiri Pirko <[email protected]>
---
 drivers/net/ethernet/rocker/rocker.c | 8 ++++++++
 1 file changed, 8 insertions(+)
drivers/net/ethernet/rocker/rocker.c +0 -0
index 2f398fa4b9e6..9743279d9121 100644

drivers/net/ethernet/rocker/rocker.c static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev) · line 3561

Quoted 5 lines
drivers/net/ethernet/rocker/rocker.c +2 -0
@@ -3557,6 +3557,9 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (!desc_info)
 		netif_stop_queue(dev);
 
+	dev->stats.tx_packets++;
+	dev->stats.tx_bytes += skb->len;

Potential use after free, the skb pointer is certainly not valid anymore here.

BTW, increasing statistics here is valid because this is a driver for a virtual piece of HW, which does not have TX reclaim/completion logic, but if it did, statistics update should occur there, not in the ndo_start_xmit() function.

Quoted 14 lines
drivers/net/ethernet/rocker/rocker.c +3 -0
+
 	return NETDEV_TX_OK;
 
 unmap_frags:

@@ -3565,6 +3568,8 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev)
 	rocker_tlv_nest_cancel(desc_info, frags);
 out:
 	dev_kfree_skb(skb);
+	dev->stats.tx_dropped++;
+
 	return NETDEV_TX_OK;
 }

drivers/net/ethernet/rocker/rocker.c static int rocker_port_rx_proc(struct rocker *rocker, · line 3899

Quoted 5 lines
drivers/net/ethernet/rocker/rocker.c +2 -0
@@ -3890,6 +3895,9 @@ static int rocker_port_rx_proc(struct rocker *rocker,
 	skb->protocol = eth_type_trans(skb, rocker_port->dev);
 	netif_receive_skb(skb);
 
+	rocker_port->dev->stats.rx_packets++;
+	rocker_port->dev->stats.rx_bytes += skb->len;

Same here, past netif_receive_skb() you should not assume that this skb reference is valid.

Quoted 5 lines
drivers/net/ethernet/rocker/rocker.c +1 -0
+
 	return rocker_dma_rx_ring_skb_alloc(rocker, rocker_port, desc_info);
 }
Signature · 1 line
Florian
author
#2 lore raw
Quoted 33 lines · Florian Fainelli
Quoted 23 lines · David Ahern

Add packet and byte counters for RX and TX paths.

$ ifconfig eth1
eth1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
         inet6 fe80::5054:ff:fe12:3501  prefixlen 64  scopeid 0x20<link>
         ether 52:54:00:12:35:01  txqueuelen 1000  (Ethernet)
         RX packets 63  bytes 15813 (15.4 KiB)
         RX errors 0  dropped 0  overruns 0  frame 0
         TX packets 79  bytes 17991 (17.5 KiB)
         TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
Signed-off-by David Ahern <[email protected]>
Cc Scott Feldman <[email protected]>
Cc Jiri Pirko <[email protected]>
---
  drivers/net/ethernet/rocker/rocker.c | 8 ++++++++
  1 file changed, 8 insertions(+)
drivers/net/ethernet/rocker/rocker.c +0 -0
index 2f398fa4b9e6..9743279d9121 100644

drivers/net/ethernet/rocker/rocker.c static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev) · line 3561

Quoted 5 lines
drivers/net/ethernet/rocker/rocker.c +2 -0
@@ -3557,6 +3557,9 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev)
  	if (!desc_info)
  		netif_stop_queue(dev);
 
+	dev->stats.tx_packets++;
+	dev->stats.tx_bytes += skb->len;

Potential use after free, the skb pointer is certainly not valid anymore here.

Quoted 4 lines

BTW, increasing statistics here is valid because this is a driver for a virtual piece of HW, which does not have TX reclaim/completion logic, but if it did, statistics update should occur there, not in the ndo_start_xmit() function.

sure. I had considered putting in the rocker_port_poll_tx function like this:

- dev_kfree_skb_any(rocker_desc_cookie_ptr_get(desc_info));
+

+       skb = rocker_desc_cookie_ptr_get(desc_info);
+       rocker_port->dev->stats.tx_packets++;
+       rocker_port->dev->stats.tx_bytes += skb->len;

+
+ dev_kfree_skb_any(skb);

I think this the reclaim point.

Quoted 18 lines
Quoted 18 lines
drivers/net/ethernet/rocker/rocker.c +3 -0
+
  	return NETDEV_TX_OK;
 
  unmap_frags:

@@ -3565,6 +3568,8 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev)
  	rocker_tlv_nest_cancel(desc_info, frags);
  out:
  	dev_kfree_skb(skb);
+	dev->stats.tx_dropped++;
+
  	return NETDEV_TX_OK;
  }
 

@@ -3890,6 +3895,9 @@ static int rocker_port_rx_proc(struct rocker *rocker,
  	skb->protocol = eth_type_trans(skb, rocker_port->dev);
  	netif_receive_skb(skb);
Quoted 5 lines

drivers/net/ethernet/rocker/rocker.c static int rocker_port_rx_proc(struct rocker *rocker, · line 3899

Quoted 2 lines
drivers/net/ethernet/rocker/rocker.c +2 -0
+	rocker_port->dev->stats.rx_packets++;
+	rocker_port->dev->stats.rx_bytes += skb->len;

Same here, past netif_receive_skb() you should not assume that this skb reference is valid.

right. I'll move the stats above the netif_receive_skb.

Thanks,
David

#3 lore raw
Quoted 56 lines · David Ahern
Quoted 38 lines · Florian Fainelli
Quoted 25 lines · David Ahern

Add packet and byte counters for RX and TX paths.

$ ifconfig eth1
eth1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST>  mtu 1500
         inet6 fe80::5054:ff:fe12:3501  prefixlen 64  scopeid 0x20<link>
         ether 52:54:00:12:35:01  txqueuelen 1000  (Ethernet)
         RX packets 63  bytes 15813 (15.4 KiB)
         RX errors 0  dropped 0  overruns 0  frame 0
         TX packets 79  bytes 17991 (17.5 KiB)
         TX errors 0  dropped 0 overruns 0  carrier 0  collisions 0
Signed-off-by David Ahern <[email protected]>
Cc Scott Feldman <[email protected]>
Cc Jiri Pirko <[email protected]>
---
  drivers/net/ethernet/rocker/rocker.c | 8 ++++++++
  1 file changed, 8 insertions(+)
drivers/net/ethernet/rocker/rocker.c +0 -0
index 2f398fa4b9e6..9743279d9121 100644

drivers/net/ethernet/rocker/rocker.c static netdev_tx_t rocker_port_xmit(struct sk_buff · line 3562

Quoted 6 lines
drivers/net/ethernet/rocker/rocker.c +2 -0
@@ -3557,6 +3557,9 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff
 *skb, struct net_device *dev)
        if (!desc_info)
                netif_stop_queue(dev);
 
+       dev->stats.tx_packets++;
+       dev->stats.tx_bytes += skb->len;

Potential use after free, the skb pointer is certainly not valid anymore here.

Quoted 4 lines

BTW, increasing statistics here is valid because this is a driver for a virtual piece of HW, which does not have TX reclaim/completion logic, but if it did, statistics update should occur there, not in the ndo_start_xmit() function.

sure. I had considered putting in the rocker_port_poll_tx function like this:

(quoted diff) +6 -1
-       dev_kfree_skb_any(rocker_desc_cookie_ptr_get(desc_info));
+
+       skb = rocker_desc_cookie_ptr_get(desc_info);
+       rocker_port->dev->stats.tx_packets++;
+       rocker_port->dev->stats.tx_bytes += skb->len;
+
+       dev_kfree_skb_any(skb);
Quoted 1 line

I think this the reclaim point.

This is better. But also need to consider err code returned from device in same routine. If the desc was marked with err, then I think we can assume pkt wasn't sent so tx_packets/bytes stats should not be incremented, but some tx err stat should, based on desc err code.

-scott