* [patch 2.6.13-rc6] net/802/tr: use interrupt-safe locking @ 2005-08-17 20:49 ` John W. Linville 2005-08-17 21:48 ` Jay Vosburgh 2005-08-17 22:00 ` David S. Miller 0 siblings, 2 replies; 5+ messages in thread From: John W. Linville @ 2005-08-17 20:49 UTC (permalink / raw) To: netdev, linux-tr, mikep; +Cc: jgarzik, davem, fubar, linux-kernel Change operations on rif_lock from spin_{un}lock_bh to spin_{un}lock_irq{save,restore} equivalents. Some of the rif_lock critical sections are called from interrupt context via tr_type_trans->tr_add_rif_info. The TR NIC drivers call tr_type_trans from their packet receive handlers. Signed-off-by: Jay Vosburg <foobar@us.ibm.com> Signed-off-by: John W. Linville <linville@tuxdriver.com> --- It is my understanding that this same patch has been submitted multiple times in the past. Some of those submissions were around a year ago, but it does not seem to have been committed. FWIW, this patch is currently being carried in the Fedora and RHEL kernels. It certainly looks like it is necessary to me. Can we get some movement on this? net/802/tr.c | 22 ++++++++++++---------- 1 files changed, 12 insertions(+), 10 deletions(-) diff --git a/net/802/tr.c b/net/802/tr.c --- a/net/802/tr.c +++ b/net/802/tr.c @@ -251,10 +251,11 @@ void tr_source_route(struct sk_buff *skb unsigned int hash; struct rif_cache *entry; unsigned char *olddata; + unsigned long flags; static const unsigned char mcast_func_addr[] = {0xC0,0x00,0x00,0x04,0x00,0x00}; - spin_lock_bh(&rif_lock); + spin_lock_irqsave(&rif_lock, flags); /* * Broadcasts are single route as stated in RFC 1042 @@ -323,7 +324,7 @@ printk("source routing for %02X:%02X:%02 else slack = 18 - ((ntohs(trh->rcf) & TR_RCF_LEN_MASK)>>8); olddata = skb->data; - spin_unlock_bh(&rif_lock); + spin_unlock_irqrestore(&rif_lock, flags); skb_pull(skb, slack); memmove(skb->data, olddata, sizeof(struct trh_hdr) - slack); @@ -337,10 +338,11 @@ printk("source routing for %02X:%02X:%02 static void tr_add_rif_info(struct trh_hdr *trh, struct net_device *dev) { unsigned int hash, rii_p = 0; + unsigned long flags; struct rif_cache *entry; - spin_lock_bh(&rif_lock); + spin_lock_irqsave(&rif_lock, flags); /* * Firstly see if the entry exists @@ -378,7 +380,7 @@ printk("adding rif_entry: addr:%02X:%02X if(!entry) { printk(KERN_DEBUG "tr.c: Couldn't malloc rif cache entry !\n"); - spin_unlock_bh(&rif_lock); + spin_unlock_irqrestore(&rif_lock, flags); return; } @@ -420,7 +422,7 @@ printk("updating rif_entry: addr:%02X:%0 } entry->last_used=jiffies; } - spin_unlock_bh(&rif_lock); + spin_unlock_irqrestore(&rif_lock, flags); } /* @@ -430,9 +432,9 @@ printk("updating rif_entry: addr:%02X:%0 static void rif_check_expire(unsigned long dummy) { int i; - unsigned long next_interval = jiffies + sysctl_tr_rif_timeout/2; + unsigned long flags, next_interval = jiffies + sysctl_tr_rif_timeout/2; - spin_lock_bh(&rif_lock); + spin_lock_irqsave(&rif_lock, flags); for(i =0; i < RIF_TABLE_SIZE; i++) { struct rif_cache *entry, **pentry; @@ -454,7 +456,7 @@ static void rif_check_expire(unsigned lo } } - spin_unlock_bh(&rif_lock); + spin_unlock_irqrestore(&rif_lock, flags); mod_timer(&rif_timer, next_interval); @@ -485,7 +487,7 @@ static struct rif_cache *rif_get_idx(lof static void *rif_seq_start(struct seq_file *seq, loff_t *pos) { - spin_lock_bh(&rif_lock); + spin_lock_irq(&rif_lock); return *pos ? rif_get_idx(*pos - 1) : SEQ_START_TOKEN; } @@ -516,7 +518,7 @@ static void *rif_seq_next(struct seq_fil static void rif_seq_stop(struct seq_file *seq, void *v) { - spin_unlock_bh(&rif_lock); + spin_unlock_irq(&rif_lock); } static int rif_seq_show(struct seq_file *seq, void *v) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch 2.6.13-rc6] net/802/tr: use interrupt-safe locking 2005-08-17 20:49 ` [patch 2.6.13-rc6] net/802/tr: use interrupt-safe locking John W. Linville @ 2005-08-17 21:48 ` Jay Vosburgh 2005-08-18 1:55 ` John W. Linville 2005-08-21 8:38 ` Andrew Morton 2005-08-17 22:00 ` David S. Miller 1 sibling, 2 replies; 5+ messages in thread From: Jay Vosburgh @ 2005-08-17 21:48 UTC (permalink / raw) To: netdev, linux-tr, mikep, jgarzik, davem, linux-kernel John W. Linville <linville@tuxdriver.com> wrote: >Change operations on rif_lock from spin_{un}lock_bh to >spin_{un}lock_irq{save,restore} equivalents. Some of the >rif_lock critical sections are called from interrupt context via >tr_type_trans->tr_add_rif_info. The TR NIC drivers call tr_type_trans >from their packet receive handlers. > >Signed-off-by: Jay Vosburg <foobar@us.ibm.com> Pretty close. Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> >It is my understanding that this same patch has been submitted multiple >times in the past. Some of those submissions were around a year ago, >but it does not seem to have been committed. I believe that I originally wrote and posted this patch in the appended message; I recall posting it a few times in various places. >FWIW, this patch is currently being carried in the Fedora and RHEL >kernels. It certainly looks like it is necessary to me. Can we get >some movement on this? It's in the SuSE kernel as well. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com To: Paul Mackerras <paulus@samba.org> Cc: linux-tr@linuxtr.net, netdev@oss.sgi.com Subject: Re: spin_lock_bh() called in irq handler Date: Wed, 28 Apr 2004 10:55:33 -0700 From: Jay Vosburgh <fubar@us.ibm.com> >I had a look and found that all of the token-ring drivers call >tr_type_trans() at interrupt level. That seems perfectly reasonable >to me. To fix the bug, it seems to me that there are two options: >either move the tr_add_rif_info() call elsewhere (but I have no idea >where) or else use spin_lock_irqsave instead of spin_lock_bh. > >Which is the more appropriate fix? I'm guessing spin_lock_irqsave; would the following be appropriate? I'm not absolutely sure about using spin_(un)lock_irq in rif_seq_start/stop, but it'd be complicated to deal with the flags in that case. I've built this and given it some basic testing, but not really hammered on it. The system doesn't panic when I cat /proc/net/tr_rif, which is a good sign. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com diff -urN linux-2.6.5-orig/net/802/tr.c linux-2.6.5/net/802/tr.c --- linux-2.6.5-orig/net/802/tr.c 2004-04-28 10:02:12.000000000 -0700 +++ linux-2.6.5/net/802/tr.c 2004-04-28 10:15:47.000000000 -0700 @@ -250,10 +250,11 @@ unsigned int hash; struct rif_cache_s *entry; unsigned char *olddata; + unsigned long flags; static const unsigned char mcast_func_addr[] = {0xC0,0x00,0x00,0x04,0x00,0x00}; - spin_lock_bh(&rif_lock); + spin_lock_irqsave(&rif_lock, flags); /* * Broadcasts are single route as stated in RFC 1042 @@ -322,7 +323,7 @@ else slack = 18 - ((ntohs(trh->rcf) & TR_RCF_LEN_MASK)>>8); olddata = skb->data; - spin_unlock_bh(&rif_lock); + spin_unlock_irqrestore(&rif_lock, flags); skb_pull(skb, slack); memmove(skb->data, olddata, sizeof(struct trh_hdr) - slack); @@ -336,10 +337,11 @@ static void tr_add_rif_info(struct trh_hdr *trh, struct net_device *dev) { unsigned int hash, rii_p = 0; + unsigned long flags; struct rif_cache_s *entry; - spin_lock_bh(&rif_lock); + spin_lock_irqsave(&rif_lock, flags); /* * Firstly see if the entry exists @@ -377,7 +379,7 @@ if(!entry) { printk(KERN_DEBUG "tr.c: Couldn't malloc rif cache entry !\n"); - spin_unlock_bh(&rif_lock); + spin_unlock_irqrestore(&rif_lock, flags); return; } @@ -419,7 +421,7 @@ } entry->last_used=jiffies; } - spin_unlock_bh(&rif_lock); + spin_unlock_irqrestore(&rif_lock, flags); } /* @@ -429,9 +431,9 @@ static void rif_check_expire(unsigned long dummy) { int i; - unsigned long next_interval = jiffies + sysctl_tr_rif_timeout/2; + unsigned long flags, next_interval = jiffies + sysctl_tr_rif_timeout/2; - spin_lock_bh(&rif_lock); + spin_lock_irqsave(&rif_lock, flags); for(i =0; i < RIF_TABLE_SIZE; i++) { struct rif_cache_s *entry, **pentry; @@ -453,7 +455,7 @@ } } - spin_unlock_bh(&rif_lock); + spin_unlock_irqrestore(&rif_lock, flags); mod_timer(&rif_timer, next_interval); @@ -484,7 +486,7 @@ static void *rif_seq_start(struct seq_file *seq, loff_t *pos) { - spin_lock_bh(&rif_lock); + spin_lock_irq(&rif_lock); return *pos ? rif_get_idx(*pos - 1) : SEQ_START_TOKEN; } @@ -515,7 +517,7 @@ static void rif_seq_stop(struct seq_file *seq, void *v) { - spin_unlock_bh(&rif_lock); + spin_unlock_irq(&rif_lock); } static int rif_seq_show(struct seq_file *seq, void *v) -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch 2.6.13-rc6] net/802/tr: use interrupt-safe locking 2005-08-17 21:48 ` Jay Vosburgh @ 2005-08-18 1:55 ` John W. Linville 2005-08-21 8:38 ` Andrew Morton 1 sibling, 0 replies; 5+ messages in thread From: John W. Linville @ 2005-08-18 1:55 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev, linux-kernel On Wed, Aug 17, 2005 at 02:48:25PM -0700, Jay Vosburgh wrote: > John W. Linville <linville@tuxdriver.com> wrote: > >Signed-off-by: Jay Vosburg <foobar@us.ibm.com> > > Pretty close. > > Signed-off-by: Jay Vosburgh <fubar@us.ibm.com> Ooops...sorry! Tired, sloppy typing... :-( > I believe that I originally wrote and posted this patch in the > appended message; I recall posting it a few times in various places. That is my understanding as well, which is why I put (an unfortunately incorrect) S-o-b line for you ahead of mine. Should I have done something else? Thanks, John -- John W. Linville linville@tuxdriver.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch 2.6.13-rc6] net/802/tr: use interrupt-safe locking 2005-08-17 21:48 ` Jay Vosburgh 2005-08-18 1:55 ` John W. Linville @ 2005-08-21 8:38 ` Andrew Morton 1 sibling, 0 replies; 5+ messages in thread From: Andrew Morton @ 2005-08-21 8:38 UTC (permalink / raw) To: Jay Vosburgh; +Cc: netdev, linux-tr, mikep, jgarzik, davem, linux-kernel Jay Vosburgh <fubar@us.ibm.com> wrote: > > >FWIW, this patch is currently being carried in the Fedora and RHEL > >kernels. It certainly looks like it is necessary to me. Can we get > >some movement on this? > > It's in the SuSE kernel as well. For how long has this fix been in the vendor kernels? Could someone please tell us why there are unmerged bugfixes in vendor kernels? Are there any more? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [patch 2.6.13-rc6] net/802/tr: use interrupt-safe locking 2005-08-17 20:49 ` [patch 2.6.13-rc6] net/802/tr: use interrupt-safe locking John W. Linville 2005-08-17 21:48 ` Jay Vosburgh @ 2005-08-17 22:00 ` David S. Miller 1 sibling, 0 replies; 5+ messages in thread From: David S. Miller @ 2005-08-17 22:00 UTC (permalink / raw) To: linville; +Cc: netdev, linux-tr, mikep, jgarzik, fubar, linux-kernel From: "John W. Linville" <linville@tuxdriver.com> Date: Wed, 17 Aug 2005 16:49:59 -0400 > Change operations on rif_lock from spin_{un}lock_bh to > spin_{un}lock_irq{save,restore} equivalents. Some of the > rif_lock critical sections are called from interrupt context via > tr_type_trans->tr_add_rif_info. The TR NIC drivers call tr_type_trans > from their packet receive handlers. Applied, I'll try to get this into 2.6.13, but it may have to wait for 2.6.14 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2005-08-21 8:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <linville@tuxdriver.com>
2005-08-17 20:49 ` [patch 2.6.13-rc6] net/802/tr: use interrupt-safe locking John W. Linville
2005-08-17 21:48 ` Jay Vosburgh
2005-08-18 1:55 ` John W. Linville
2005-08-21 8:38 ` Andrew Morton
2005-08-17 22:00 ` David S. Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).