| Message ID | 4AABCF28.6090505@hartkopp.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Saturday 12 September 2009 18:41:12 Oliver Hartkopp wrote: > Michael Buesch wrote: > > >> As there are several users in the kernel do exact this test and call the > >> appropriate netif_rx() function, i would suggest to create a static inline > >> function: > >> > >> static inline int netif_rx_ti(struct sk_buff *skb) > >> { > >> if (in_interrupt()) > >> return netif_rx(skb); > >> return netif_rx_ni(skb); > >> } > >> > >> ('ti' for test in_interrupt()) > >> > >> in include/linux/netdevice.h > >> > >> What do you think about that? > > > > Yeah, I'm fine with that. > > > > Hi Michael, > > i cooked a patch that introduces netif_rx_ti() and fixes up the problems in > mac80211 and the CAN subsystem. > > Currently i'm pondering whether netif_rx_ti() is needed in all cases or if > there are code sections that'll never be executed from irq-context. > > In theses cases netif_rx_ni() should be prefered to netif_rx_ti() to prevent > the obsolete check ... > > Is there any of your changes that should better use netif_rx_ni() ? > > Regards, > Oliver > Well, I'd say this check does not cost much at all. If I were the net maintainer, I'd get rid of netif_rx_ni() _and_ netif_rx_ti() and do the check internally in netif_rx(). But as I don't have to decide that, I just want the mac80211 issue fixed.
Michael Buesch wrote: > On Saturday 12 September 2009 18:41:12 Oliver Hartkopp wrote: >> Michael Buesch wrote: >> >>>> As there are several users in the kernel do exact this test and call the >>>> appropriate netif_rx() function, i would suggest to create a static inline >>>> function: >>>> >>>> static inline int netif_rx_ti(struct sk_buff *skb) >>>> { >>>> if (in_interrupt()) >>>> return netif_rx(skb); >>>> return netif_rx_ni(skb); >>>> } >>>> >>>> ('ti' for test in_interrupt()) >>>> >>>> in include/linux/netdevice.h >>>> >>>> What do you think about that? >>> Yeah, I'm fine with that. >>> >> Hi Michael, >> >> i cooked a patch that introduces netif_rx_ti() and fixes up the problems in >> mac80211 and the CAN subsystem. >> >> Currently i'm pondering whether netif_rx_ti() is needed in all cases or if >> there are code sections that'll never be executed from irq-context. >> >> In theses cases netif_rx_ni() should be prefered to netif_rx_ti() to prevent >> the obsolete check ... >> >> Is there any of your changes that should better use netif_rx_ni() ? >> >> Regards, >> Oliver >> > > Well, I'd say this check does not cost much at all. > If I were the net maintainer, I'd get rid of netif_rx_ni() _and_ netif_rx_ti() and > do the check internally in netif_rx(). > But as I don't have to decide that, I just want the mac80211 issue fixed. > Like this? int netif_rx(struct sk_buff *skb) { int err; if (likely(in_interrupt())) err = __netif_rx(skb); else { preempt_disable(); err = __netif_rx(skb); if (local_softirq_pending()) do_softirq(); preempt_enable(); } return err; } I don't know how expensive in_interrupt() is ... checking the code does not give any answers to *me* ;-) But i found 356 netif_rx() but only 18 netif_rx_ni() in the kernel tree. And three of them check for in_interrupt() before using netif_rx() or netif_rx_ni() ... Finally i would tend to introduce netif_rx_ti() in include/linux/netdevice.h as described above, for the rare code that can be used inside and outside the irq context. I assume the affected code in the CAN stuff has to use netif_rx_ni() - but i will doublecheck that (and prepare a separate CAN patch). For the mac80211 i would suggest to check whether you really need netif_rx()/netif_rx_ni()/netif_rx_ti() in all the regarded cases. I assume always using netif_rx_ti() (as you proposed in the original patch) is not the most efficient approach. Best regards, Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 30 September 2009 16:54:26 Johannes Berg wrote: > On Wed, 2009-09-30 at 17:47 +0300, Kalle Valo wrote: > > > I agree with Michael. The bug is real and I have verified that > > Michael's patch fixes the issue. Better to apply the patch now, it's > > trivial to change the implementation if/when the network stack has > > support for this. > > FWIW, I think in mac80211 the in_interrupt() check can never return true > since we postpone all RX to the tasklet. But the tasklet seems to be ok > -- so should it really be in_interrupt()? I think a tasklet is also in_interrupt(), because it's a softirq. in_interrupt() returns false in process context. The problem appeared when the b43 driver started passing RX frames while being in process context (threaded IRQ). It previously was in tasklet (= softirq) context.
diff --git a/drivers/net/can/vcan.c b/drivers/net/can/vcan.c index 6971f6c..899f3d3 100644 --- a/drivers/net/can/vcan.c +++ b/drivers/net/can/vcan.c @@ -80,7 +80,7 @@ static void vcan_rx(struct sk_buff *skb, struct net_device *dev) skb->dev = dev; skb->ip_summed = CHECKSUM_UNNECESSARY; - netif_rx(skb); + netif_rx_ti(skb); } static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev) diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index a44118b..b34c05d 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -1503,6 +1503,18 @@ extern int netdev_budget; extern void netdev_run_todo(void); /** + * netif_rx_ti - test for irq context and post buffer to the network code + * @skb: buffer to post + * + */ +static inline int netif_rx_ti(struct sk_buff *skb) +{ + if (in_interrupt()) + return netif_rx(skb); + return netif_rx_ni(skb); +} + +/** * dev_put - release reference to device * @dev: network device * diff --git a/net/can/af_can.c b/net/can/af_can.c index ef1c43a..c21e7f4 100644 --- a/net/can/af_can.c +++ b/net/can/af_can.c @@ -278,7 +278,7 @@ int can_send(struct sk_buff *skb, int loop) } if (newskb) - netif_rx(newskb); + netif_rx_ti(newskb); /* update statistics */ can_stats.tx_frames++; diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 5608f6c..bbcb4cb 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -606,7 +606,7 @@ static void ieee80211_send_layer2_update(struct sta_info *sta) skb->dev = sta->sdata->dev; skb->protocol = eth_type_trans(skb, sta->sdata->dev); memset(skb->cb, 0, sizeof(skb->cb)); - netif_rx(skb); + netif_rx_ti(skb); } static void sta_apply_parameters(struct ieee80211_local *local, diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 797f539..1109f99 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -591,7 +591,7 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb) skb2 = skb_clone(skb, GFP_ATOMIC); if (skb2) { skb2->dev = prev_dev; - netif_rx(skb2); + netif_rx_ti(skb2); } } @@ -600,7 +600,7 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb) } if (prev_dev) { skb->dev = prev_dev; - netif_rx(skb); + netif_rx_ti(skb); skb = NULL; } rcu_read_unlock(); diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index c01588f..5bb7c04 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -309,7 +309,7 @@ ieee80211_rx_monitor(struct ieee80211_local *local, struct sk_buff *origskb, skb2 = skb_clone(skb, GFP_ATOMIC); if (skb2) { skb2->dev = prev_dev; - netif_rx(skb2); + netif_rx_ti(skb2); } } @@ -320,7 +320,7 @@ ieee80211_rx_monitor(struct ieee80211_local *local, struct sk_buff *origskb, if (prev_dev) { skb->dev = prev_dev; - netif_rx(skb); + netif_rx_ti(skb); } else dev_kfree_skb(skb); @@ -1349,7 +1349,7 @@ ieee80211_deliver_skb(struct ieee80211_rx_data *rx) /* deliver to local stack */ skb->protocol = eth_type_trans(skb, dev); memset(skb->cb, 0, sizeof(skb->cb)); - netif_rx(skb); + netif_rx_ti(skb); } } @@ -1943,7 +1943,7 @@ static void ieee80211_rx_cooked_monitor(struct ieee80211_rx_data *rx) skb2 = skb_clone(skb, GFP_ATOMIC); if (skb2) { skb2->dev = prev_dev; - netif_rx(skb2); + netif_rx_ti(skb2); } } @@ -1954,7 +1954,7 @@ static void ieee80211_rx_cooked_monitor(struct ieee80211_rx_data *rx) if (prev_dev) { skb->dev = prev_dev; - netif_rx(skb); + netif_rx_ti(skb); skb = NULL; } else goto out_free_skb;