| Message ID | 1305894135-14036-6-git-send-email-rmanoharan@atheros.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, 2011-05-20 at 17:52 +0530, Rajkumar Manoharan wrote: > Stop tx queues before updating rate control to ensure > proper rate selection. Otherwise packets can be transmitted > in 40 Mhz whereas hw is configured in HT20. Looks like I completely missed this since you hid it in an ath9k patchset. DON'T DO THAT. Anyway, John, please revert. This is completely useless. Not only is abusing the CSA stop reason a show-stopper, the whole patch is also just not right, it seems like a workaround around a rate control algorithm that isn't able to do an atomic HT change by itself. Also, it won't even do what you want, there may be packets being processed concurrently while stopping the queue -- calling stop_queues() is no guarantee that no packet will be processed afterwards. 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 31 May 2011 14:35, Johannes Berg <johannes@sipsolutions.net> wrote: > Looks like I completely missed this since you hid it in an ath9k > patchset. DON'T DO THAT. > > Anyway, John, please revert. This is completely useless. Not only is > abusing the CSA stop reason a show-stopper, the whole patch is also just > not right, it seems like a workaround around a rate control algorithm > that isn't able to do an atomic HT change by itself. Also, it won't even > do what you want, there may be packets being processed concurrently > while stopping the queue -- calling stop_queues() is no guarantee that > no packet will be processed afterwards. I'm unsure of what's going on in mac80211 and ath9k here. FreeBSD handles it very simply - it goes via ath_reset() which drains each TX queue, freeing existing packets. Inefficient (ie, packet loss) but fine for now. How is ath9k handling the situation where the hardware currently has HT40 packets queued in the TX queues and you do a 40->20 change? The 40->20 change in the ath9k instance requires reinit'ing of the card. What happens to currently queued packets? Adrian -- 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 Tue, May 31, 2011 at 08:35:17AM +0200, Johannes Berg wrote: > On Fri, 2011-05-20 at 17:52 +0530, Rajkumar Manoharan wrote: > > Stop tx queues before updating rate control to ensure > > proper rate selection. Otherwise packets can be transmitted > > in 40 Mhz whereas hw is configured in HT20. > > Looks like I completely missed this since you hid it in an ath9k > patchset. DON'T DO THAT. > > Anyway, John, please revert. This is completely useless. Not only is > abusing the CSA stop reason a show-stopper, the whole patch is also just > not right, it seems like a workaround around a rate control algorithm > that isn't able to do an atomic HT change by itself. Also, it won't even > do what you want, there may be packets being processed concurrently > while stopping the queue -- calling stop_queues() is no guarantee that > no packet will be processed afterwards. Rajkumar, do you have an alternative fix to propose?
> On Fri, 2011-05-20 at 17:52 +0530, Rajkumar Manoharan wrote: > > Stop tx queues before updating rate control to ensure > > proper rate selection. Otherwise packets can be transmitted > > in 40 Mhz whereas hw is configured in HT20. > > Looks like I completely missed this since you hid it in an ath9k > patchset. DON'T DO THAT. Sorry for the delayed response. I was on vacation. > > Anyway, John, please revert. This is completely useless. Not only is > abusing the CSA stop reason a show-stopper, the whole patch is also just > not right, it seems like a workaround around a rate control algorithm > that isn't able to do an atomic HT change by itself. Also, it won't even > do what you want, there may be packets being processed concurrently > while stopping the queue -- calling stop_queues() is no guarantee that > no packet will be processed afterwards. During the channel type change, the pending tx frames in hw queues are dropped by hw config. But before updating rate control, the packets can be queued again with older HT rates. This contradicts with hw config mode and sometimes is causing baseband issues. This issue was observed only on flooding uplink traffic. To ensure that the frames are always xmitted with updated rates, the queues are stopped before hw config and waken up after rc updation. -- Rajkumar -- 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 Fri, Jun 03, 2011 at 05:00:01AM +0000, Manoharan, Rajkumar wrote: > > On Fri, 2011-05-20 at 17:52 +0530, Rajkumar Manoharan wrote: > > > Stop tx queues before updating rate control to ensure > > > proper rate selection. Otherwise packets can be transmitted > > > in 40 Mhz whereas hw is configured in HT20. > > > > Looks like I completely missed this since you hid it in an ath9k > > patchset. DON'T DO THAT. > > Sorry for the delayed response. I was on vacation. > > > > Anyway, John, please revert. This is completely useless. Not only is > > abusing the CSA stop reason a show-stopper, the whole patch is also just > > not right, it seems like a workaround around a rate control algorithm > > that isn't able to do an atomic HT change by itself. Also, it won't even > > do what you want, there may be packets being processed concurrently > > while stopping the queue -- calling stop_queues() is no guarantee that > > no packet will be processed afterwards. > > During the channel type change, the pending tx frames in hw queues are dropped by hw config. > But before updating rate control, the packets can be queued again with older HT rates. > This contradicts with hw config mode and sometimes is causing baseband issues. This issue > was observed only on flooding uplink traffic. To ensure that the frames are always xmitted with > updated rates, the queues are stopped before hw config and waken up after rc updation. Johannes, do you find this explanation satisfactory (perhaps with some new queue stop reason definition)? John
On Mon, 2011-06-06 at 14:44 -0400, John W. Linville wrote: > > During the channel type change, the pending tx frames in hw queues are dropped by hw config. > > But before updating rate control, the packets can be queued again with older HT rates. > > This contradicts with hw config mode and sometimes is causing baseband issues. This issue > > was observed only on flooding uplink traffic. To ensure that the frames are always xmitted with > > updated rates, the queues are stopped before hw config and waken up after rc updation. > > Johannes, do you find this explanation satisfactory (perhaps with > some new queue stop reason definition)? No, it doesn't address the fact that any packet that is pending will still be processed -- as I said before, the stop doesn't include a flush. 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 Mon, 2011-06-06 at 14:44 -0400, John W. Linville wrote: > > > > During the channel type change, the pending tx frames in hw queues are dropped by hw config. > > > But before updating rate control, the packets can be queued again with older HT rates. > > > This contradicts with hw config mode and sometimes is causing baseband issues. This issue > > > was observed only on flooding uplink traffic. To ensure that the frames are always xmitted with > > > updated rates, the queues are stopped before hw config and waken up after rc updation. >> > > Johannes, do you find this explanation satisfactory (perhaps with > > some new queue stop reason definition)? > > No, it doesn't address the fact that any packet that is pending will > still be processed -- as I said before, the stop doesn't include a > flush. Yes. I agree. Without flushing, still the packets can choose wrong rates. I missed that. But I assumed that it would be better to stop queues before rc update. Thus we can avoid revisiting the queued frames chosen with older (this case ht40) rate after rc changes. And fixing at mac80211 would help to other rate controls too. -- Rajkumar-- 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 Tue, Jun 07, 2011 at 08:39:57AM +0000, Manoharan, Rajkumar wrote: > > > On Mon, 2011-06-06 at 14:44 -0400, John W. Linville wrote: > > > > > > During the channel type change, the pending tx frames in hw queues are dropped by hw config. > > > > But before updating rate control, the packets can be queued again with older HT rates. > > > > This contradicts with hw config mode and sometimes is causing baseband issues. This issue > > > > was observed only on flooding uplink traffic. To ensure that the frames are always xmitted with > > > > updated rates, the queues are stopped before hw config and waken up after rc updation. > >> > > > Johannes, do you find this explanation satisfactory (perhaps with > > > some new queue stop reason definition)? > > > > No, it doesn't address the fact that any packet that is pending will > > still be processed -- as I said before, the stop doesn't include a > > flush. > > Yes. I agree. Without flushing, still the packets can choose wrong rates. I missed that. > But I assumed that it would be better to stop queues before rc update. Thus we can avoid revisiting > the queued frames chosen with older (this case ht40) rate after rc changes. And fixing at mac80211 would help > to other rate controls too. OK, it seems like this isn't getting resolved quickly. I'm going to revert it for now, and hope for a more widely acceptable solution soon. John
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c index 4f6b267..7465955 100644 --- a/net/mac80211/mlme.c +++ b/net/mac80211/mlme.c @@ -232,6 +232,9 @@ static u32 ieee80211_enable_ht(struct ieee80211_sub_if_data *sdata, WARN_ON(!ieee80211_set_channel_type(local, sdata, channel_type)); } + ieee80211_stop_queues_by_reason(&sdata->local->hw, + IEEE80211_QUEUE_STOP_REASON_CSA); + /* channel_type change automatically detected */ ieee80211_hw_config(local, 0); @@ -245,6 +248,9 @@ static u32 ieee80211_enable_ht(struct ieee80211_sub_if_data *sdata, rcu_read_unlock(); } + ieee80211_wake_queues_by_reason(&sdata->local->hw, + IEEE80211_QUEUE_STOP_REASON_CSA); + ht_opmode = le16_to_cpu(hti->operation_mode); /* if bss configuration changed store the new one */
Stop tx queues before updating rate control to ensure proper rate selection. Otherwise packets can be transmitted in 40 Mhz whereas hw is configured in HT20. Signed-off-by: Rajkumar Manoharan <rmanoharan@atheros.com> --- net/mac80211/mlme.c | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-)