| Message ID | 1531225597-4736-1-git-send-email-mpubbise@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h > index 172aeae..d07f7f9 100644 > --- a/net/mac80211/ieee80211_i.h > +++ b/net/mac80211/ieee80211_i.h > @@ -818,6 +818,7 @@ enum txq_info_flags { > IEEE80211_TXQ_STOP, > IEEE80211_TXQ_AMPDU, > IEEE80211_TXQ_NO_AMSDU, > + IEEE80211_TXQ_PAUSED, > }; I think it would be a good idea to either rename the flags, or at least add an explanation somewhere of the difference between a paused and a stopped queue... > /** > @@ -1226,6 +1227,7 @@ struct ieee80211_local { > > struct sk_buff_head pending[IEEE80211_MAX_QUEUES]; > struct tasklet_struct tx_pending_tasklet; > + struct tasklet_struct wake_txqs_tasklet; It's not quite clear to me why a tasklet is needed? Couldn't you just call the ieee80211_wake_txqs() function at the same place where you currently schedule the tasklet? -Toke
On 7/10/2018 6:28 PM, Toke Høiland-Jørgensen wrote: >> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h >> index 172aeae..d07f7f9 100644 >> --- a/net/mac80211/ieee80211_i.h >> +++ b/net/mac80211/ieee80211_i.h >> @@ -818,6 +818,7 @@ enum txq_info_flags { >> IEEE80211_TXQ_STOP, >> IEEE80211_TXQ_AMPDU, >> IEEE80211_TXQ_NO_AMSDU, >> + IEEE80211_TXQ_PAUSED, >> }; > I think it would be a good idea to either rename the flags, or at least > add an explanation somewhere of the difference between a paused and a > stopped queue... Initially, the idea was to use IEEE80211_TXQ_STOP flag to indicate that iTXQs are stopped; since this flag was used in the aggregation code, I was unsure whether the same flag can be used to indicate the iTXQ stop condition. I could not find any better name for this:-). Johannes, any thought? >> /** >> @@ -1226,6 +1227,7 @@ struct ieee80211_local { >> >> struct sk_buff_head pending[IEEE80211_MAX_QUEUES]; >> struct tasklet_struct tx_pending_tasklet; >> + struct tasklet_struct wake_txqs_tasklet; > It's not quite clear to me why a tasklet is needed? Couldn't you just > call the ieee80211_wake_txqs() function at the same place where you > currently schedule the tasklet? Since driver can also invoke wake_queues() operation; there can be a possible deadlock situation. At least, in ath10k there was deadlock on the "htt->tx_lock"; wake_queues() is invoked by taking the tx_lock and the same lock is acquired again in wake_tx_queue(). Also, by deferring wake_txqs(), drivers can safely invoke wake_queues(). -Manikanta
Manikanta Pubbisetty <mpubbise@codeaurora.org> writes: > On 7/10/2018 6:28 PM, Toke Høiland-Jørgensen wrote: > >>> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h >>> index 172aeae..d07f7f9 100644 >>> --- a/net/mac80211/ieee80211_i.h >>> +++ b/net/mac80211/ieee80211_i.h >>> @@ -818,6 +818,7 @@ enum txq_info_flags { >>> IEEE80211_TXQ_STOP, >>> IEEE80211_TXQ_AMPDU, >>> IEEE80211_TXQ_NO_AMSDU, >>> + IEEE80211_TXQ_PAUSED, >>> }; >> I think it would be a good idea to either rename the flags, or at least >> add an explanation somewhere of the difference between a paused and a >> stopped queue... > > Initially, the idea was to use IEEE80211_TXQ_STOP flag to indicate that > iTXQs are stopped; since this flag was used in the aggregation code, I > was unsure whether the same flag can be used to indicate the iTXQ stop > condition. > I could not find any better name for this:-). Hmm, yeah, not sure whether the two code paths can stomp on each other if you reuse the flag. It would be neat to be able to reuse it, though... Otherwise, how about renaming the old one to _STOP_AGGR and calling the new one _STOP_NETIF or something? >>> /** >>> @@ -1226,6 +1227,7 @@ struct ieee80211_local { >>> >>> struct sk_buff_head pending[IEEE80211_MAX_QUEUES]; >>> struct tasklet_struct tx_pending_tasklet; >>> + struct tasklet_struct wake_txqs_tasklet; >> It's not quite clear to me why a tasklet is needed? Couldn't you just >> call the ieee80211_wake_txqs() function at the same place where you >> currently schedule the tasklet? > > Since driver can also invoke wake_queues() operation; there can be a > possible deadlock situation. At least, in ath10k there was deadlock on > the "htt->tx_lock"; wake_queues() is invoked by taking the tx_lock and > the same lock is acquired again in wake_tx_queue(). > Also, by deferring wake_txqs(), drivers can safely invoke wake_queues(). Right, that makes sense :) -Toke
Hi Manikanta,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on mac80211-next/master]
[also build test WARNING on v4.18-rc4 next-20180710]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Manikanta-Pubbisetty/mac80211-add-stop-start-logic-for-software-TXQs/20180710-202808
base: https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git master
reproduce: make htmldocs
All warnings (new ones prefixed by >>):
WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
mm/mempool.c:228: warning: Function parameter or member 'pool' not described in 'mempool_init'
include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.ibss' not described in 'wireless_dev'
include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.connect' not described in 'wireless_dev'
include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.keys' not described in 'wireless_dev'
include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.ie' not described in 'wireless_dev'
include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.ie_len' not described in 'wireless_dev'
include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.bssid' not described in 'wireless_dev'
include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.ssid' not described in 'wireless_dev'
include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.default_key' not described in 'wireless_dev'
include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.default_mgmt_key' not described in 'wireless_dev'
include/net/cfg80211.h:4381: warning: Function parameter or member 'wext.prev_bssid_valid' not described in 'wireless_dev'
include/net/mac80211.h:2332: warning: Function parameter or member 'radiotap_timestamp.units_pos' not described in 'ieee80211_hw'
include/net/mac80211.h:2332: warning: Function parameter or member 'radiotap_timestamp.accuracy' not described in 'ieee80211_hw'
>> include/net/mac80211.h:1537: warning: Function parameter or member 'txqs_stopped' not described in 'ieee80211_vif'
include/net/mac80211.h:977: warning: Function parameter or member 'control.rates' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'control.rts_cts_rate_idx' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'control.use_rts' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'control.use_cts_prot' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'control.short_preamble' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'control.skip_table' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'control.jiffies' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'control.vif' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'control.hw_key' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'control.flags' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'control.enqueue_time' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'ack' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'ack.cookie' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'status.rates' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'status.ack_signal' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'status.ampdu_ack_len' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'status.ampdu_len' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'status.antenna' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'status.tx_time' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'status.is_valid_ack_signal' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'status.status_driver_data' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'driver_rates' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'pad' not described in 'ieee80211_tx_info'
include/net/mac80211.h:977: warning: Function parameter or member 'rate_driver_data' not described in 'ieee80211_tx_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.signal' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_retries' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_failed' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack_signal' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.ack_signal_filled' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.avg_ack_signal' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info'
net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.msdu' not described in 'sta_info'
kernel/sched/fair.c:3760: warning: Function parameter or member 'flags' not described in 'attach_entity_load_avg'
include/linux/device.h:93: warning: bad line: this bus.
include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.cb' not described in 'dma_buf'
include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.poll' not described in 'dma_buf'
include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.active' not described in 'dma_buf'
include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.cb' not described in 'dma_buf'
include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.poll' not described in 'dma_buf'
include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.active' not described in 'dma_buf'
include/linux/dma-fence-array.h:54: warning: Function parameter or member 'work' not described in 'dma_fence_array'
include/linux/gpio/driver.h:142: warning: Function parameter or member 'request_key' not described in 'gpio_irq_chip'
include/linux/iio/hw-consumer.h:1: warning: no structured comments found
include/linux/device.h:94: warning: bad line: this bus.
include/linux/input/sparse-keymap.h:46: warning: Function parameter or member 'sw' not described in 'key_entry'
include/linux/regulator/driver.h:227: warning: Function parameter or member 'resume_early' not described in 'regulator_ops'
drivers/regulator/core.c:4465: warning: Excess function parameter 'state' description in 'regulator_suspend_late'
arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw0' not described in 'irb'
arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw1' not described in 'irb'
arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw2' not described in 'irb'
arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw3' not described in 'irb'
arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.eadm' not described in 'irb'
drivers/usb/dwc3/gadget.c:510: warning: Excess function parameter 'dwc' description in 'dwc3_gadget_start_config'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver'
include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver'
drivers/gpu/drm/i915/i915_vma.h:48: warning: cannot understand function prototype: 'struct i915_vma '
drivers/gpu/drm/i915/i915_vma.h:1: warning: no structured comments found
include/drm/tinydrm/tinydrm.h:34: warning: Function parameter or member 'fb_dirty' not described in 'tinydrm_device'
drivers/gpu/drm/tinydrm/mipi-dbi.c:272: warning: Function parameter or member 'crtc_state' not described in 'mipi_dbi_enable_flush'
drivers/gpu/drm/tinydrm/mipi-dbi.c:272: warning: Function parameter or member 'plane_state' not described in 'mipi_dbi_enable_flush'
include/linux/skbuff.h:853: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'list' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'skb_mstamp' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member '__unused' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'pfmemalloc' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'offload_fwd_mark' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'offload_mr_fwd_mark' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
include/linux/skbuff.h:853: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
include/net/sock.h:238: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
vim +1537 include/net/mac80211.h
ddbfe860 Stanislaw Gruszka 2013-03-08 1530
1b09b556 Andrei Otcheretianski 2015-08-15 1531 unsigned int probe_req_reg;
1b09b556 Andrei Otcheretianski 2015-08-15 1532
17fd7be6 Manikanta Pubbisetty 2018-07-10 1533 bool txqs_stopped[IEEE80211_NUM_ACS];
17fd7be6 Manikanta Pubbisetty 2018-07-10 1534
32bfd35d Johannes Berg 2007-12-19 1535 /* must be last */
1c06ef98 Johannes Berg 2012-12-28 1536 u8 drv_priv[0] __aligned(sizeof(void *));
32bfd35d Johannes Berg 2007-12-19 @1537 };
32bfd35d Johannes Berg 2007-12-19 1538
:::::: The code at line 1537 was first introduced by commit
:::::: 32bfd35d4b63bd63de4bb0d791ef049c3c868726 mac80211: dont use interface indices in drivers
:::::: TO: Johannes Berg <johannes@sipsolutions.net>
:::::: CC: David S. Miller <davem@davemloft.net>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
>>> include/net/mac80211.h:1537: warning: Function parameter or member 'txqs_stopped' not described in 'ieee80211_vif' > Described txq_stopped instead of txqs_stopped; will fix this in the next version. Thanks, Manikanta
On 7/10/2018 8:52 PM, Toke Høiland-Jørgensen wrote: > Manikanta Pubbisetty <mpubbise@codeaurora.org> writes: > >> On 7/10/2018 6:28 PM, Toke Høiland-Jørgensen wrote: >> >>>> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h >>>> index 172aeae..d07f7f9 100644 >>>> --- a/net/mac80211/ieee80211_i.h >>>> +++ b/net/mac80211/ieee80211_i.h >>>> @@ -818,6 +818,7 @@ enum txq_info_flags { >>>> IEEE80211_TXQ_STOP, >>>> IEEE80211_TXQ_AMPDU, >>>> IEEE80211_TXQ_NO_AMSDU, >>>> + IEEE80211_TXQ_PAUSED, >>>> }; >>> I think it would be a good idea to either rename the flags, or at least >>> add an explanation somewhere of the difference between a paused and a >>> stopped queue... >> Initially, the idea was to use IEEE80211_TXQ_STOP flag to indicate that >> iTXQs are stopped; since this flag was used in the aggregation code, I >> was unsure whether the same flag can be used to indicate the iTXQ stop >> condition. >> I could not find any better name for this:-). > Hmm, yeah, not sure whether the two code paths can stomp on each other > if you reuse the flag. It would be neat to be able to reuse it, though... > > Otherwise, how about renaming the old one to _STOP_AGGR and calling the > new one _STOP_NETIF or something? These ones are much better, thanks toke!! I would probably like to extend the name to _STOP_NETIF_TX; how about keeping the old one as is and renaming _TXQ_PAUSED to _TXQ_STOP_NETIF_TX ? -Manikanta
Manikanta Pubbisetty <mpubbise@codeaurora.org> writes: > On 7/10/2018 8:52 PM, Toke Høiland-Jørgensen wrote: >> Manikanta Pubbisetty <mpubbise@codeaurora.org> writes: >> >>> On 7/10/2018 6:28 PM, Toke Høiland-Jørgensen wrote: >>> >>>>> diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h >>>>> index 172aeae..d07f7f9 100644 >>>>> --- a/net/mac80211/ieee80211_i.h >>>>> +++ b/net/mac80211/ieee80211_i.h >>>>> @@ -818,6 +818,7 @@ enum txq_info_flags { >>>>> IEEE80211_TXQ_STOP, >>>>> IEEE80211_TXQ_AMPDU, >>>>> IEEE80211_TXQ_NO_AMSDU, >>>>> + IEEE80211_TXQ_PAUSED, >>>>> }; >>>> I think it would be a good idea to either rename the flags, or at least >>>> add an explanation somewhere of the difference between a paused and a >>>> stopped queue... >>> Initially, the idea was to use IEEE80211_TXQ_STOP flag to indicate that >>> iTXQs are stopped; since this flag was used in the aggregation code, I >>> was unsure whether the same flag can be used to indicate the iTXQ stop >>> condition. >>> I could not find any better name for this:-). >> Hmm, yeah, not sure whether the two code paths can stomp on each other >> if you reuse the flag. It would be neat to be able to reuse it, though... >> >> Otherwise, how about renaming the old one to _STOP_AGGR and calling the >> new one _STOP_NETIF or something? > > These ones are much better, thanks toke!! > I would probably like to extend the name to _STOP_NETIF_TX; how about > keeping the old one as is and renaming _TXQ_PAUSED to > _TXQ_STOP_NETIF_TX ? Sure, that works as well :) -Toke
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 5790f55..3d9ec07 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1504,6 +1504,8 @@ enum ieee80211_vif_flags { * @drv_priv: data area for driver use, will always be aligned to * sizeof(void \*). * @txq: the multicast data TX queue (if driver uses the TXQ abstraction) + * @txq_stopped: per AC flag to indicate that intermediate TXQs are stopped, + * protected by fq->lock. */ struct ieee80211_vif { enum nl80211_iftype type; @@ -1528,6 +1530,8 @@ struct ieee80211_vif { unsigned int probe_req_reg; + bool txqs_stopped[IEEE80211_NUM_ACS]; + /* must be last */ u8 drv_priv[0] __aligned(sizeof(void *)); }; diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 172aeae..d07f7f9 100644 --- a/net/mac80211/ieee80211_i.h +++ b/net/mac80211/ieee80211_i.h @@ -818,6 +818,7 @@ enum txq_info_flags { IEEE80211_TXQ_STOP, IEEE80211_TXQ_AMPDU, IEEE80211_TXQ_NO_AMSDU, + IEEE80211_TXQ_PAUSED, }; /** @@ -1226,6 +1227,7 @@ struct ieee80211_local { struct sk_buff_head pending[IEEE80211_MAX_QUEUES]; struct tasklet_struct tx_pending_tasklet; + struct tasklet_struct wake_txqs_tasklet; atomic_t agg_queue_stop[IEEE80211_MAX_QUEUES]; @@ -2038,6 +2040,7 @@ void ieee80211_txq_remove_vlan(struct ieee80211_local *local, struct ieee80211_sub_if_data *sdata); void ieee80211_fill_txq_stats(struct cfg80211_txq_stats *txqstats, struct txq_info *txqi); +void ieee80211_wake_txqs(unsigned long data); void ieee80211_send_auth(struct ieee80211_sub_if_data *sdata, u16 transaction, u16 auth_alg, u16 status, const u8 *extra, size_t extra_len, const u8 *bssid, diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 4fb2709..e121541 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -668,6 +668,10 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len, tasklet_init(&local->tx_pending_tasklet, ieee80211_tx_pending, (unsigned long)local); + if (ops->wake_tx_queue) + tasklet_init(&local->wake_txqs_tasklet, ieee80211_wake_txqs, + (unsigned long)local); + tasklet_init(&local->tasklet, ieee80211_tasklet_handler, (unsigned long) local); diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index cd332e3..d593eb8 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -3466,13 +3466,19 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw, struct ieee80211_tx_info *info; struct ieee80211_tx_data tx; ieee80211_tx_result r; - struct ieee80211_vif *vif; + struct ieee80211_vif *vif = txq->vif; spin_lock_bh(&fq->lock); - if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags)) + if (test_bit(IEEE80211_TXQ_STOP, &txqi->flags) || + test_bit(IEEE80211_TXQ_PAUSED, &txqi->flags)) goto out; + if (vif->txqs_stopped[ieee80211_ac_from_tid(txq->tid)]) { + set_bit(IEEE80211_TXQ_PAUSED, &txqi->flags); + goto out; + } + /* Make sure fragments stay together. */ skb = __skb_dequeue(&txqi->frags); if (skb) @@ -3567,6 +3573,7 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw, } IEEE80211_SKB_CB(skb)->control.vif = vif; + out: spin_unlock_bh(&fq->lock); diff --git a/net/mac80211/util.c b/net/mac80211/util.c index 3e68132..aabbc44 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -240,6 +240,99 @@ __le16 ieee80211_ctstoself_duration(struct ieee80211_hw *hw, } EXPORT_SYMBOL(ieee80211_ctstoself_duration); +static void __ieee80211_wake_txqs(struct ieee80211_sub_if_data *sdata, int ac) +{ + struct ieee80211_local *local = sdata->local; + struct ieee80211_vif *vif = &sdata->vif; + struct fq *fq = &local->fq; + struct ps_data *ps = NULL; + struct txq_info *txqi; + struct sta_info *sta; + int i; + + spin_lock_bh(&fq->lock); + + if (sdata->vif.type == NL80211_IFTYPE_AP) + ps = &sdata->bss->ps; + + sdata->vif.txqs_stopped[ac] = false; + + list_for_each_entry_rcu(sta, &local->sta_list, list) { + if (sdata != sta->sdata) + continue; + + for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) { + struct ieee80211_txq *txq = sta->sta.txq[i]; + + txqi = to_txq_info(txq); + + if (ac != txq->ac) + continue; + + if (!test_and_clear_bit(IEEE80211_TXQ_PAUSED, + &txqi->flags)) + continue; + + spin_unlock_bh(&fq->lock); + drv_wake_tx_queue(local, txqi); + spin_lock_bh(&fq->lock); + } + } + + if (!vif->txq) + goto out; + + txqi = to_txq_info(vif->txq); + + if (!test_and_clear_bit(IEEE80211_TXQ_PAUSED, &txqi->flags) || + (ps && atomic_read(&ps->num_sta_ps)) || ac != vif->txq->ac) + goto out; + + spin_unlock_bh(&fq->lock); + + drv_wake_tx_queue(local, txqi); + return; +out: + spin_unlock_bh(&fq->lock); +} + +void ieee80211_wake_txqs(unsigned long data) +{ + struct ieee80211_local *local = (struct ieee80211_local *)data; + struct ieee80211_sub_if_data *sdata; + int n_acs = IEEE80211_NUM_ACS; + unsigned long flags; + int i; + + rcu_read_lock(); + spin_lock_irqsave(&local->queue_stop_reason_lock, flags); + + if (local->hw.queues < IEEE80211_NUM_ACS) + n_acs = 1; + + for (i = 0; i < local->hw.queues; i++) { + if (local->queue_stop_reasons[i]) + continue; + + spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags); + list_for_each_entry_rcu(sdata, &local->interfaces, list) { + int ac; + + for (ac = 0; ac < n_acs; ac++) { + int ac_queue = sdata->vif.hw_queue[ac]; + + if (ac_queue == i || + sdata->vif.cab_queue == i) + __ieee80211_wake_txqs(sdata, ac); + } + } + spin_lock_irqsave(&local->queue_stop_reason_lock, flags); + } + + spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags); + rcu_read_unlock(); +} + void ieee80211_propagate_queue_wake(struct ieee80211_local *local, int queue) { struct ieee80211_sub_if_data *sdata; @@ -308,6 +401,9 @@ static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue, rcu_read_unlock(); } else tasklet_schedule(&local->tx_pending_tasklet); + + if (local->ops->wake_tx_queue) + tasklet_schedule(&local->wake_txqs_tasklet); } void ieee80211_wake_queue_by_reason(struct ieee80211_hw *hw, int queue, @@ -351,9 +447,6 @@ static void __ieee80211_stop_queue(struct ieee80211_hw *hw, int queue, if (__test_and_set_bit(reason, &local->queue_stop_reasons[queue])) return; - if (local->ops->wake_tx_queue) - return; - if (local->hw.queues < IEEE80211_NUM_ACS) n_acs = 1; @@ -366,8 +459,15 @@ static void __ieee80211_stop_queue(struct ieee80211_hw *hw, int queue, for (ac = 0; ac < n_acs; ac++) { if (sdata->vif.hw_queue[ac] == queue || - sdata->vif.cab_queue == queue) - netif_stop_subqueue(sdata->dev, ac); + sdata->vif.cab_queue == queue) { + if (!local->ops->wake_tx_queue) { + netif_stop_subqueue(sdata->dev, ac); + continue; + } + spin_lock(&local->fq.lock); + sdata->vif.txqs_stopped[ac] = true; + spin_unlock(&local->fq.lock); + } } } rcu_read_unlock();
Sometimes, it is required to stop the transmissions momentarily and resume it later; stopping the txqs becomes very critical in scenarios where the packet transmission has to be ceased completely. For example, during the hardware restart, during off channel operations, when initiating CSA(upon detecting a radar on the DFS channel), etc. The TX queue stop/start logic in mac80211 works well in stopping the TX when drivers make use of netdev queues, i.e, when Qdiscs in network layer take care of traffic scheduling. Since the devices implementing wake_tx_queue can run without Qdiscs, packets will be handed to mac80211 directly without queueing them in the netdev queues. Also, mac80211 does not invoke any of the netif_stop_*/netif_wake_* APIs if wake_tx_queue is implemented. Since the queues are not stopped in this case, transmissions can continue and this will impact negatively on the operation of the wireless device. For example, During hardware restart, we stop the netdev queues so that packets are not sent to the driver. Since ath10k implements wake_tx_queue, TX queues will not be stopped and packets might reach the hardware while it is restarting; this can make hardware unresponsive and the only possible option for recovery is to reboot the entire system. There is another problem to this, it is observed that the packets were sent on the DFS channel for a prolonged duration after radar detection impacting the channel closing time. We can still invoke netif stop/wake APIs when wake_tx_queue is implemented but this could lead to packet drops in network layer; adding stop/start logic for software TXQs in mac80211 instead makes more sense; the change proposed adds the same in mac80211. Signed-off-by: Manikanta Pubbisetty <mpubbise@codeaurora.org> --- V2: * addressed locking issues * addressed refcounting issues * per AC stop/start TXQs include/net/mac80211.h | 4 ++ net/mac80211/ieee80211_i.h | 3 ++ net/mac80211/main.c | 4 ++ net/mac80211/tx.c | 11 ++++- net/mac80211/util.c | 110 ++++++++++++++++++++++++++++++++++++++++++--- 5 files changed, 125 insertions(+), 7 deletions(-)