hello,
this patch changes the atm_dev_lock spinlock to a semaphore. this
solves the sleeping while holding a spinlock problem. none of the
code paths doing the original locking are in_interrupt() so this is
fairly straight forward.
diff -u net/atm.000/addr.c net/atm/addr.c
--- net/atm.000/addr.c Mon Feb 10 13:37:57 2003
+++ net/atm/addr.c Sun Feb 16 09:54:12 2003
@@ -42,7 +42,6 @@
*/
static DECLARE_MUTEX(local_lock);
-extern spinlock_t atm_dev_lock;
static void notify_sigd(struct atm_dev *dev)
{
diff -u net/atm.000/common.c net/atm/common.c
--- net/atm.000/common.c Mon Feb 10 13:39:18 2003
+++ net/atm/common.c Sun Feb 16 09:54:12 2003
@@ -18,6 +18,7 @@
#include <linux/capability.h>
#include <linux/mm.h> /* verify_area */
#include <linux/sched.h>
+#include <linux/sem.h>
#include <linux/time.h> /* struct timeval */
#include <linux/skbuff.h>
#include <linux/bitops.h>
@@ -27,6 +28,7 @@
#include <asm/atomic.h>
#include <asm/poll.h>
#include <asm/ioctls.h>
+#include <asm/semaphore.h>
#if defined(CONFIG_ATM_LANE) || defined(CONFIG_ATM_LANE_MODULE)
#include <linux/atmlec.h>
@@ -79,7 +81,7 @@
#define DPRINTK(format,args...)
#endif
-spinlock_t atm_dev_lock = SPIN_LOCK_UNLOCKED;
+DECLARE_MUTEX(atm_dev_sem);
static struct sk_buff *alloc_tx(struct atm_vcc *vcc,unsigned int size)
{
@@ -146,7 +148,7 @@
vcc->dev->ops->free_rx_skb(vcc,skb);
else kfree_skb(skb);
}
- spin_lock (&atm_dev_lock);
+ down(&atm_dev_sem);
fops_put (vcc->dev->ops);
if (atomic_read(&vcc->rx_inuse))
printk(KERN_WARNING "atm_release_vcc: strange ... "
@@ -154,11 +156,11 @@
atomic_read(&vcc->rx_inuse));
bind_vcc(vcc,NULL);
} else
- spin_lock (&atm_dev_lock);
+ down(&atm_dev_sem);
if (free_sk) free_atm_vcc_sk(sk);
- spin_unlock (&atm_dev_lock);
+ up(&atm_dev_sem);
}
@@ -269,14 +271,14 @@
struct atm_dev *dev;
int return_val;
- spin_lock (&atm_dev_lock);
+ down(&atm_dev_sem);
dev = atm_find_dev(itf);
if (!dev)
return_val = -ENODEV;
else
return_val = atm_do_connect_dev(vcc,dev,vpi,vci);
- spin_unlock (&atm_dev_lock);
+ up(&atm_dev_sem);
return return_val;
}
@@ -308,10 +310,10 @@
else {
struct atm_dev *dev;
- spin_lock (&atm_dev_lock);
+ down(&atm_dev_sem);
for (dev = atm_devs; dev; dev = dev->next)
if (!atm_do_connect_dev(vcc,dev,vpi,vci)) break;
- spin_unlock (&atm_dev_lock);
+ up(&atm_dev_sem);
if (!dev) return -ENODEV;
}
if (vpi == ATM_VPI_UNSPEC || vci == ATM_VCI_UNSPEC)
@@ -553,7 +555,7 @@
int error,len,size,number, ret_val;
ret_val = 0;
- spin_lock (&atm_dev_lock);
+ down(&atm_dev_sem);
vcc = ATM_SD(sock);
switch (cmd) {
case SIOCOUTQ:
@@ -945,7 +947,7 @@
ret_val = 0;
done:
- spin_unlock (&atm_dev_lock);
+ up(&atm_dev_sem);
return ret_val;
}
diff -u net/atm.000/resources.c net/atm/resources.c
--- net/atm.000/resources.c Mon Feb 10 13:38:54 2003
+++ net/atm/resources.c Sun Feb 16 09:58:11 2003
@@ -16,6 +16,8 @@
#include <linux/module.h>
#include <linux/bitops.h>
#include <net/sock.h> /* for struct sock */
+#include <linux/sem.h>
+#include <asm/semaphore.h>
#include "common.h"
#include "resources.h"
@@ -29,9 +31,9 @@
struct atm_dev *atm_devs = NULL;
static struct atm_dev *last_dev = NULL;
struct atm_vcc *nodev_vccs = NULL;
-extern spinlock_t atm_dev_lock;
+extern struct semaphore atm_dev_sem;
-/* Caller must hold atm_dev_lock. */
+/* Caller must hold atm_dev_sem. */
static struct atm_dev *__alloc_atm_dev(const char *type)
{
struct atm_dev *dev;
@@ -56,7 +58,7 @@
return dev;
}
-/* Caller must hold atm_dev_lock. */
+/* Caller must hold atm_dev_sem. */
static void __free_atm_dev(struct atm_dev *dev)
{
if (dev->prev)
@@ -70,7 +72,7 @@
kfree(dev);
}
-/* Caller must hold atm_dev_lock. */
+/* Caller must hold atm_dev_sem. */
struct atm_dev *atm_find_dev(int number)
{
struct atm_dev *dev;
@@ -87,7 +89,7 @@
{
struct atm_dev *dev;
- spin_lock(&atm_dev_lock);
+ down(&atm_dev_sem);
dev = __alloc_atm_dev(type);
if (!dev) {
@@ -131,7 +133,7 @@
#endif
done:
- spin_unlock(&atm_dev_lock);
+ up(&atm_dev_sem);
return dev;
}
@@ -142,9 +144,9 @@
if (dev->ops->proc_read)
atm_proc_dev_deregister(dev);
#endif
- spin_lock(&atm_dev_lock);
+ down(&atm_dev_sem);
__free_atm_dev(dev);
- spin_unlock(&atm_dev_lock);
+ up(&atm_dev_sem);
}
void shutdown_atm_dev(struct atm_dev *dev)
diff -u net/atm.000/signaling.c net/atm/signaling.c
--- net/atm.000/signaling.c Mon Feb 10 13:38:49 2003
+++ net/atm/signaling.c Sun Feb 16 09:59:12 2003
@@ -13,6 +13,8 @@
#include <linux/atmsvc.h>
#include <linux/atmdev.h>
#include <linux/bitops.h>
+#include <linux/sem.h>
+#include <asm/semaphore.h>
#include "resources.h"
#include "signaling.h"
@@ -33,7 +35,7 @@
struct atm_vcc *sigd = NULL;
static DECLARE_WAIT_QUEUE_HEAD(sigd_sleep);
-extern spinlock_t atm_dev_lock;
+extern struct semaphore atm_dev_sem;
static void sigd_put_skb(struct sk_buff *skb)
{
@@ -220,9 +222,9 @@
skb_queue_purge(&vcc->recvq);
purge_vccs(nodev_vccs);
- spin_lock (&atm_dev_lock);
+ down(&atm_dev_sem);
for (dev = atm_devs; dev; dev = dev->next) purge_vccs(dev->vccs);
- spin_unlock (&atm_dev_lock);
+ up(&atm_dev_sem);
}
you seem to be under the impression that <linux/sem.h> has something
to do with linux semaphores. this is not the case; they're sysv semaphores.
apart from this, i think it's a pretty bad idea to just replace the
spinlocks with semaphores. atm really needs fixing properly.
--
"It's not Hollywood. War is real, war is primarily not about defeat or
victory, it is about death. I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk
On Tue, 2003-02-18 at 18:53, Matthew Wilcox wrote:
> you seem to be under the impression that <linux/sem.h> has something
> to do with linux semaphores. this is not the case; they're sysv semaphores.
I agree, this bit has to be fixed.
> atm really needs fixing properly.
True, but his change by itself is OK. All of the places where
atm_dev_lock is acquired it is safe to do things like sleep.
While checking this I notice that atm_alloc_dev uses GFP_ATOMIC
thus unnecessarily.
Anyways, Matt, without a full time ATM maintainer and having basically
nobody who wants to take that on, we should take the small fixes when
they do occur and are correct as Chas's patch is.
In message <[email protected]>,Matthew Wilcox writes:
>you seem to be under the impression that <linux/sem.h> has something
>to do with linux semaphores. this is not the case; they're sysv semaphores.
sorry about that. i just picked what seemed like likely culprits for
header files.
>apart from this, i think it's a pretty bad idea to just replace the
>spinlocks with semaphores. atm really needs fixing properly.
yes, atm_dev_lock is wrapped around large chunks of the code. however,
it should probably never have been a spinlock--it certainly doesnt need
to be one. as a mutex its far less offensive. i am willing to take
suggestions as to what would need to be done to fix atm properly?
In message <[email protected]>,"David S. Miller" writes:
>Anyways, Matt, without a full time ATM maintainer and having basically
>nobody who wants to take that on, we should take the small fixes when
>they do occur and are correct as Chas's patch is.
we (meaning some folks here at nrl) already maintain a seperate
kernel with atm 'enhancements' locally. we are very interested in keeping
linux-atm alive (particularly in the linux kernel) and extending it
as well. i would take the role of maintainer for atm if no one truly
wants it.
From: chas williams <[email protected]>
Date: Wed, 19 Feb 2003 00:01:01 -0500
we (meaning some folks here at nrl) already maintain a seperate
kernel with atm 'enhancements' locally. we are very interested in keeping
linux-atm alive (particularly in the linux kernel) and extending it
as well. i would take the role of maintainer for atm if no one truly
wants it.
This is the situation.
Therefore, please send me a patch to add an appropriate entry
to linux/MAINTAINERS.
Thank you.
On Tue, Feb 18, 2003 at 11:52:50PM -0500, chas williams wrote:
> yes, atm_dev_lock is wrapped around large chunks of the code. however,
> it should probably never have been a spinlock--it certainly doesnt need
> to be one. as a mutex its far less offensive. i am willing to take
> suggestions as to what would need to be done to fix atm properly?
well, I'm no networking guru, but here are some of the things that seem
useful to me:
Try to use `struct sock' where you can. For example, using sk->stamp
would allow the SIOCGSTAMP ioctl to be dealt with at a higher level.
Right now, atm uses a timestamp embedded in vcc. (Actually, I think
I see how to do this one. I'll send you a patch for your review and
testing, OK?)
Figure out what actually needs to be locked and lock only that.
spinlocks are, by and large, the right solution -- having a Big ATM Lock
isn't a good thing (though I do recognise that you may want to do this
temporarily while you work on fixing it properly).
You should probably do away with SOCKOPS_WRAP / SOCKOPS_WRAPPED. The
BKL doesn't really protect you from much any more.
Might want to consider converting the proc files to the seqfile interface.
There seems to be some custom doubly-linked-list handling going on in
atm/resources.c. Should probably be switched to use <linux/list.h>.
I don't like the look of the locking in ipcommon.c:skb_migrate().
If there's really no better way of doing it, I'd recommend something like:
spinlock_t *first, *second;
if ((unsigned long)from < (unsigned long) to)) {
first = &from->lock;
second = &to->lock;
} else {
first = &to->lock;
second = &from->lock;
}
local_irq_save(flags);
spin_lock(&first);
spin_lock(&second);
(note that you can leave the spin_unlock and spin_unlock_irqrestore
calls as they are).
That's just a quick survey... I'm sure someone who actually uses ATM
could find more things to look at ;-)
--
"It's not Hollywood. War is real, war is primarily not about defeat or
victory, it is about death. I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk
David S. Miller wrote:
> From: chas williams <[email protected]>
> Date: Wed, 19 Feb 2003 00:01:01 -0500
>
> we (meaning some folks here at nrl) already maintain a seperate
> kernel with atm 'enhancements' locally. we are very interested in keeping
> linux-atm alive (particularly in the linux kernel) and extending it
> as well. i would take the role of maintainer for atm if no one truly
> wants it.
>
> This is the situation.
>
> Therefore, please send me a patch to add an appropriate entry
> to linux/MAINTAINERS.
Yes, Chas and I have already been discussing moving the maintainership.
Previously his work has been concentrated on 2.4 but now he's porting
his updates to 2.5 and will be taking over maintenence of the kernel stuff.
As for the locking issues a semaphore is probably the best thing to use
at the moment. The *correct* fix would be to have atm_dev's and atm_vcc's
be reference counted instead so the card's interrupt handlers can safely
work with them. This has been well understood for awhile and I was planning
on implementing it about a year ago for 2.5 but I've had basically zero time
to devote to linux-atm these days (sorry) Maybe Chas or I or someone will
do it for 2.7. The bad part isn't the atm core code (I could probably do
that myself in a day) but it would require overhaul of a lot of the ATM
drivers. Some of the drivers are pretty scary.
(Side note: basically linux-atm was originally written before SMP support
existed and made heavy use of cti/sti to make sure the dev's and vcc's
didn't change underneath the drivers. Later these became largely spinlocks
but they never really became SMP safe - even though the core code would
have proper SMP-exclusion we could have the card doing rx an atm_vcc
on one CPU while another was closing it.
There's some stream-of-consiousness comments about this that I wrote a
long time ago in drivers/atm/lanai.c around line 616. They're not 100%
correct - I was planning on researching the issue more before submitting
that driver for inclusion into the tree but it lanai.c eventually got
submitted by someone else without asking me first)
-Mitch
Hello Mitchell & Chas , Please do not forget about 2.4 tho as you
move forward . Tia , JimL
--
+------------------------------------------------------------------+
| James W. Laferriere | System Techniques | Give me VMS |
| Network Engineer | P.O. Box 854 | Give me Linux |
| [email protected] | Coudersport PA 16915 | only on AXP |
+------------------------------------------------------------------+