Skip to contentSkip to conversation

drop_monitor: fix trace_napi_poll_hit()

PATCH RESEND Applied Xiao Guangrong 4 participants 10 messages Aug 31, 2009 – Sep 3, 2009

Subsystems (12)
Networking Network drivers InfiniBand s390 +8 more subsystems
lore
Share & tools
lore

Xiao Guangrong

author
#0 lore raw

PATCH RESEND 1/1 drop_monitor: fix trace_napi_poll_hit()

The net_dev of backlog napi is NULL, like below:

__get_cpu_var(softnet_data).backlog.dev == NULL

So, we should check it in napi tracepoint's probe function

Acked-by Neil Horman <[email protected]>
Signed-off-by Xiao Guangrong <[email protected]>
---
 net/core/drop_monitor.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
net/core/drop_monitor.c +2 -1
index 9d66fa9..d311202 100644
@@ -182,7 +182,8 @@ static void trace_napi_poll_hit(struct napi_struct *napi)
 	/*
 	 * Ratelimit our check time to dm_hw_check_delta jiffies
 	 */
-	if (!time_after(jiffies, napi->dev->last_rx + dm_hw_check_delta))
+	if (!napi->dev ||
+	    !time_after(jiffies, napi->dev->last_rx + dm_hw_check_delta))
 		return;
 
 	rcu_read_lock();
Signature · 1 line
1.6.1.2
#1 lore raw
Quoted 17 lines · Xiao Guangrong

The net_dev of backlog napi is NULL, like below:

__get_cpu_var(softnet_data).backlog.dev == NULL

So, we should check it in napi tracepoint's probe function

Acked-by Neil Horman <[email protected]>
Signed-off-by Xiao Guangrong <[email protected]>
---
 net/core/drop_monitor.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
net/core/drop_monitor.c +0 -0
index 9d66fa9..d311202 100644

net/core/drop_monitor.c static void trace_napi_poll_hit(struct napi_struct *napi) · line 189

Quoted 9 lines
net/core/drop_monitor.c +2 -1
@@ -182,7 +182,8 @@ static void trace_napi_poll_hit(struct napi_struct *napi)
 	/*
 	 * Ratelimit our check time to dm_hw_check_delta jiffies
 	 */
-	if (!time_after(jiffies, napi->dev->last_rx + dm_hw_check_delta))
+	if (!napi->dev ||
+	    !time_after(jiffies, napi->dev->last_rx + dm_hw_check_delta))
 		return;
 
 	rcu_read_lock();

This reminds me dev->last_rx is not anymore updated, unless special conditions are met.

Test done in trace_napi_poll_hit() is probably not good, even with a non null napi->dev

maintainer
#2 lore raw
Quoted 26 lines · Eric Dumazet
Quoted 25 lines · Xiao Guangrong

The net_dev of backlog napi is NULL, like below:

__get_cpu_var(softnet_data).backlog.dev == NULL

So, we should check it in napi tracepoint's probe function

Acked-by Neil Horman <[email protected]>
Signed-off-by Xiao Guangrong <[email protected]>
---
 net/core/drop_monitor.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
net/core/drop_monitor.c +2 -1
index 9d66fa9..d311202 100644
@@ -182,7 +182,8 @@ static void trace_napi_poll_hit(struct napi_struct *napi)
 	/*
 	 * Ratelimit our check time to dm_hw_check_delta jiffies
 	 */
-	if (!time_after(jiffies, napi->dev->last_rx + dm_hw_check_delta))
+	if (!napi->dev ||
+	    !time_after(jiffies, napi->dev->last_rx + dm_hw_check_delta))
 		return;
Quoted 6 lines

net/core/drop_monitor.c static void trace_napi_poll_hit(struct napi_struct *napi) · line 189

Quoted 1 line
net/core/drop_monitor.c +0 -0
 	rcu_read_lock();

This reminds me dev->last_rx is not anymore updated, unless special conditions are met.

I still see a large number of drivers that update dev->last_rx, although its not all as I look through the list, so something definately seems amiss.

If its not going to be consistently updated, why are still carrying that field in dev? Are we just waiting on someone to do the janitorial work to remove it? If so, I can, and I'll fix up the drop monitor in the process, to use a private timestamp.

Neil

Quoted 6 lines

Test done in trace_napi_poll_hit() is probably not good, even with a non null napi->dev --
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#3 lore raw

From: Neil Horman <[email protected]>
Date: Mon, 31 Aug 2009 07:12:46 -0400

Quoted 4 lines

If its not going to be consistently updated, why are still carrying that field in dev? Are we just waiting on someone to do the janitorial work to remove it? If so, I can, and I'll fix up the drop monitor in the process, to use a private timestamp.

It's used only for bonding, so we only update it when a device receives packet as part of a bond.

%99.9999 of people are not in that situation, and in that case this is a very wasteful and expensive cacheline dirtying, so we elide it when we can.

#4 lore raw
In reply to Neil Horman
Quoted 2 lines · Neil Horman

I still see a large number of drivers that update dev->last_rx, although its not all as I look through the list, so something definately seems amiss.

Some drivers still update dev->last_rx for their own needs, not a core network concern.

But a cleanup is certainly possible on few other drivers, about a dozen if I count correctly.

Quoted 5 lines

If its not going to be consistently updated, why are still carrying that field in dev? Are we just waiting on someone to do the janitorial work to remove it? If so, I can, and I'll fix up the drop monitor in the process, to use a private timestamp.

We have to keep dev->last_rx for bonding use, so please use a private timestamp for drop monitor.

Thanks

maintainer
#5 lore raw
Quoted 11 lines · Eric Dumazet
Quoted 2 lines · Neil Horman

I still see a large number of drivers that update dev->last_rx, although its not all as I look through the list, so something definately seems amiss.

Some drivers still update dev->last_rx for their own needs, not a core network concern.

But a cleanup is certainly possible on few other drivers, about a dozen if I count correctly.

Quoted 1 line
Quoted 8 lines
Quoted 4 lines

If its not going to be consistently updated, why are still carrying that field in dev? Are we just waiting on someone to do the janitorial work to remove it? If so, I can, and I'll fix up the drop monitor in the process, to use a private timestamp.

We have to keep dev->last_rx for bonding use, so please use a private timestamp for drop monitor.

Copy that, I'll submit a drop monitor patch for this sometime this week. Thanks for the heads up!
Neil

Quoted 6 lines

Thanks
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#6 lore raw

PATCH 1/1 drivers: Kill now superfluous ->last_rx stores

In reply to Eric Dumazet
Quoted 9 lines · Eric Dumazet
Quoted 2 lines · Neil Horman

I still see a large number of drivers that update dev->last_rx, although its not all as I look through the list, so something definately seems amiss.

Some drivers still update dev->last_rx for their own needs, not a core network concern.

But a cleanup is certainly possible on few other drivers, about a dozen if I count correctly.

Well, a litle bit more if we look outside of drivers/net

[PATCH net-next-2.6] drivers: Kill now superfluous ->last_rx stores

The generic packet receive code takes care of setting netdev->last_rx when necessary, for the sake of the bonding ARP monitor.

Signed-off-by Eric Dumazet <[email protected]>
---
 drivers/firewire/net.c                  |    2 --
 drivers/ieee1394/eth1394.c              |    1 -
 drivers/infiniband/hw/amso1100/c2.c     |    1 -
 drivers/infiniband/hw/nes/nes_hw.c      |    2 +-
 drivers/infiniband/ulp/ipoib/ipoib_cm.c |    1 -
 drivers/infiniband/ulp/ipoib/ipoib_ib.c |    1 -
 drivers/misc/sgi-xp/xpnet.c             |    1 -
 drivers/net/arm/ks8695net.c             |    1 -
 drivers/net/arm/w90p910_ether.c         |    1 -
 drivers/net/atl1c/atl1c_main.c          |    1 -
 drivers/net/benet/be_main.c             |    2 --
 drivers/net/can/dev.c                   |    1 -
 drivers/net/can/sja1000/sja1000.c       |    2 --
 drivers/net/davinci_emac.c              |    1 -
 drivers/net/ethoc.c                     |    1 -
 drivers/net/igbvf/netdev.c              |    4 ----
 drivers/net/smsc911x.c                  |    1 -
 drivers/net/smsc9420.c                  |    1 -
 drivers/net/vxge/vxge-main.c            |    2 --
 drivers/net/wireless/b43legacy/xmit.c   |    1 -
 drivers/net/xilinx_emaclite.c           |    1 -
 drivers/s390/net/ctcm_main.c            |    1 -
 drivers/s390/net/netiucv.c              |    1 -
 drivers/s390/net/qeth_l2_main.c         |    1 -
 drivers/s390/net/qeth_l3_main.c         |    1 -
 25 files changed, 1 insertion(+), 32 deletions(-)
drivers/firewire/net.c +0 -2
index a42209a..3f58969 100644
@@ -663,8 +663,6 @@ static int fwnet_finish_incoming_packet(struct net_device *net,
 	if (netif_queue_stopped(net))
 		netif_wake_queue(net);
 
-	net->last_rx = jiffies;
-
 	return 0;
 }
drivers/ieee1394/eth1394.c +0 -1
index f5c586c..f3bf083 100644
@@ -1300,7 +1300,6 @@ static void ether1394_iso(struct hpsb_iso *iso)
 
 	hpsb_iso_recv_release_packets(iso, i);
 
-	dev->last_rx = jiffies;
 }
 
 /******************************************
drivers/infiniband/hw/amso1100/c2.c +0 -1
index 0cfbb6d..8c5d284 100644
@@ -530,7 +530,6 @@ static void c2_rx_interrupt(struct net_device *netdev)
 
 		netif_rx(skb);
 
-		netdev->last_rx = jiffies;
 		netdev->stats.rx_packets++;
 		netdev->stats.rx_bytes += buflen;
 	}
drivers/infiniband/hw/nes/nes_hw.c +1 -1
index 4a84d02..97d4c2a 100644
@@ -2741,7 +2741,7 @@ void nes_nic_ce_handler(struct nes_device *nesdev, struct nes_hw_nic_cq *cq)
 				}
 
 skip_rx_indicate0:
-				nesvnic->netdev->last_rx = jiffies;
+				;
 				/* nesvnic->netstats.rx_packets++; */
 				/* nesvnic->netstats.rx_bytes += rx_pkt_size; */
 			}
drivers/infiniband/ulp/ipoib/ipoib_cm.c +0 -1
index 181b1f3..986f07f 100644
@@ -663,7 +663,6 @@ copied:
 	skb_reset_mac_header(skb);
 	skb_pull(skb, IPOIB_ENCAP_LEN);
 
-	dev->last_rx = jiffies;
 	++dev->stats.rx_packets;
 	dev->stats.rx_bytes += skb->len;
drivers/infiniband/ulp/ipoib/ipoib_ib.c +0 -1
index e7e5adf..c9dcb20 100644
@@ -277,7 +277,6 @@ static void ipoib_ib_handle_rx_wc(struct net_device *dev, struct ib_wc *wc)
 	skb_reset_mac_header(skb);
 	skb_pull(skb, IPOIB_ENCAP_LEN);
 
-	dev->last_rx = jiffies;
 	++dev->stats.rx_packets;
 	dev->stats.rx_bytes += skb->len;
drivers/misc/sgi-xp/xpnet.c +0 -1
index 212da9a..16f0abd 100644
@@ -240,7 +240,6 @@ xpnet_receive(short partid, int channel, struct xpnet_message *msg)
 		(void *)skb->head, (void *)skb->data, skb_tail_pointer(skb),
 		skb_end_pointer(skb), skb->len);
 
-	xpnet_device->last_rx = jiffies;
 	xpnet_device->stats.rx_packets++;
 	xpnet_device->stats.rx_bytes += skb->len + ETH_HLEN;
drivers/net/arm/ks8695net.c +0 -1
index 35cd264..e12d2c8 100644
@@ -467,7 +467,6 @@ ks8695_rx_irq(int irq, void *dev_id)
 			netif_rx(skb);
 
 			/* Record stats */
-			ndev->last_rx = jiffies;
 			ndev->stats.rx_packets++;
 			ndev->stats.rx_bytes += pktlen;
 			goto rx_finished;
drivers/net/arm/w90p910_ether.c +0 -1
index 890716f..04f4114 100644
@@ -777,7 +777,6 @@ static void netdev_rx(struct net_device *dev)
 
 		rxbd = &ether->rdesc->desclist[ether->cur_rx];
 
-		dev->last_rx = jiffies;
 	} while (1);
 }
drivers/net/atl1c/atl1c_main.c +0 -1
index 1d601ce..81c6928 100644
@@ -1740,7 +1740,6 @@ rrs_checked:
 		} else
 			netif_receive_skb(skb);
 
-		netdev->last_rx = jiffies;
 		(*work_done)++;
 		count++;
 	}
drivers/net/benet/be_main.c +0 -2
index 7b9efee..0695acc 100644
@@ -774,8 +774,6 @@ static void be_rx_compl_process(struct be_adapter *adapter,
 		netif_receive_skb(skb);
 	}
 
-	adapter->netdev->last_rx = jiffies;
-
 	return;
 }
drivers/net/can/dev.c +0 -1
index e1a4f82..1d29082 100644
@@ -357,7 +357,6 @@ void can_restart(unsigned long data)
 
 	netif_rx(skb);
 
-	dev->last_rx = jiffies;
 	stats->rx_packets++;
 	stats->rx_bytes += cf->can_dlc;
drivers/net/can/sja1000/sja1000.c +0 -2
index b3004de..56a1355 100644
@@ -339,7 +339,6 @@ static void sja1000_rx(struct net_device *dev)
 
 	netif_rx(skb);
 
-	dev->last_rx = jiffies;
 	stats->rx_packets++;
 	stats->rx_bytes += dlc;
 }

@@ -454,7 +453,6 @@ static int sja1000_err(struct net_device *dev, uint8_t isrc, uint8_t status)
 
 	netif_rx(skb);
 
-	dev->last_rx = jiffies;
 	stats->rx_packets++;
 	stats->rx_bytes += cf->can_dlc;
drivers/net/davinci_emac.c +0 -1
index 5e6652b..d465eaa 100644
@@ -1920,7 +1920,6 @@ static int emac_net_rx_cb(struct emac_priv *priv,
 	skb_put(p_skb, net_pkt_list->pkt_length);
 	EMAC_CACHE_INVALIDATE((unsigned long)p_skb->data, p_skb->len);
 	p_skb->protocol = eth_type_trans(p_skb, priv->ndev);
-	p_skb->dev->last_rx = jiffies;
 	netif_receive_skb(p_skb);
 	priv->net_dev_stats.rx_bytes += net_pkt_list->pkt_length;
 	priv->net_dev_stats.rx_packets++;
drivers/net/ethoc.c +0 -1
index 4dbe5f1..710b564 100644
@@ -404,7 +404,6 @@ static int ethoc_rx(struct net_device *dev, int limit)
 				void *src = priv->membase + bd.addr;
 				memcpy_fromio(skb_put(skb, size), src, size);
 				skb->protocol = eth_type_trans(skb, dev);
-				dev->last_rx = jiffies;
 				priv->stats.rx_packets++;
 				priv->stats.rx_bytes += size;
 				netif_receive_skb(skb);
drivers/net/igbvf/netdev.c +0 -4
index 3f03c42..3dced7c 100644
@@ -96,8 +96,6 @@ static void igbvf_receive_skb(struct igbvf_adapter *adapter,
 		                         E1000_RXD_SPC_VLAN_MASK);
 	else
 		netif_receive_skb(skb);
-
-	netdev->last_rx = jiffies;
 }
 
 static inline void igbvf_rx_checksum_adv(struct igbvf_adapter *adapter,

@@ -342,8 +340,6 @@ send_up:
 		igbvf_receive_skb(adapter, netdev, skb, staterr,
 		                  rx_desc->wb.upper.vlan);
 
-		netdev->last_rx = jiffies;
-
 next_desc:
 		rx_desc->wb.upper.status_error = 0;
drivers/net/smsc911x.c +0 -1
index c266785..ccdd196 100644
@@ -1047,7 +1047,6 @@ static int smsc911x_poll(struct napi_struct *napi, int budget)
 		/* Update counters */
 		dev->stats.rx_packets++;
 		dev->stats.rx_bytes += (pktlength - 4);
-		dev->last_rx = jiffies;
 	}
 
 	/* Return total received packets */
drivers/net/smsc9420.c +0 -1
index 60abdb1..4434b51 100644
@@ -817,7 +817,6 @@ static void smsc9420_rx_handoff(struct smsc9420_pdata *pd, const int index,
 	skb->protocol = eth_type_trans(skb, dev);
 
 	netif_receive_skb(skb);
-	dev->last_rx = jiffies;
 }
 
 static int smsc9420_alloc_rx_buffer(struct smsc9420_pdata *pd, int index)
drivers/net/vxge/vxge-main.c +0 -2
index 094d155..53941b9 100644
@@ -588,8 +588,6 @@ vxge_rx_1b_compl(struct __vxge_hw_ring *ringh, void *dtr,
 	if (first_dtr)
 		vxge_hw_ring_rxd_post_post_wmb(ringh, first_dtr);
 
-	dev->last_rx = jiffies;
-
 	vxge_debug_entryexit(VXGE_TRACE,
 				"%s:%d  Exiting...",
 				__func__, __LINE__);
drivers/net/wireless/b43legacy/xmit.c +0 -1
index f79cee8..103f3c9 100644
@@ -590,7 +590,6 @@ void b43legacy_rx(struct b43legacy_wldev *dev,
 		       chanstat);
 	}
 
-	dev->stats.last_rx = jiffies;
 	memcpy(IEEE80211_SKB_RXCB(skb), &status, sizeof(status));
 	ieee80211_rx_irqsafe(dev->wl->hw, skb);
drivers/net/xilinx_emaclite.c +0 -1
index 7e05b40..dc22782 100755
@@ -587,7 +587,6 @@ static void xemaclite_rx_handler(struct net_device *dev)
 
 	dev->stats.rx_packets++;
 	dev->stats.rx_bytes += len;
-	dev->last_rx = jiffies;
 
 	netif_rx(skb);		/* Send the packet upstream */
 }
drivers/s390/net/ctcm_main.c +0 -1
index 38b5079..c5b8387 100644
@@ -164,7 +164,6 @@ void ctcm_unpack_skb(struct channel *ch, struct sk_buff *pskb)
 		priv->stats.rx_packets++;
 		priv->stats.rx_bytes += skblen;
 		netif_rx_ni(skb);
-		dev->last_rx = jiffies;
 		if (len > 0) {
 			skb_pull(pskb, header->length);
 			if (skb_tailroom(pskb) < LL_HEADER_LENGTH) {
drivers/s390/net/netiucv.c +0 -1
index bb1183a..271c4a8 100644
@@ -676,7 +676,6 @@ static void netiucv_unpack_skb(struct iucv_connection *conn,
 		 * we must use netif_rx_ni() instead of netif_rx()
 		 */
 		netif_rx_ni(skb);
-		dev->last_rx = jiffies;
 		skb_pull(pskb, header->next);
 		skb_put(pskb, NETIUCV_HDRLEN);
 	}
drivers/s390/net/qeth_l2_main.c +0 -1
index 12ee7a3..7d7d6d9 100644
@@ -449,7 +449,6 @@ static void qeth_l2_process_inbound_buffer(struct qeth_card *card,
 			QETH_DBF_HEX(CTRL, 3, hdr, QETH_DBF_CTRL_LEN);
 			continue;
 		}
-		card->dev->last_rx = jiffies;
 		card->stats.rx_packets++;
 		card->stats.rx_bytes += len;
 	}
drivers/s390/net/qeth_l3_main.c +0 -1
index d9fabe3..d2eabbc 100644
@@ -1987,7 +1987,6 @@ static void qeth_l3_process_inbound_buffer(struct qeth_card *card,
 			continue;
 		}
 
-		card->dev->last_rx = jiffies;
 		card->stats.rx_packets++;
 		card->stats.rx_bytes += len;
 	}
maintainer Acked-by on v1 1/1
#7 lore raw

Re: [PATCH net-next-2.6] drivers: Kill now superfluous ->last_rx stores

Quoted 13 lines · Eric Dumazet
Quoted 9 lines · Eric Dumazet
Quoted 2 lines · Neil Horman

I still see a large number of drivers that update dev->last_rx, although its not all as I look through the list, so something definately seems amiss.

Some drivers still update dev->last_rx for their own needs, not a core network concern.

But a cleanup is certainly possible on few other drivers, about a dozen if I count correctly.

Well, a litle bit more if we look outside of drivers/net

Quoted 7 lines

[PATCH net-next-2.6] drivers: Kill now superfluous ->last_rx stores

The generic packet receive code takes care of setting netdev->last_rx when necessary, for the sake of the bonding ARP monitor.

Signed-off-by Eric Dumazet <[email protected]>
Acked-by Neil Horman <[email protected]>

Thanks!
Neil

maintainer Applied
#8 lore raw

Re: [PATCH net-next-2.6] drivers: Kill now superfluous ->last_rx stores

In reply to Eric Dumazet

From: Eric Dumazet <[email protected]>
Date: Mon, 31 Aug 2009 18:34:50 +0200

Quoted 13 lines
Quoted 9 lines · Eric Dumazet
Quoted 2 lines · Neil Horman

I still see a large number of drivers that update dev->last_rx, although its not all as I look through the list, so something definately seems amiss.

Some drivers still update dev->last_rx for their own needs, not a core network concern.

But a cleanup is certainly possible on few other drivers, about a dozen if I count correctly.

Well, a litle bit more if we look outside of drivers/net

Quoted 7 lines

[PATCH net-next-2.6] drivers: Kill now superfluous ->last_rx stores

The generic packet receive code takes care of setting netdev->last_rx when necessary, for the sake of the bonding ARP monitor.

Signed-off-by Eric Dumazet <[email protected]>

Applied, thanks Eric.