Patchwork [net-next-2.6,8/9] vxge: Acquire correct lock based on interrupt context.

login
register
mail settings
Submitter Sreenivasa Honnur
Date Oct. 5, 2009, 11:58 a.m.
Message ID <Pine.GSO.4.10.10910050757300.9377-100000@guinness>
Download mbox | patch
Permalink /patch/35000/
State Rejected
Delegated to: David Miller
Headers show

Comments

Sreenivasa Honnur - Oct. 5, 2009, 11:58 a.m.
- Added macros that check if the thread is in interrupt context or not to
  acquire or release locks

Signed-off-by: Sreenivasa Honnur <sreenivasa.honnur@neterion.com>
---

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 5, 2009, 12:37 p.m.
From: Sreenivasa Honnur <Sreenivasa.Honnur@neterion.com>
Date: Mon, 5 Oct 2009 07:58:42 -0400 (EDT)

> - Added macros that check if the thread is in interrupt context or not to
>   acquire or release locks
> 
> Signed-off-by: Sreenivasa Honnur <sreenivasa.honnur@neterion.com>

There is nothing at all wrong with using irqsave/irqrestore spin lock
calls when you're already in an interrupt.

I don't see any reason for this change.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ramkrishna Vepa - Oct. 6, 2009, 2:36 a.m.
> > - Added macros that check if the thread is in interrupt context or
not
> to
> >   acquire or release locks
> >
> > Signed-off-by: Sreenivasa Honnur <sreenivasa.honnur@neterion.com>
> 
> There is nothing at all wrong with using irqsave/irqrestore spin lock
> calls when you're already in an interrupt.
> 
> I don't see any reason for this change.
[Ram] Right, but why have the additional step of saving and restoring
the flags while in the interrupt context?

Ram
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - Oct. 6, 2009, 3:19 a.m.
Ramkrishna Vepa a écrit :
> [Ram] Right, but why have the additional step of saving and restoring
> the flags while in the interrupt context?
> 

It costs about the same thing than testing two times in_interrupt()

pushf
pop register
test iflag,register
je ...


In the end, original code is shorter and faster.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ramkrishna Vepa - Oct. 6, 2009, 3:23 a.m.
> Ramkrishna Vepa a écrit :
> > [Ram] Right, but why have the additional step of saving and restoring
> > the flags while in the interrupt context?
> >
> 
> It costs about the same thing than testing two times in_interrupt()
> 
> pushf
> pop register
> test iflag,register
> je ...
> 
> 
> In the end, original code is shorter and faster.
[Ram] Got it. We'll remove this patch from the patch set.

Ram
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 6, 2009, 4:18 a.m.
From: "Ramkrishna Vepa" <Ramkrishna.Vepa@neterion.com>
Date: Mon, 5 Oct 2009 22:36:22 -0400

>> > - Added macros that check if the thread is in interrupt context or
> not
>> to
>> >   acquire or release locks
>> >
>> > Signed-off-by: Sreenivasa Honnur <sreenivasa.honnur@neterion.com>
>> 
>> There is nothing at all wrong with using irqsave/irqrestore spin lock
>> calls when you're already in an interrupt.
>> 
>> I don't see any reason for this change.
> [Ram] Right, but why have the additional step of saving and restoring
> the flags while in the interrupt context?

Do you know that in the interrupt handler, cpu interrupts are
actually enabled?

And the cost is (relatively speaking) next to nothing.  You're
in the process of taking cache misses and toucing PIO registers
(on the order of thousands of cycles).

Not doing a IRQ save/restore is going to save you a hand full
of cycles.

The new branch mispredict you might get there is probably more
expensive or of equal expense to the IRQ save/restore itself.

This change makes really no sense, NO OTHER DRIVER does crap
like this.  Because there is no reason to.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff -urpN patch7/drivers/net/vxge/vxge-main.c patch8/drivers/net/vxge/vxge-main.c
--- patch7/drivers/net/vxge/vxge-main.c	2009-09-04 02:13:34.000000000 -0700
+++ patch8/drivers/net/vxge/vxge-main.c	2009-09-04 02:18:15.000000000 -0700
@@ -97,10 +97,10 @@  static inline void VXGE_COMPLETE_VPATH_T
 		more = 0;
 		skb_ptr = completed;
 
-		if (spin_trylock_irqsave(&fifo->tx_lock, flags)) {
+		if (vxge_spin_trylock(&fifo->tx_lock, flags)) {
 			vxge_hw_vpath_poll_tx(fifo->handle, &skb_ptr,
 						NR_SKB_COMPLETED, &more);
-			spin_unlock_irqrestore(&fifo->tx_lock, flags);
+			vxge_spin_unlock(&fifo->tx_lock, flags);
 		}
 		/* free SKBs */
 		for (temp = completed; temp != skb_ptr; temp++)
diff -urpN patch7/drivers/net/vxge/vxge-main.h patch8/drivers/net/vxge/vxge-main.h
--- patch7/drivers/net/vxge/vxge-main.h	2009-09-04 02:05:36.000000000 -0700
+++ patch8/drivers/net/vxge/vxge-main.h	2009-09-04 02:16:40.000000000 -0700
@@ -89,6 +89,26 @@ 
 
 #define VXGE_LL_MAX_FRAME_SIZE(dev) ((dev)->mtu + VXGE_HW_MAC_HEADER_MAX_SIZE)
 
+#define vxge_spin_lock(l, f) {                  \
+	if (in_interrupt())                     \
+		spin_lock(l);                   \
+	else                                    \
+		spin_lock_irqsave(l, f);        \
+}
+
+#define vxge_spin_trylock(l, f) \
+({ \
+	in_interrupt() ? \
+	spin_trylock(l) : spin_trylock_irqsave(l, f); \
+})
+
+#define vxge_spin_unlock(l, f) {                \
+	if (in_interrupt())                     \
+		spin_unlock(l);                 \
+	else                                    \
+		spin_unlock_irqrestore(l, f);   \
+}
+
 enum vxge_reset_event {
 	/* reset events */
 	VXGE_LL_VPATH_RESET	= 0,