| Message ID | 1529997415-20551-1-git-send-email-mpubbise@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
Manikanta Pubbisetty <mpubbise@codeaurora.org> writes: > 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. I agree with the approach; having packets queued in mac80211 while the queues are stopped also means that CoDel can react to them when they are resumed again. > + list_for_each_entry_rcu(sta, &local->sta_list, list) { > + if (!atomic_read(&sta->txqs_paused)) > + continue; > + > + atomic_set(&sta->txqs_paused, 0); I'm not terribly well-versed in the kernel atomics, but doesn't the split of read and set kinda defeat the point of using them? Also, as this is under RCU, why do you need an atomic in the first place? -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-rc2 next-20180626]
[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/20180626-153410
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'
Error: Cannot open file arch/x86/kernel/cpu/mtrr/main.c
Error: Cannot open file arch/x86/kernel/cpu/mtrr/main.c
WARNING: kernel-doc 'scripts/kernel-doc -rst -enable-lineno -export arch/x86/kernel/cpu/mtrr/main.c' failed with return code 2
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:2328: warning: Function parameter or member 'radiotap_timestamp.units_pos' not described in 'ieee80211_hw'
include/net/mac80211.h:2328: warning: Function parameter or member 'radiotap_timestamp.accuracy' not described in 'ieee80211_hw'
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:589: warning: Function parameter or member 'rx_stats_avg' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'rx_stats_avg.signal' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'status_stats.msdu_retries' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'status_stats.msdu_failed' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'status_stats.last_ack_signal' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'status_stats.ack_signal_filled' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'status_stats.avg_ack_signal' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info'
net/mac80211/sta_info.h:589: warning: Function parameter or member 'tx_stats.msdu' not described in 'sta_info'
>> net/mac80211/sta_info.h:589: warning: Function parameter or member 'txqs_paused' 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'
vim +589 net/mac80211/sta_info.h
17741cdc2 Johannes Berg 2008-09-11 574
0af83d3df Johannes Berg 2012-12-27 575 enum ieee80211_sta_rx_bandwidth cur_max_bandwidth;
0af83d3df Johannes Berg 2012-12-27 576
687da1322 Emmanuel Grumbach 2013-10-01 577 enum ieee80211_smps_mode known_smps_mode;
2475b1cc0 Max Stepanov 2013-03-24 578 const struct ieee80211_cipher_scheme *cipher_scheme;
687da1322 Emmanuel Grumbach 2013-10-01 579
484a54c2e Toke Høiland-Jørgensen 2017-04-06 580 struct codel_params cparams;
484a54c2e Toke Høiland-Jørgensen 2017-04-06 581
b6da911b3 Liad Kaufman 2014-11-19 582 u8 reserved_tid;
b6da911b3 Liad Kaufman 2014-11-19 583
0fabfaafe Arik Nemtsov 2015-06-10 584 struct cfg80211_chan_def tdls_chandef;
061b82d2b Manikanta Pubbisetty 2018-06-26 585 atomic_t txqs_paused;
0fabfaafe Arik Nemtsov 2015-06-10 586
17741cdc2 Johannes Berg 2008-09-11 587 /* keep last! */
17741cdc2 Johannes Berg 2008-09-11 588 struct ieee80211_sta sta;
f0706e828 Jiri Benc 2007-05-05 @589 };
f0706e828 Jiri Benc 2007-05-05 590
:::::: The code at line 589 was first introduced by commit
:::::: f0706e828e96d0fa4e80c0d25aa98523f6d589a0 [MAC80211]: Add mac80211 wireless stack.
:::::: TO: Jiri Benc <jbenc@suse.cz>
:::::: 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
On 6/26/2018 4:55 PM, Toke Høiland-Jørgensen wrote: > Manikanta Pubbisetty <mpubbise@codeaurora.org> writes: > >> 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. > I agree with the approach; having packets queued in mac80211 while the > queues are stopped also means that CoDel can react to them when they are > resumed again. Agreed!! > >> + list_for_each_entry_rcu(sta, &local->sta_list, list) { >> + if (!atomic_read(&sta->txqs_paused)) >> + continue; >> + >> + atomic_set(&sta->txqs_paused, 0); > I'm not terribly well-versed in the kernel atomics, but doesn't the > split of read and set kinda defeat the point of using them? Also, as > this is under RCU, why do you need an atomic in the first place? Valid point, txqs_paused is set in ieee80211_tx_dequeue; I was thinking a case where tx_dequeue is called without rcu_read_lock but seems that is not the case. All drivers using wake_tx_queue seems are using rcu_read_lock before tx_dequeue; can there be a case where tx_dequeue is called without rcu_read_lock? Manikanta
Manikanta Pubbisetty <mpubbise@codeaurora.org> writes: > On 6/26/2018 4:55 PM, Toke Høiland-Jørgensen wrote: >> Manikanta Pubbisetty <mpubbise@codeaurora.org> writes: >> >>> 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. >> I agree with the approach; having packets queued in mac80211 while the >> queues are stopped also means that CoDel can react to them when they are >> resumed again. > > Agreed!! > >> >>> + list_for_each_entry_rcu(sta, &local->sta_list, list) { >>> + if (!atomic_read(&sta->txqs_paused)) >>> + continue; >>> + >>> + atomic_set(&sta->txqs_paused, 0); >> I'm not terribly well-versed in the kernel atomics, but doesn't the >> split of read and set kinda defeat the point of using them? Also, as >> this is under RCU, why do you need an atomic in the first place? > Valid point, txqs_paused is set in ieee80211_tx_dequeue; I was thinking > a case where tx_dequeue is called without rcu_read_lock but seems that > is not the case. All drivers using wake_tx_queue seems are using > rcu_read_lock before tx_dequeue; can there be a case where tx_dequeue is > called without rcu_read_lock? I'm pretty sure we would have other problems if there were... :) -Toke
On Tue, 2018-06-26 at 13:25 +0200, Toke Høiland-Jørgensen wrote: > Manikanta Pubbisetty <mpubbise@codeaurora.org> writes: > > > 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. > > I agree with the approach; having packets queued in mac80211 while the > queues are stopped also means that CoDel can react to them when they are > resumed again. > > > > + list_for_each_entry_rcu(sta, &local->sta_list, list) { > > + if (!atomic_read(&sta->txqs_paused)) > > + continue; > > + > > + atomic_set(&sta->txqs_paused, 0); > > I'm not terribly well-versed in the kernel atomics, but doesn't the > split of read and set kinda defeat the point of using them? Also, as > this is under RCU, why do you need an atomic in the first place? RCU doesn't do any locking, but the usage of atomic_t is indeed rather pointless since all you do is "atomic_set()" and "atomic_read()" - which are actually not special at all, they're just writing/reading the variable respectively! The only thing that's really special is atomic_inc(), atomic_dec() and friends. If you explain what you were trying to achieve here then we can probably help you. I'll also review the rest of the patch now, but I'll already note you should fix the kernel-doc complaint from the kbuild test robot. johannes
On Tue, 2018-06-26 at 18:01 +0530, Manikanta Pubbisetty wrote: > > Valid point, txqs_paused is set in ieee80211_tx_dequeue; I was thinking > a case where tx_dequeue is called without rcu_read_lock but seems that > is not the case. All drivers using wake_tx_queue seems are using > rcu_read_lock before tx_dequeue; can there be a case where tx_dequeue is > called without rcu_read_lock? FWIW, the rcu_read_lock() isn't relevant at all - multiple threads can well (and will for sure!) enter it. johannes
On Tue, 2018-06-26 at 12:46 +0530, Manikanta Pubbisetty wrote: > > - 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 (local->txqs_stopped) { How is it even possible to have txqs_stopped set, but not TXQ_PAUSED? It seemed to me - from a first glance at the rest of the code - that the txqs_stopped is just tracking when you can re-enable, but you propagate it to the TXQs? > + set_bit(IEEE80211_TXQ_PAUSED, &txqi->flags); Oh. Or maybe I just misread that and you're only "lazily" propagating it here? > + if (txq->sta) { > + sta = container_of(txq->sta, struct sta_info, sta); > + atomic_set(&sta->txqs_paused, 1); > + } It seems to me you could remove this - possibly at the expense of doing a little more work in ieee80211_wake_txqs()? Have you measured it and found it to be a problem? The atomic stuff here doesn't work the way you want it to though. In fact, even the lazy propagation doesn't work the way you want it to, I think! Thinking about it: CPU 1 CPU 2 * you're disabling TX -> local->txqs_stopped = 1 * check local->txqs_stopped * re-enable TX -> local->txqs_stopped = 0 * call ieee80211_wake_txqs() check sta->txqs_paused * set txq->flags |= PAUSED * set sta->txqs_paused I don't see how you can prevent this right now? In ieee80211_tx_dequeue() you have the fq lock, but you're not taking it on the other side (in __ieee80211_stop_queue/__ieee80211_wake_queue). If you always iterate the stas/TXQs and set the PAUSED bit non-lazily at least you have a single source of truth. You may still need the txqs_stopped bitmap, but only in stop_queue/wake_queue which has its own locking. > + for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) { > + txqi = to_txq_info(sta->sta.txq[i]); > + > + if (!test_and_clear_bit(IEEE80211_TXQ_PAUSED, > + &txqi->flags)) > + continue; > + > + drv_wake_tx_queue(local, txqi); Maybe that should be conditional on there being packets? Actually, no ... ok, the lazy setting helps you with this because it means that the driver wanted a packet but didn't get one, so now you should wake it. If it never wanted a packet, then you don't care about the state. Ok - so that means we need to keep the lazy setting, but fix the locking :-) > + /* Software interfaces like AP_VLAN, monitor do not have > + * vif specific queues. > + */ > + if (!sdata->dev || !vif->txq) > + continue; Is there any way you can have vif->txq && !sdata->dev? It seems to me that no, and then you only need the vif->txq check? > @@ -298,10 +354,15 @@ static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue, > if (local->q_stop_reasons[queue][reason] == 0) > __clear_bit(reason, &local->queue_stop_reasons[queue]); > > + if (local->ops->wake_tx_queue) > + __clear_bit(reason, &local->txqs_stopped); > + > if (local->queue_stop_reasons[queue] != 0) > /* someone still has this queue stopped */ > return; > > + ieee80211_wake_txqs(local); I'm not sure this is the right place to hook in? This might interact really strangely with drivers - which also get here. Perhaps we should make this conditional on reason != DRIVER? Also, your bitmap setting should be different - it should be per queue? But then since "queue" here should basically be an AC for iTXQ drivers, I think you need to handle that properly. Right now you get bugs if you do stop_queue(0, reason=aggregation) stop_queue(1, reason=aggregation) wake_queue(0, reason=aggregation) -> TXQs will wake up because local->txqs_stopped is only stopping the reason, not the queue number. So to summarize: * overall the approach seems fine, and the lazy disabling is probably for the better, but * need to address locking issues with that, and * need to address the refcounting issues. johannes
>> + if (txq->sta) { >> + sta = container_of(txq->sta, struct sta_info, sta); >> + atomic_set(&sta->txqs_paused, 1); >> + } > It seems to me you could remove this - possibly at the expense of doing > a little more work in ieee80211_wake_txqs()? Atomic set on txqs_paused is done to avoid checking each txq for pause condition in ieee80211_wake_txqs; In other words, we will walk through the STA iTXQs only if sta->txqs_paused is set. This minor optimization might be helpful when the sta_list is huge, no? > Have you measured it and found it to be a problem? > > The atomic stuff here doesn't work the way you want it to though. In > fact, even the lazy propagation doesn't work the way you want it to, I > think! > > Thinking about it: > > CPU 1 CPU 2 > * you're disabling TX > -> local->txqs_stopped = 1 > * check local->txqs_stopped > * re-enable TX > -> local->txqs_stopped = 0 > * call ieee80211_wake_txqs() > check sta->txqs_paused > * set txq->flags |= PAUSED > * set sta->txqs_paused > > I don't see how you can prevent this right now? In > ieee80211_tx_dequeue() you have the fq lock, but you're not taking it on > the other side (in __ieee80211_stop_queue/__ieee80211_wake_queue). > You are right, we should take care of locking. >> + for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) { >> + txqi = to_txq_info(sta->sta.txq[i]); >> + >> + if (!test_and_clear_bit(IEEE80211_TXQ_PAUSED, >> + &txqi->flags)) >> + continue; >> + >> + drv_wake_tx_queue(local, txqi); > Maybe that should be conditional on there being packets? Actually, no > ... ok, the lazy setting helps you with this because it means that the > driver wanted a packet but didn't get one, so now you should wake it. If > it never wanted a packet, then you don't care about the state. > > Ok - so that means we need to keep the lazy setting, but fix the locking > :-) Correct!! >> + /* Software interfaces like AP_VLAN, monitor do not have >> + * vif specific queues. >> + */ >> + if (!sdata->dev || !vif->txq) >> + continue; > Is there any way you can have vif->txq && !sdata->dev? It seems to me > that no, and then you only need the vif->txq check? Makes sense; we need not check for sdata->dev. >> @@ -298,10 +354,15 @@ static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue, >> if (local->q_stop_reasons[queue][reason] == 0) >> __clear_bit(reason, &local->queue_stop_reasons[queue]); >> >> + if (local->ops->wake_tx_queue) >> + __clear_bit(reason, &local->txqs_stopped); >> + >> if (local->queue_stop_reasons[queue] != 0) >> /* someone still has this queue stopped */ >> return; >> >> + ieee80211_wake_txqs(local); > I'm not sure this is the right place to hook in? > > This might interact really strangely with drivers - which also get here. > Perhaps we should make this conditional on reason != DRIVER? Good point! IMO the condition (reason != DRIVER) does not fit well; if the reason for stop queues is DRIVER then we don't get a chance to wake them again. May be deferring the wake_txqs by scheduling a tasklet should help? > > Also, your bitmap setting should be different - it should be per queue? > But then since "queue" here should basically be an AC for iTXQ drivers, > I think you need to handle that properly. > > Right now you get bugs if you do > > stop_queue(0, reason=aggregation) > stop_queue(1, reason=aggregation) > wake_queue(0, reason=aggregation) > > -> TXQs will wake up because local->txqs_stopped is only stopping the > reason, not the queue number. > Makes sense!! > So to summarize: > * overall the approach seems fine, and the lazy disabling is probably > for the better, but > * need to address locking issues with that, and > * need to address the refcounting issues. Sure Johannes!! Thanks, Manikanta
> I'll also review the rest of the patch now, but I'll already note you > should fix the kernel-doc complaint from the kbuild test robot. Sure, I will take care of this in the next revision of the patch. Thanks, Manikanta
On Mon, 2018-07-02 at 14:48 +0530, Manikanta Pubbisetty wrote: > > > > > @@ -298,10 +354,15 @@ static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue, > > > if (local->q_stop_reasons[queue][reason] == 0) > > > __clear_bit(reason, &local->queue_stop_reasons[queue]); > > > > > > + if (local->ops->wake_tx_queue) > > > + __clear_bit(reason, &local->txqs_stopped); > > > + > > > if (local->queue_stop_reasons[queue] != 0) > > > /* someone still has this queue stopped */ > > > return; > > > > > > + ieee80211_wake_txqs(local); > > > > I'm not sure this is the right place to hook in? > > > > This might interact really strangely with drivers - which also get here. > > Perhaps we should make this conditional on reason != DRIVER? > > Good point! IMO the condition (reason != DRIVER) does not fit well; if > the reason for stop queues is DRIVER then we don't get a chance to wake > them again. May be deferring the wake_txqs by scheduling a tasklet > should help? Not sure what you mean? But I see what I said was also confusing. What I meant is that we should just not allow the driver to stop the TXQs this way - if it's a TXQ driver then it should just stop scheduling if it needs to, it shouldn't continue scheduling and rely on us blocking it. We, for mac80211's purposes, need to stop, cannot influence scheduling and want to unify it with the 'normal' queue stop - so my thought would be that we should only set the TXQ to stopped if the reason isn't DRIVER. johannes
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h index 172aeae..500da15 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, }; /** @@ -1139,6 +1140,11 @@ struct ieee80211_local { /* also used to protect ampdu_ac_queue and amdpu_ac_stop_refcnt */ spinlock_t queue_stop_reason_lock; + /* pause/resume logic for intermediate software queues, + * applicable when wake_tx_queue is defined. + */ + unsigned long txqs_stopped; + int open_count; int monitors, cooked_mntrs; /* number of interfaces with corresponding FIF_ flags */ diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h index 9a04327..82e5bbb 100644 --- a/net/mac80211/sta_info.h +++ b/net/mac80211/sta_info.h @@ -582,6 +582,7 @@ struct sta_info { u8 reserved_tid; struct cfg80211_chan_def tdls_chandef; + atomic_t txqs_paused; /* keep last! */ struct ieee80211_sta sta; diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c index 5b93bde..9b30ee6 100644 --- a/net/mac80211/tx.c +++ b/net/mac80211/tx.c @@ -3467,12 +3467,23 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw, struct ieee80211_tx_data tx; ieee80211_tx_result r; struct ieee80211_vif *vif; + struct sta_info *sta; 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 (local->txqs_stopped) { + set_bit(IEEE80211_TXQ_PAUSED, &txqi->flags); + if (txq->sta) { + sta = container_of(txq->sta, struct sta_info, sta); + atomic_set(&sta->txqs_paused, 1); + } + goto out; + } + /* Make sure fragments stay together. */ skb = __skb_dequeue(&txqi->frags); if (skb) diff --git a/net/mac80211/util.c b/net/mac80211/util.c index c77c843..b9db417 100644 --- a/net/mac80211/util.c +++ b/net/mac80211/util.c @@ -273,6 +273,62 @@ void ieee80211_propagate_queue_wake(struct ieee80211_local *local, int queue) } } +static void ieee80211_wake_txqs(struct ieee80211_local *local) +{ + struct ieee80211_sub_if_data *sdata; + struct sta_info *sta; + struct txq_info *txqi; + int i; + + if (!local->ops->wake_tx_queue) + return; + + rcu_read_lock(); + + list_for_each_entry_rcu(sta, &local->sta_list, list) { + if (!atomic_read(&sta->txqs_paused)) + continue; + + atomic_set(&sta->txqs_paused, 0); + + for (i = 0; i < ARRAY_SIZE(sta->sta.txq); i++) { + txqi = to_txq_info(sta->sta.txq[i]); + + if (!test_and_clear_bit(IEEE80211_TXQ_PAUSED, + &txqi->flags)) + continue; + + drv_wake_tx_queue(local, txqi); + } + } + + list_for_each_entry_rcu(sdata, &local->interfaces, list) { + struct ieee80211_vif *vif = &sdata->vif; + struct ps_data *ps = NULL; + + /* Software interfaces like AP_VLAN, monitor do not have + * vif specific queues. + */ + if (!sdata->dev || !vif->txq) + continue; + + txqi = to_txq_info(vif->txq); + + if (!test_and_clear_bit(IEEE80211_TXQ_PAUSED, &txqi->flags)) + continue; + + if (sdata->vif.type == NL80211_IFTYPE_AP) + ps = &sdata->bss->ps; + + if (ps && atomic_read(&ps->num_sta_ps)) + continue; + + drv_wake_tx_queue(local, txqi); + } + + rcu_read_unlock(); +} + static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue, enum queue_stop_reason reason, bool refcounted) @@ -298,10 +354,15 @@ static void __ieee80211_wake_queue(struct ieee80211_hw *hw, int queue, if (local->q_stop_reasons[queue][reason] == 0) __clear_bit(reason, &local->queue_stop_reasons[queue]); + if (local->ops->wake_tx_queue) + __clear_bit(reason, &local->txqs_stopped); + if (local->queue_stop_reasons[queue] != 0) /* someone still has this queue stopped */ return; + ieee80211_wake_txqs(local); + if (skb_queue_empty(&local->pending[queue])) { rcu_read_lock(); ieee80211_propagate_queue_wake(local, queue); @@ -351,8 +412,10 @@ 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) + if (local->ops->wake_tx_queue) { + __set_bit(reason, &local->txqs_stopped); return; + } if (local->hw.queues < IEEE80211_NUM_ACS) n_acs = 1;
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 times. 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> --- net/mac80211/ieee80211_i.h | 6 +++++ net/mac80211/sta_info.h | 1 + net/mac80211/tx.c | 13 +++++++++- net/mac80211/util.c | 65 +++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 83 insertions(+), 2 deletions(-)