e1000 stop raw interrupts disabled nag from RT

PATCH In review Mark Huth 3 participants 8 messages Mar 1, 2007

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Mark Huth

author

2007-03-01 02:54 UTC #0 lore raw

PATCH 1/1 e1000 stop raw interrupts disabled nag from RT

Current e1000_xmit_frame spews raw interrupt disabled nag messages when used with RT kernel patches. This patch uses spin_trylock_irqsave, which allows RT patches to properly manage the irq semantics.

Signed-off-by Mark Huth <[email protected]>

---

drivers/net/e1000/e1000_main.c +1 -4

index 619c892..48f94ee 100644
@@ -3363,12 +3363,9 @@ e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 	    (adapter->hw.mac_type == e1000_82573))
 		e1000_transfer_dhcp_info(adapter, skb);
 
-	local_irq_save(flags);
-	if (!spin_trylock(&tx_ring->tx_lock)) {
+	if (!spin_trylock_irqsave(&tx_ring->tx_lock, flags))
 		/* Collision - tell upper layer to requeue */
-		local_irq_restore(flags);
 		return NETDEV_TX_LOCKED;
-	}
 
 	/* need: count + 2 desc gap to keep tail from touching
 	 * head, otherwise try next time */

Kok, Auke

2007-03-01 03:18 UTC #1 lore raw

Quoted 3 lines · Mark Huth

Current e1000_xmit_frame spews raw interrupt disabled nag messages when used with RT kernel patches. This patch uses spin_trylock_irqsave, which allows RT patches to properly manage the irq semantics.

Looks OK with me on first sight, I'll keep it on my stack and push it upstream after Jesse looks it over.

Which -RT paches make this pop up btw? I'd like to repro it.

Thanks,

Auke

Quoted 20 lines

Signed-off-by Mark Huth <[email protected]>

---

drivers/net/e1000/e1000_main.c +1 -4

index 619c892..48f94ee 100644
@@ -3363,12 +3363,9 @@ e1000_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 	    (adapter->hw.mac_type == e1000_82573))
 		e1000_transfer_dhcp_info(adapter, skb);
 
-	local_irq_save(flags);
-	if (!spin_trylock(&tx_ring->tx_lock)) {
+	if (!spin_trylock_irqsave(&tx_ring->tx_lock, flags))
 		/* Collision - tell upper layer to requeue */
-		local_irq_restore(flags);
 		return NETDEV_TX_LOCKED;
-	}
 
 	/* need: count + 2 desc gap to keep tail from touching
 	 * head, otherwise try next time */

Mark Huth

author

2007-03-01 05:00 UTC #2 lore raw

Quoted 5 lines · Auke

Quoted 3 lines · Mark Huth
Current e1000_xmit_frame spews raw interrupt disabled nag messages when used with RT kernel patches. This patch uses spin_trylock_irqsave, which allows RT patches to properly manage the irq semantics.

Quoted 7 lines

Looks OK with me on first sight, I'll keep it on my stack and push it upstream after Jesse looks it over.
Which -RT paches make this pop up btw? I'd like to repro it.
Thanks,
Auke

Auke,

Well, I'm not an expert on the realtime patches - but most any patch set from Ingo seems to set this off - we've run through a bunch all the way since 2.6.10. It's a standard warning to get drivers to not mess with the processor interrupt function, since RT threads both the hard and soft irqs, and except for rare instances, the drivers and critical region protection no longer require the processor interrupt to be off. The XX_irqsave functions get turned into pre-empt disable, which is adequate for most things. But the local_irq_save is recognized and warned, and then if it is really necessary it can be converted to an RT varient that won't warn.

Quoted 26 lines

Quoted 25 lines

Signed-off-by Mark Huth <[email protected]>

---

drivers/net/e1000/e1000_main.c +1 -4

index 619c892..48f94ee 100644
@@ -3363,12 +3363,9 @@ e1000_xmit_frame(struct sk_buff *skb, struct 
 net_device *netdev)
         (adapter->hw.mac_type == e1000_82573))
         e1000_transfer_dhcp_info(adapter, skb);
 
-    local_irq_save(flags);
-    if (!spin_trylock(&tx_ring->tx_lock)) {
+    if (!spin_trylock_irqsave(&tx_ring->tx_lock, flags))
         /* Collision - tell upper layer to requeue */
-        local_irq_restore(flags);
         return NETDEV_TX_LOCKED;
-    }
 
     /* need: count + 2 desc gap to keep tail from touching

* head, otherwise try next time */

Kok, Auke

2007-03-01 16:56 UTC #3 lore raw

Quoted 11 lines · Mark Huth

Quoted 8 lines · Auke
Quoted 3 lines · Mark Huth
Current e1000_xmit_frame spews raw interrupt disabled nag messages when used with RT kernel patches. This patch uses spin_trylock_irqsave, which allows RT patches to properly manage the irq semantics.
Looks OK with me on first sight, I'll keep it on my stack and push it upstream after Jesse looks it over.
Which -RT paches make this pop up btw? I'd like to repro it.
Well, I'm not an expert on the realtime patches - but most any patch set from Ingo seems to set this off - we've run through a bunch all the way since 2.6.10.

that's bizarre, and would imply that Ingo himself never spotted this in ages, not to mention never tell us about it?

Ingo?

Michal Schmidt

2007-03-01 17:14 UTC #4 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 7 lines · Auke

Quoted 3 lines · Mark Huth
Well, I'm not an expert on the realtime patches - but most any patch set from Ingo seems to set this off - we've run through a bunch all the way since 2.6.10.
that's bizarre, and would imply that Ingo himself never spotted this in ages, not to mention never tell us about it?

Ingo's -rt patches have contained this change to e1000 for some time. The code is perfectly fine as is for non-RT kernels, so maybe that's why noone felt like telling you about it.

Michal
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with Red Hat - http://enigmail.mozdev.org

iD8DBQFF5wlZabKV90ewf0QRAse3AJ979xur0u8X1DokSzaAMvLQCWPkXACfU9ss xC05DAbFNAaFMLnQrzBKit0=
=I5mt
-----END PGP SIGNATURE-----

Kok, Auke

2007-03-01 17:29 UTC #5 lore raw

Quoted 11 lines · Michal Schmidt

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 3 lines · Auke
Quoted 1 line · Mark Huth
Well, I'm not an expert on the realtime patches - but most any patch set from Ingo seems to set this off - we've run through a bunch all the way since 2.6.10.
that's bizarre, and would imply that Ingo himself never spotted this in ages, not to mention never tell us about it?
Ingo's -rt patches have contained this change to e1000 for some time. The code is perfectly fine as is for non-RT kernels, so maybe that's why noone felt like telling you about it.

uhh, sure... while we are at it then, are there any more patches like this? perhaps for e100? ixgb?

Cheers,

Auke

Michal Schmidt

2007-03-01 17:37 UTC #6 lore raw

Quoted 2 lines · Auke

uhh, sure... while we are at it then, are there any more patches like this? perhaps for e100? ixgb?

A quick grep through the current RT patch shows there are no other changes to e100, e1000, ixgb.

Michal

Mark Huth

author

2007-03-01 21:58 UTC #7 lore raw

In reply to Michal Schmidt

Quoted 21 lines · Michal Schmidt

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 6 lines · Auke
Quoted 2 lines · Mark Huth
Well, I'm not an expert on the realtime patches - but most any patch set from Ingo seems to set this off - we've run through a bunch all the way since 2.6.10.
that's bizarre, and would imply that Ingo himself never spotted this in ages, not to mention never tell us about it?
Ingo's -rt patches have contained this change to e1000 for some time. The code is perfectly fine as is for non-RT kernels, so maybe that's why noone felt like telling you about it.
Michal
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with Red Hat - http://enigmail.mozdev.org

Quoted 5 lines

iD8DBQFF5wlZabKV90ewf0QRAse3AJ979xur0u8X1DokSzaAMvLQCWPkXACfU9ss xC05DAbFNAaFMLnQrzBKit0=
=I5mt
-----END PGP SIGNATURE-----

Ah, tis true indeed. This only shows up when one brings a newer driver forward after the RT patch has been applied. I guess you would need to coordinate this with Ingo.

Thanks,
Mark Huth