spin_locks without smp.

spin_locks without smp.

[Maciej Soltysiak]

Hi,

while browsing through the network drivers about the etherleak issue i
found that some drivers have:

#ifdef CONFIG_SMP
	spin_lock_irqsave(...)
#endif

and some just:

	spin_lock_irqsave(...)

or similar.
Which version should be practiced? i thought spinlocks are irrelevant
without SMP so we should use #ifdef to shorten the execution path.

Regards,
Maciej Soltysiak

Re: spin_locks without smp.

[William Lee Irwin III]

On Fri, Jan 10, 2003 at 12:42:34PM +0100, Maciej Soltysiak wrote:
> while browsing through the network drivers about the etherleak issue i
> found that some drivers have:
> #ifdef CONFIG_SMP
> 	spin_lock_irqsave(...)
> #endif
> and some just:
> 	spin_lock_irqsave(...)
> or similar.
> Which version should be practiced? i thought spinlocks are irrelevant
> without SMP so we should use #ifdef to shorten the execution path.

Buggy on preempt. Remove the #ifdef


Bill

Re: spin_locks without smp.

[William Lee Irwin III]

On Fri, Jan 10, 2003 at 12:42:34PM +0100, Maciej Soltysiak wrote:
>> while browsing through the network drivers about the etherleak issue i
>> found that some drivers have:
>> #ifdef CONFIG_SMP
>> 	spin_lock_irqsave(...)
>> #endif
>> and some just:
>> 	spin_lock_irqsave(...)
>> or similar.
>> Which version should be practiced? i thought spinlocks are irrelevant
>> without SMP so we should use #ifdef to shorten the execution path.

On Fri, Jan 10, 2003 at 03:45:46AM -0800, William Lee Irwin III wrote:
> Buggy on preempt. Remove the #ifdef

Actually the only extant example of this is in eexpress.c


Bill

[PATCH]Re: spin_locks without smp.

[Maciej Soltysiak]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1506 bytes --]

> On Fri, Jan 10, 2003 at 03:45:46AM -0800, William Lee Irwin III wrote:
> > Buggy on preempt. Remove the #ifdef
Yes sir. :)

Is that okay?

Maciej


--- linux/drivers/net/eexpress.c	2002-11-29 00:53:13.000000000 +0100
+++ linux.new/drivers/net/eexpress.c	2003-01-10 13:08:24.000000000 +0100
@@ -600,9 +600,7 @@
 static void eexp_timeout(struct net_device *dev)
 {
 	struct net_local *lp = (struct net_local *)dev->priv;
-#ifdef CONFIG_SMP
 	unsigned long flags;
-#endif
 	int status;

 	disable_irq(dev->irq);
@@ -612,9 +610,7 @@
 	 *	lets make it work first..
 	 */

-#ifdef CONFIG_SMP
 	spin_lock_irqsave(&lp->lock, flags);
-#endif

 	status = scb_status(dev);
 	unstick_cu(dev);
@@ -628,9 +624,7 @@
 		outb(0,dev->base_addr+SIGNAL_CA);
 	}
 	netif_wake_queue(dev);
-#ifdef CONFIG_SMP
 	spin_unlock_irqrestore(&lp->lock, flags);
-#endif
 }

 /*
@@ -640,9 +634,7 @@
 static int eexp_xmit(struct sk_buff *buf, struct net_device *dev)
 {
 	struct net_local *lp = (struct net_local *)dev->priv;
-#ifdef CONFIG_SMP
 	unsigned long flags;
-#endif

 #if NET_DEBUG > 6
 	printk(KERN_DEBUG "%s: eexp_xmit()\n", dev->name);
@@ -655,9 +647,7 @@
 	 *	lets make it work first..
 	 */

-#ifdef CONFIG_SMP
 	spin_lock_irqsave(&lp->lock, flags);
-#endif

 	{
 		unsigned short length = (ETH_ZLEN < buf->len) ? buf->len :
@@ -669,9 +659,7 @@
 	        eexp_hw_tx_pio(dev,data,length);
 	}
 	dev_kfree_skb(buf);
-#ifdef CONFIG_SMP
 	spin_unlock_irqrestore(&lp->lock, flags);
-#endif
 	enable_irq(dev->irq);
 	return 0;
 }

[-- Attachment #2: Type: TEXT/plain, Size: 1434 bytes --]

--- linux/drivers/net/eexpress.c	2002-11-29 00:53:13.000000000 +0100
+++ linux.new/drivers/net/eexpress.c	2003-01-10 13:08:24.000000000 +0100
@@ -600,9 +600,7 @@
 static void eexp_timeout(struct net_device *dev)
 {
 	struct net_local *lp = (struct net_local *)dev->priv;
-#ifdef CONFIG_SMP
 	unsigned long flags;
-#endif
 	int status;
 	
 	disable_irq(dev->irq);
@@ -612,9 +610,7 @@
 	 *	lets make it work first..
 	 */
 	 
-#ifdef CONFIG_SMP
 	spin_lock_irqsave(&lp->lock, flags);
-#endif
 
 	status = scb_status(dev);
 	unstick_cu(dev);
@@ -628,9 +624,7 @@
 		outb(0,dev->base_addr+SIGNAL_CA);
 	}
 	netif_wake_queue(dev);	
-#ifdef CONFIG_SMP
 	spin_unlock_irqrestore(&lp->lock, flags);
-#endif
 }
 
 /*
@@ -640,9 +634,7 @@
 static int eexp_xmit(struct sk_buff *buf, struct net_device *dev)
 {
 	struct net_local *lp = (struct net_local *)dev->priv;
-#ifdef CONFIG_SMP
 	unsigned long flags;
-#endif
 
 #if NET_DEBUG > 6
 	printk(KERN_DEBUG "%s: eexp_xmit()\n", dev->name);
@@ -655,9 +647,7 @@
 	 *	lets make it work first..
 	 */
 	 
-#ifdef CONFIG_SMP
 	spin_lock_irqsave(&lp->lock, flags);
-#endif
   
 	{
 		unsigned short length = (ETH_ZLEN < buf->len) ? buf->len :
@@ -669,9 +659,7 @@
 	        eexp_hw_tx_pio(dev,data,length);
 	}
 	dev_kfree_skb(buf);
-#ifdef CONFIG_SMP
 	spin_unlock_irqrestore(&lp->lock, flags);
-#endif
 	enable_irq(dev->irq);
 	return 0;
 }

Re: [PATCH]Re: spin_locks without smp.

[Alan Cox]

On Fri, 2003-01-10 at 12:09, Maciej Soltysiak wrote:
> > On Fri, Jan 10, 2003 at 03:45:46AM -0800, William Lee Irwin III wrote:
> > > Buggy on preempt. Remove the #ifdef
> Yes sir. :)
> 
> Is that okay?

I'm not convinced its the right way. The driver does the things it does in order
to keep performance acceptable. eexpress, 8390 and one or two other
drivers have a paticular problem that is hard to handle with our current
locks (and which at the time Linus made a decision wasn't a good thing
to try and handle generically). 

We have to ensure that the IRQ path doesn't execute in parallel with
the transmit/timeout path. At the same time the packet upload to the
card is extremely slow. Sufficiently slow in fact that serial ports
just stop working when you use it without the ifdef paths.

On uniprocessor systems even with pre-empt the IRQ handler cannot be 
pre-empted by normal code execution. On SMP they can run across two
processors. What the disable_irq path is doing for uniprocessor is
implementing

	spin_lock_irqsavesomeirqsonly()

and on the kind of boxes that have these old cards its pretty important
to keep this.

I would argue that if we have an IRQ disabled we should forbid pre-empt.
If an IRQ is disabled and we pre-empt to a task that needs to allocate
memory and we swap to a device on that IRQ we may deadlock.

So the fix is either to make disable_irq()/enable_irq() correctly 
adjust the pre-empt restrictions, which is actually quite hard to see
how to do right as the disable/enable may be in different tasks, or to
change the code to do the following


	preempt_disable()
	disable_irq()
#ifdef CONFIG_SMP
	spin_lock_...
#endif

Note that we must disable the irq before taking the spinlock or we 
have another deadlock with our irq path versus disable_irq waiting
for the IRQ completion before returning.

If my analysis of the disable_irq versus pre-empt and memory allocation
deadlock is correct we have some other cases we need to address too.

Alan

Re: [PATCH]Re: spin_locks without smp.

[William Lee Irwin III]

On Fri, 2003-01-10 at 12:09, Maciej Soltysiak wrote:
>> Yes sir. :)
>> Is that okay?

On Fri, Jan 10, 2003 at 01:20:48PM +0000, Alan Cox wrote:
> I'm not convinced its the right way. The driver does the things it
> does in order
> to keep performance acceptable. eexpress, 8390 and one or two other
> drivers have a paticular problem that is hard to handle with our current
> locks (and which at the time Linus made a decision wasn't a good thing
> to try and handle generically). 
> We have to ensure that the IRQ path doesn't execute in parallel with
> the transmit/timeout path. At the same time the packet upload to the
> card is extremely slow. Sufficiently slow in fact that serial ports
> just stop working when you use it without the ifdef paths.
> On uniprocessor systems even with pre-empt the IRQ handler cannot be 
> pre-empted by normal code execution. On SMP they can run across two
> processors. What the disable_irq path is doing for uniprocessor is
> implementing

Okay, what I'm getting here is that the UP case already has preempt
disabled b/c the locks are taken in IRQ context?

The thing I don't get is how the spinlock bits cause horrendous
timing issues on UP that are different from SMP, esp. b/c they are
#ifdef'd elsewhere to do nothing but inc/dec preempt_count elsewhere.
There's a bit of "how did it happen" missing in my mind at least.


On Fri, Jan 10, 2003 at 01:20:48PM +0000, Alan Cox wrote:
> 	spin_lock_irqsavesomeirqsonly()
> and on the kind of boxes that have these old cards its pretty important
> to keep this.
> I would argue that if we have an IRQ disabled we should forbid pre-empt.
> If an IRQ is disabled and we pre-empt to a task that needs to allocate
> memory and we swap to a device on that IRQ we may deadlock.
> So the fix is either to make disable_irq()/enable_irq() correctly 
> adjust the pre-empt restrictions, which is actually quite hard to see
> how to do right as the disable/enable may be in different tasks, or to
> change the code to do the following
> 	preempt_disable()
> 	disable_irq()
> #ifdef CONFIG_SMP
> 	spin_lock_...
> #endif

Hmm, the part I'm missing here is why folding the preempt_disable()
into the spin_lock() is wrong. Or is it the implicit local_irq_save()
that's the (massive performance) problem?


On Fri, Jan 10, 2003 at 01:20:48PM +0000, Alan Cox wrote:
> Note that we must disable the irq before taking the spinlock or we 
> have another deadlock with our irq path versus disable_irq waiting
> for the IRQ completion before returning.
> If my analysis of the disable_irq versus pre-empt and memory allocation
> deadlock is correct we have some other cases we need to address too.

Hmm, this is tricky, since it's really disabling interrupts for too
long to make progress on UP; I suspect this issue might have _some_
(negative) impact on SMP, but how much (or for how many relevant
systems) I'm not sure.

Some serious thought may need to go into this, but it's very far afield
for me. I think some ppl more directly involved with these issues
(rml, mingo, others???) might need to get flagged down to fix it for
legacy systems (presumably modern ones won't have the issue at all)
for 2.7.x etc. if it really does matter.

I'm tied up with 64GB at the moment so my wetware cpu cycles are really
totally unavailable for this. =(


Thanks,
Bill

Re: [PATCH]Re: spin_locks without smp.

[Alan Cox]

On Fri, 2003-01-10 at 13:04, William Lee Irwin III wrote:
> Okay, what I'm getting here is that the UP case already has preempt
> disabled b/c the locks are taken in IRQ context?

The tx/timeout path isnt always in IRQ context. It may have pre-empt
disabled I'm just playing safe

> The thing I don't get is how the spinlock bits cause horrendous
> timing issues on UP that are different from SMP, esp. b/c they are
> #ifdef'd elsewhere to do nothing but inc/dec preempt_count elsewhere.
> There's a bit of "how did it happen" missing in my mind at least

Take a look at 8390.c for the whole how to do it SMP thing. That took
2 months to debug. For junk like the eexpress I've taken the attitude
that people who stick on in an SMP box deserve what tkey get. OTOH
lots of old single cpu boxes ues them and with the ifdef stuff in they
are perfectly usable cards for firewalls, linux terminal server recycled
PC's in schools and so forth.

> > 	preempt_disable()
> > 	disable_irq()
> > #ifdef CONFIG_SMP
> > 	spin_lock_...
> > #endif
> 
> Hmm, the part I'm missing here is why folding the preempt_disable()
> into the spin_lock() is wrong. Or is it the implicit local_irq_save()
> that's the (massive performance) problem?

Its the implicit irqsave we need to avoid

> I'm tied up with 64GB at the moment so my wetware cpu cycles are really
> totally unavailable for this. =(

Commiserations. I suspect the ethernet stuff is easier.

Alan

Re: spin_locks without smp.

[Alan Cox]

On Fri, 2003-01-10 at 11:45, William Lee Irwin III wrote:
> On Fri, Jan 10, 2003 at 12:42:34PM +0100, Maciej Soltysiak wrote:
> > while browsing through the network drivers about the etherleak issue i
> > found that some drivers have:
> > #ifdef CONFIG_SMP
> > 	spin_lock_irqsave(...)
> > #endif
> > and some just:
> > 	spin_lock_irqsave(...)
> > or similar.
> > Which version should be practiced? i thought spinlocks are irrelevant
> > without SMP so we should use #ifdef to shorten the execution path.
> 
> Buggy on preempt. Remove the #ifdef

And render the driver unusable. Very clever. How about understanding *why*
something was done first 8)

Re: spin_locks without smp.

[William Lee Irwin III]

On Fri, Jan 10, 2003 at 12:42:34PM +0100, Maciej Soltysiak wrote:
>>> Which version should be practiced? i thought spinlocks are irrelevant
>>> without SMP so we should use #ifdef to shorten the execution path.

On Fri, 2003-01-10 at 11:45, William Lee Irwin III wrote:
>> Buggy on preempt. Remove the #ifdef

On Fri, Jan 10, 2003 at 01:23:56PM +0000, Alan Cox wrote:
> And render the driver unusable. Very clever. How about understanding *why*
> something was done first 8)


It's hard to see offhand (esp. w/o the hw) why increasing the
preempt_count temporarily would render it unusable. It looks like
there are deeper issues here from what you're telling me. I'll go
regroup and attempt to form some intelligent questions from your
other response.


Thanks,
Bill

Re: spin_locks without smp.

[Alan Cox]

On Fri, 2003-01-10 at 11:42, Maciej Soltysiak wrote:
> Hi,
> 
> while browsing through the network drivers about the etherleak issue i
> found that some drivers have:
> 
> #ifdef CONFIG_SMP
> 	spin_lock_irqsave(...)
> #endif
> 
> and some just:
> 
> 	spin_lock_irqsave(...)
> 
> or similar.
> Which version should be practiced? i thought spinlocks are irrelevant
> without SMP so we should use #ifdef to shorten the execution path.

Long answer: The network driver authors are doing some fairly clever
things to make old ISA adapters usable in Linux 2.4 and later. Linux
lacks the functionality to do 
	
	spin_lock_disable_irqmask(lock, flags, mask)

Implementing it is rather expensive and hard to do well. In general
those code paths need reviewing and probably to change to

	preempt_disable()
#ifdef CONFIG_SMP
	spin_lock_irqsave(..)
#endif
..


Please ensure that if you change these drivers you 
a) Have the hardware and test it uniprocessor and SMP
b) Verify that with your change a serial modem port still works
c) Understand the game the author is playing

(Also d) ensure the author understands the games she/he is playing too!)

If you've been looking at the etherleak stuff btw, the -ac tree has what
is theoretically a full set of fixes. I'd love someone who is looking
at this into 2.5 to review them and merge them into the 2.5 tree. I 
doubt I have them all right so more eyes is most welcome.

Alan

Re: [PATCH]Re: spin_locks without smp.

[Manfred Spraul]

Alan wrote:

>On Fri, 2003-01-10 at 13:04, William Lee Irwin III wrote:
>> Okay, what I'm getting here is that the UP case already has preempt
>> disabled b/c the locks are taken in IRQ context?
>
>The tx/timeout path isnt always in IRQ context.
>
It is.
tx and timeout are both called at BH context with the dev_xmit spinlock 
held. See Documentation/networking/netdevices.txt

What about

    disable_irq();
    spin_lock(&np->lock);

That's what 8390.c uses, no need for an #ifdef.

--
    Manfred

Re: [PATCH]Re: spin_locks without smp.

[Alan Cox]

On Fri, 2003-01-10 at 17:19, Manfred Spraul wrote:
> 
>     disable_irq();
>     spin_lock(&np->lock);
> 
> That's what 8390.c uses, no need for an #ifdef.

Does someone have a card they can test that on. If so then I agree
entirely it is the best way to go 

Re: [PATCH]Re: spin_locks without smp.

[Mikael Pettersson]

On 10 Jan 2003 18:18:39 +0000, Alan Cox wrote:
>On Fri, 2003-01-10 at 17:19, Manfred Spraul wrote:
>> 
>>     disable_irq();
>>     spin_lock(&np->lock);
>> 
>> That's what 8390.c uses, no need for an #ifdef.
>
>Does someone have a card they can test that on. If so then I agree
>entirely it is the best way to go 

I have an ISA NE2000 available for testing, if someone feeds me patches.
Only UP hardware, though.

/Mikael

Re: [PATCH]Re: spin_locks without smp.

[Alan Cox]

On Sat, 2003-01-11 at 13:22, Mikael Pettersson wrote:
> On 10 Jan 2003 18:18:39 +0000, Alan Cox wrote:
> >On Fri, 2003-01-10 at 17:19, Manfred Spraul wrote:
> >> 
> >>     disable_irq();
> >>     spin_lock(&np->lock);
> >> 
> >> That's what 8390.c uses, no need for an #ifdef.
> >
> >Does someone have a card they can test that on. If so then I agree
> >entirely it is the best way to go 
> 
> I have an ISA NE2000 available for testing, if someone feeds me patches.
> Only UP hardware, though.

Sorry I wasnt clear enough. The 8390 is done, the one that would need the 
testing is SMP etherexpress.c.