| Message ID | 1392237482-18172-1-git-send-email-johannes@sipsolutions.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Wed, Feb 12, 2014 at 10:38 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > From: Johannes Berg <johannes.berg@intel.com> > > NAPI was originally added to mac80211 a long time ago (by John in > commit 4e6cbfd09c66 in July 2010), but then removed years later > (by Stanislaw in commit 30c97120c6c7 in February 2013). No driver > ever used it, so that was fine. > > Now I'm adding support for NAPI to our driver, so add some code > to mac80211 again to support NAPI. John was originally wrapping > some (but not nearly all NAPI-related functions), but that doesn't > scale very well with the number of functions that are there, some > of which are even only inlines. Thus, instead of doing that, let > the drivers manage the NAPI struct, except for napi_add() which is > needed so mac80211 knows how to call napi_gro_receive(). > > Also remove some no longer needed definitions that were left when > NAPI support was removed. > > Reviewed-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com> > Reviewed-by: Eyal Shapira <eyal@wizery.com> > Signed-off-by: Johannes Berg <johannes.berg@intel.com> > --- > include/net/mac80211.h | 34 +++++++++++++--------------------- > net/mac80211/ieee80211_i.h | 2 ++ > net/mac80211/main.c | 12 ++++++++++++ > net/mac80211/rx.c | 5 ++++- > 4 files changed, 31 insertions(+), 22 deletions(-) > > diff --git a/include/net/mac80211.h b/include/net/mac80211.h > index 4005c5b..2d4d312 100644 > --- a/include/net/mac80211.h > +++ b/include/net/mac80211.h > @@ -1642,10 +1642,6 @@ enum ieee80211_hw_flags { > * the hw can report back. > * @max_rate_tries: maximum number of tries for each stage > * > - * @napi_weight: weight used for NAPI polling. You must specify an > - * appropriate value here if a napi_poll operation is provided > - * by your driver. > - * > * @max_rx_aggregation_subframes: maximum buffer size (number of > * sub-frames) to be used for A-MPDU block ack receiver > * aggregation. > @@ -1699,7 +1695,6 @@ struct ieee80211_hw { > int vif_data_size; > int sta_data_size; > int chanctx_data_size; > - int napi_weight; > u16 queues; > u16 max_listen_interval; > s8 max_signal; > @@ -2622,8 +2617,6 @@ enum ieee80211_roc_type { > * callback. They must then call ieee80211_chswitch_done() to indicate > * completion of the channel switch. > * > - * @napi_poll: Poll Rx queue for incoming data frames. > - * > * @set_antenna: Set antenna configuration (tx_ant, rx_ant) on the device. > * Parameters are bitmaps of allowed antennas to use for TX/RX. Drivers may > * reject TX/RX mask combinations they cannot support by returning -EINVAL > @@ -2882,7 +2875,6 @@ struct ieee80211_ops { > void (*flush)(struct ieee80211_hw *hw, u32 queues, bool drop); > void (*channel_switch)(struct ieee80211_hw *hw, > struct ieee80211_channel_switch *ch_switch); > - int (*napi_poll)(struct ieee80211_hw *hw, int budget); > int (*set_antenna)(struct ieee80211_hw *hw, u32 tx_ant, u32 rx_ant); > int (*get_antenna)(struct ieee80211_hw *hw, u32 *tx_ant, u32 *rx_ant); > > @@ -3164,21 +3156,21 @@ void ieee80211_free_hw(struct ieee80211_hw *hw); > */ > void ieee80211_restart_hw(struct ieee80211_hw *hw); > > -/** ieee80211_napi_schedule - schedule NAPI poll > - * > - * Use this function to schedule NAPI polling on a device. > - * > - * @hw: the hardware to start polling > - */ > -void ieee80211_napi_schedule(struct ieee80211_hw *hw); > - > -/** ieee80211_napi_complete - complete NAPI polling > - * > - * Use this function to finish NAPI polling on a device. > +/** > + * ieee80211_napi_add - initialize mac80211 NAPI context > + * @hw: the hardware to initialize the NAPI context on > + * @napi: the NAPI context to initialize > + * @napi_dev: dummy NAPI netdevice, here to not waste the space if the > + * driver doesn't use NAPI > + * @poll: poll function > + * @weight: default weight > * > - * @hw: the hardware to stop polling > + * See also netif_napi_add(). > */ > -void ieee80211_napi_complete(struct ieee80211_hw *hw); > +void ieee80211_napi_add(struct ieee80211_hw *hw, struct napi_struct *napi, > + struct net_device *napi_dev, > + int (*poll)(struct napi_struct *, int), > + int weight); > > /** > * ieee80211_rx - receive frame > diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h > index 0014b53..8603dfb 100644 > --- a/net/mac80211/ieee80211_i.h > +++ b/net/mac80211/ieee80211_i.h > @@ -1242,6 +1242,8 @@ struct ieee80211_local { > > struct ieee80211_sub_if_data __rcu *p2p_sdata; > > + struct napi_struct *napi; > + > /* virtual monitor interface */ > struct ieee80211_sub_if_data __rcu *monitor_sdata; > struct cfg80211_chan_def monitor_chandef; > diff --git a/net/mac80211/main.c b/net/mac80211/main.c > index 1f7d842..b055f6a5 100644 > --- a/net/mac80211/main.c > +++ b/net/mac80211/main.c > @@ -1076,6 +1076,18 @@ int ieee80211_register_hw(struct ieee80211_hw *hw) > } > EXPORT_SYMBOL(ieee80211_register_hw); > > +void ieee80211_napi_add(struct ieee80211_hw *hw, struct napi_struct *napi, > + struct net_device *napi_dev, > + int (*poll)(struct napi_struct *, int), > + int weight) > +{ > + struct ieee80211_local *local = hw_to_local(hw); > + > + netif_napi_add(napi_dev, napi, poll, weight); > + local->napi = napi; I'm not really familiar with NAPI, but shouldn't netif_napi_del be called at some point? And if so, it will leave mac80211 in an inconsistent state. Maybe if we make it part of ieee80211_register/unregister_hw the scoping of local->napi will be nicer? Arik -- 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 Thu, 2014-02-13 at 10:55 +0200, Arik Nemtsov wrote: > > + netif_napi_add(napi_dev, napi, poll, weight); > > + local->napi = napi; > > I'm not really familiar with NAPI, but shouldn't netif_napi_del be > called at some point? And if so, it will leave mac80211 in an > inconsistent state. Oops. I totally forgot about that, since we use a fake netdev it doesn't seem to have hurt, but it could leak memory. > Maybe if we make it part of ieee80211_register/unregister_hw the > scoping of local->napi will be nicer? But then we'd have to have a whole bunch of new arguments to _register_hw(), would we really want to go there? I don't think so - also in iwlwifi we'd have to go through some contortions to do that (rather than just call this function we'd have to store the stuff coming in from below.) I think we can just require that the driver calls netif_napi_del() itself, and for now it'd only be allowed to do it after unregister_hw(), but that seems reasonable, no? johannes -- 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 Thu, Feb 13, 2014 at 11:06 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2014-02-13 at 10:55 +0200, Arik Nemtsov wrote: > >> > + netif_napi_add(napi_dev, napi, poll, weight); >> > + local->napi = napi; >> >> I'm not really familiar with NAPI, but shouldn't netif_napi_del be >> called at some point? And if so, it will leave mac80211 in an >> inconsistent state. > > Oops. I totally forgot about that, since we use a fake netdev it doesn't > seem to have hurt, but it could leak memory. > >> Maybe if we make it part of ieee80211_register/unregister_hw the >> scoping of local->napi will be nicer? > > But then we'd have to have a whole bunch of new arguments to > _register_hw(), would we really want to go there? I don't think so - > also in iwlwifi we'd have to go through some contortions to do that > (rather than just call this function we'd have to store the stuff coming > in from below.) > > I think we can just require that the driver calls netif_napi_del() > itself, and for now it'd only be allowed to do it after unregister_hw(), > but that seems reasonable, no? It's fine. But I guess it's nicer to add a small ieee80211 wrapper for it that also clears local->napi? not sure. In general can we even add/remove napi when an interface is up or is it a bad idea? if it is, maybe it should be warned/blocked? I guess the scoping wasn't really clear for me, but maybe it is for anyone familiar with napi. -- 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 Thu, 2014-02-13 at 12:00 +0200, Arik Nemtsov wrote: > It's fine. But I guess it's nicer to add a small ieee80211 wrapper for > it that also clears local->napi? not sure. Well if the whole thing is going down there's little point in that. > In general can we even add/remove napi when an interface is up or is > it a bad idea? if it is, maybe it should be warned/blocked? I guess > the scoping wasn't really clear for me, but maybe it is for anyone > familiar with napi. It seems that isn't allowed, the locking would seem rather missing for that? *Maybe* if the driver locks everything, but even then I'd be wary. johannes -- 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 Thu, Feb 13, 2014 at 12:03 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2014-02-13 at 12:00 +0200, Arik Nemtsov wrote: > >> It's fine. But I guess it's nicer to add a small ieee80211 wrapper for >> it that also clears local->napi? not sure. > > Well if the whole thing is going down there's little point in that. Sure. Arik -- 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
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 4005c5b..2d4d312 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1642,10 +1642,6 @@ enum ieee80211_hw_flags { * the hw can report back. * @max_rate_tries: maximum number of tries for each stage * - * @napi_weight: weight used for NAPI polling. You must specify an - * appropriate value here if a napi_poll operation is provided - * by your driver. - * * @max_rx_aggregation_subframes: maximum buffer size (number of * sub-frames) to be used for A-MPDU block ack receiver * aggregation. @@ -1699,7 +1695,6 @@ struct ieee80211_hw { int vif_data_size; int sta_data_size; int chanctx_data_size; - int napi_weight; u16 queues; u16 max_listen_interval; s8 max_signal; @@ -2622,8 +2617,6 @@ enum ieee80211_roc_type { * callback. They must then call ieee80211_chswitch_done() to indicate * completion of the channel switch. * - * @napi_poll: Poll Rx queue for incoming data frames. - * * @set_antenna: Set antenna configuration (tx_ant, rx_ant) on the device. * Parameters are bitmaps of allowed antennas to use for TX/RX. Drivers may * reject TX/RX mask combinations they cannot support by returning -EINVAL @@ -2882,7 +2875,6 @@ struct ieee80211_ops { void (*flush)(struct ieee80211_hw *hw, u32 queues, bool drop); void (*channel_switch)(struct ieee80211_hw *hw, struct ieee80211_channel_switch *ch_switch); - int (*napi_poll)(struct ieee80211_hw *hw, int budget); int (*set_antenna)(struct ieee80211_hw *hw, u32 tx_ant, u32 rx_ant); int (*get_antenna)(struct ieee80211_hw *hw, u32 *tx_ant, u32 *rx_ant); @@ -3164,21 +3156,21 @@ void ieee80211_free_hw(struct ieee80211_hw *hw); */ void ieee80211_restart_hw(struct ieee80211_hw *hw); -/** ieee80211_napi_schedule - schedule NAPI poll - * - * Use this function to schedule NAPI polling on a device. - * - * @hw: the hardware to start polling - */ -void ieee80211_napi_schedule(struct ieee80211_hw *hw); - -/** ieee80211_napi_complete - complete NAPI polling - * - * Use this function to finish NAPI polling on a device. +/** + * ieee80211_napi_add - initialize mac80211 NAPI context + * @hw: the hardware to initialize the NAPI context on + * @napi: the NAPI context to initialize + * @napi_dev: dummy NAPI netdevice, here to not waste the space if the + * driver doesn't use NAPI + * @poll: poll function + * @weight: default weight * - * @hw: the hardware to stop polling + * See also netif_napi_add(). */ -void ieee80211_napi_complete(struct ieee80211_hw *hw); +void ieee80211_napi_add(struct ieee80211_hw *hw, struct napi_struct *napi, + struct net_device *napi_dev, + int (*poll)(struct napi_struct *, int), + int weight); /** * ieee80211_rx - receive frame diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 0014b53..8603dfb 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -1242,6 +1242,8 @@ struct ieee80211_local { struct ieee80211_sub_if_data __rcu *p2p_sdata; + struct napi_struct *napi; + /* virtual monitor interface */ struct ieee80211_sub_if_data __rcu *monitor_sdata; struct cfg80211_chan_def monitor_chandef; diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 1f7d842..b055f6a5 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -1076,6 +1076,18 @@ int ieee80211_register_hw(struct ieee80211_hw *hw) } EXPORT_SYMBOL(ieee80211_register_hw); +void ieee80211_napi_add(struct ieee80211_hw *hw, struct napi_struct *napi, + struct net_device *napi_dev, + int (*poll)(struct napi_struct *, int), + int weight) +{ + struct ieee80211_local *local = hw_to_local(hw); + + netif_napi_add(napi_dev, napi, poll, weight); + local->napi = napi; +} +EXPORT_SYMBOL_GPL(ieee80211_napi_add); + void ieee80211_unregister_hw(struct ieee80211_hw *hw) { struct ieee80211_local *local = hw_to_local(hw); diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c index 5930621..58e4b70 100644 --- a/net/mac80211/rx.c +++ b/net/mac80211/rx.c @@ -1954,7 +1954,10 @@ 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_receive_skb(skb); + if (rx->local->napi) + napi_gro_receive(rx->local->napi, skb); + else + netif_receive_skb(skb); } if (xmit_skb) {