Messages in this thread | | | Date | Wed, 19 Feb 2003 14:30:52 +0000 | From | Matthew Wilcox <> | Subject | Re: [PATCH][2.5] convert atm_dev_lock from spinlock to semaphore |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |