Hello all,
The following patch swaps the save_flags/cli/restore_flags for a
spinlock. Please review.
Regards,
Frank
--- linux/net/atm/lec.c.old Sat Oct 19 12:08:19 2002
+++ linux/net/atm/lec.c Sat Dec 28 20:14:50 2002
@@ -20,7 +20,7 @@
#include <net/arp.h>
#include <net/dst.h>
#include <linux/proc_fs.h>
-
+#include <linux/spinlock.h>
/* TokenRing if needed */
#ifdef CONFIG_TR
#include <linux/trdevice.h>
@@ -63,6 +63,7 @@
#define LEC_UNRES_QUE_LEN 8 /* number of tx packets to queue for a
single destination while waiting for SVC */
+static spinlock_t lec_lock = SPIN_LOCK_UNLOCKED;
static int lec_open(struct net_device *dev);
static int lec_send_packet(struct sk_buff *skb, struct net_device *dev);
@@ -1110,8 +1111,7 @@
unsigned long flags;
struct lec_arp_table *tmp;
- save_flags(flags);
- cli();
+ spin_lock_irqsave(&lec_lock, flags);
place = HASH(to_put->mac_addr[ETH_ALEN-1]);
tmp = lec_arp_tables[place];
@@ -1125,7 +1125,7 @@
tmp->next = to_put;
}
- restore_flags(flags);
+ spin_unlock_irqrestore(&lec_lock, flags);
DPRINTK("LEC_ARP: Added entry:%2.2x %2.2x %2.2x %2.2x %2.2x %2.2x\n",
0xff&to_put->mac_addr[0], 0xff&to_put->mac_addr[1],
0xff&to_put->mac_addr[2], 0xff&to_put->mac_addr[3],
@@ -1144,11 +1144,10 @@
unsigned long flags;
int remove_vcc=1;
- save_flags(flags);
- cli();
+ spin_lock_irqsave(&lec_lock, flags);
if (!to_remove) {
- restore_flags(flags);
+ spin_unlock_irqrestore(&lec_lock, flags);
return -1;
}
place = HASH(to_remove->mac_addr[ETH_ALEN-1]);
@@ -1160,7 +1159,7 @@
tmp = tmp->next;
}
if (!tmp) {/* Entry was not found */
- restore_flags(flags);
+ spin_unlock_irqrestore(&lec_lock, flags);
return -1;
}
}
@@ -1186,7 +1185,7 @@
lec_arp_clear_vccs(to_remove);
}
skb_queue_purge(&to_remove->tx_wait); /* FIXME: good place for this? */
- restore_flags(flags);
+ spin_unlock_irqrestore(&lec_lock, flags);
DPRINTK("LEC_ARP: Removed entry:%2.2x %2.2x %2.2x %2.2x %2.2x %2.2x\n",
0xff&to_remove->mac_addr[0], 0xff&to_remove->mac_addr[1],
0xff&to_remove->mac_addr[2], 0xff&to_remove->mac_addr[3],
@@ -1374,8 +1373,7 @@
unsigned long flags;
int i;
- save_flags(flags);
- cli();
+ spin_lock_irqsave(&lec_lock, flags);
del_timer(&priv->lec_arp_timer);
@@ -1419,7 +1417,7 @@
priv->mcast_vcc = NULL;
memset(priv->lec_arp_tables, 0,
sizeof(struct lec_arp_table*)*LEC_ARP_TABLE_SIZE);
- restore_flags(flags);
+ spin_unlock_irqrestore(&lec_lock, flags);
}
@@ -2150,14 +2148,13 @@
lec_arp_lock(priv);
entry = priv->lec_arp_empty_ones;
if (vcc == entry->vcc) {
- save_flags(flags);
- cli();
+ spin_lock_irqsave(&lec_lock, flags);
del_timer(&entry->timer);
memcpy(entry->mac_addr, src, ETH_ALEN);
entry->status = ESI_FORWARD_DIRECT;
entry->last_used = jiffies;
priv->lec_arp_empty_ones = entry->next;
- restore_flags(flags);
+ spin_unlock_irqrestore(&lec_lock, flags);
/* We might have got an entry */
if ((prev=lec_arp_find(priv,src))) {
lec_arp_remove(priv->lec_arp_tables, prev);
@@ -2178,14 +2175,13 @@
lec_arp_unlock(priv);
return;
}
- save_flags(flags);
- cli();
+ spin_lock_irqsave(&lec_lock, flags);
del_timer(&entry->timer);
memcpy(entry->mac_addr, src, ETH_ALEN);
entry->status = ESI_FORWARD_DIRECT;
entry->last_used = jiffies;
prev->next = entry->next;
- restore_flags(flags);
+ spin_unlock_irqresore(&lec_lock, flags);
if ((prev = lec_arp_find(priv, src))) {
lec_arp_remove(priv->lec_arp_tables,prev);
kfree(prev);
Greetings,
Frank Davis <[email protected]> :
[locking in net/atm/lec.c]
net/atm/lec.c::lec_arp_destroy()
-> spin_lock_irqsave(&lec_lock, flags);
-> lec_arp_remove(priv->lec_arp_tables, entry);
-> spin_lock_irqsave(&lec_lock, flags);
-> good bye Charlie !
Both lec_arp_check_expire() (timer function) and lec_arp_destroy() are
walking the lec_arp_table from the 'lec_priv' they were attached to and if
the intent of lec_arp_{lock/unlock} is to protect the timer function
lec_arp_check_expire() by virtue of refcounting, it seems slightly racy.
If you are wondering who the callers of lec_arp_remove() are:
lec_arp_remove
<- lec_atm_send
<- lecdev_ops.send = lec_atm_send
<- lec_arp_destroy
<- lec_atm_close
<- lecdev_ops.close = lec_atm_close
<- lec_arp_check_expire
<- priv->lec_arp_timer.function = lec_arp_check_expire;
<- lec_addr_delete
<- lec_atm_send
<- lec_vcc_close
<- lec_push
<- vcc->push = lec_push;
<- lec_arp_check_empties
<- lec_push
Removing the lock from lec_arp_remove() and asking callers to do themselves
the locking (liek lec_arp_destroy) doesn't seem too unsane.
Btw, del_timer_sync() in lec_arp_destroy() shouldn't hurt imho.
--
Ueimor