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
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
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
> 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;
}
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
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)
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
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
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
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
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
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
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
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.