mac80211_hwsim: Report rate info in tx status
PATCH 1/1 mac80211_hwsim: Report rate info in tx status
Assuming an ideal channel, always the first transmission is considered successful if an ACK is received. If no ACK is received, the rates/attempts are reported as set by the rate control.
--- drivers/net/wireless/mac80211_hwsim.c | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-)
(quoted diff) +0 -0
b/drivers/net/wireless/mac80211_hwsim.c
index b7ce6a6..64adb3c 100644
drivers/net/wireless/mac80211_hwsim.c +0 -0
@@ -698,6 +698,8 @@ static void mac80211_hwsim_tx(struct ieee80211_hw *hw,
struct sk_buff *skb)
bool ack;
struct ieee80211_tx_info *txi;
u32 _pid;
+ u8 tx_count[IEEE80211_TX_MAX_RATES];
+ int i;
struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *) skb->data; struct mac80211_hwsim_data *data = hw->priv;
@@ -734,9 +736,22 @@ static void mac80211_hwsim_tx(struct ieee80211_hw *hw, struct sk_buff *skb)
if (txi->control.sta)
hwsim_check_sta_magic(txi->control.sta);
+ if (!ack)
+ for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
+ tx_count[i] = txi->control.rates[i].count;
+
ieee80211_tx_info_clear_status(txi);
if (!(txi->flags & IEEE80211_TX_CTL_NO_ACK) && ack)
txi->flags |= IEEE80211_TX_STAT_ACK;
+
+ if (ack) {
+ txi->status.rates[0].count = 1;
+ txi->status.rates[1].idx = -1;
+ } else {
+ for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
+ txi->control.rates[i].count = tx_count[i];
+ }
+
ieee80211_tx_status_irqsafe(hw, skb);
}
Signature · 1 line
1.7.9.1
Johannes Berg
maintainerQuoted 8 lines · Timo Lindhorst
(quoted diff) +6
-0+ if (!ack) + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + tx_count[i] = txi->control.rates[i].count; + + + if (ack) {
...
Review
Quoted 4 lines
(quoted diff) +4
-0+ } else { + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + txi->control.rates[i].count = tx_count[i]; + }
I don't think I understand why you're copying the same data over itself?
johannes
Hey,
Quoted 8 lines
(quoted diff) +5
-0+ if (!ack) + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + tx_count[i] = txi->control.rates[i].count; + ieee80211_tx_info_clear_status(txi); if (!(txi->flags & IEEE80211_TX_CTL_NO_ACK) && ack) txi->flags |= IEEE80211_TX_STAT_ACK; +
Review
Quoted 10 lines
(quoted diff) +8
-0+ if (ack) { + txi->status.rates[0].count = 1; + txi->status.rates[1].idx = -1; + } else { + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + txi->control.rates[i].count = tx_count[i]; + } + ieee80211_tx_status_irqsafe(hw, skb); }
I know: backing up the count values, clearing the status, and restoring the values if necessary is kind of ugly. Would it be better to partly clear the status manually instead of using ieee80211_tx_info_clear_status() ?
Regards
Timo
Johannes Berg
maintainerQuoted 16 lines · Timo Lindhorst
Hey,
Quoted 14 lines
(quoted diff) +11
-0+ if (!ack) + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + tx_count[i] = txi->control.rates[i].count; + ieee80211_tx_info_clear_status(txi); if (!(txi->flags & IEEE80211_TX_CTL_NO_ACK) && ack) txi->flags |= IEEE80211_TX_STAT_ACK; + + if (ack) { + txi->status.rates[0].count = 1; + txi->status.rates[1].idx = -1; + } else { + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + txi->control.rates[i].count = tx_count[i];
Quoted 8 lines
Review
Quoted 4 lines
(quoted diff) +2
-0+ } + ieee80211_tx_status_irqsafe(hw, skb); }I know: backing up the count values, clearing the status, and restoring the values if necessary is kind of ugly. Would it be better to partly clear the status manually instead of using
ieee80211_tx_info_clear_status()?
Yeah just noticed the ieee80211_tx_info_clear_status() in there too...
OTOH, what are you using this for? It seems almost like we should always just set
txi->status.rates[0].count = 1;
since we never attempted multiple transmits? I'm not really sure though, it's a corner case ... I could also imagine this being populated by userspace (wmediumd) but I guess that isn't there now ...
johannes
Quoted 29 lines
Quoted 20 lines
Quoted 20 lines
(quoted diff) +13
-0+ if (!ack) + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + tx_count[i] = txi->control.rates[i].count; + ieee80211_tx_info_clear_status(txi); if (!(txi->flags & IEEE80211_TX_CTL_NO_ACK) && ack) txi->flags |= IEEE80211_TX_STAT_ACK; + + if (ack) { + txi->status.rates[0].count = 1; + txi->status.rates[1].idx = -1; + } else { + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + txi->control.rates[i].count = tx_count[i]; + } +Quoted 8 lines
Quoted 3 lines
ieee80211_tx_status_irqsafe(hw, skb);}
I know: backing up the count values, clearing the status, and restoring the values if necessary is kind of ugly. Would it be better to partly clear the status manually instead of using
ieee80211_tx_info_clear_status()?
Quoted 3 lines
Yeah just noticed the
ieee80211_tx_info_clear_status()in there too...OTOH, what are you using this for?
While working on some modifications to the rate control code, I thought it would be handy to use mac80211_hwsim for debugging and testing. Thereby I noticed that the tx status does not report any tx attempts, thus the rate control could not work at all.
Quoted 3 lines
It seems almost like we should always
just settxi->status.rates[0].count = 1;
At least,
txi->status.rates[1].idx = -1;
has to be set too, to indicate that only the first rate was used.
Quoted 2 lines
since we never attempted multiple transmits? I'm not really sure though, it's a corner case ...
We would only attempt multiple transmits if the receiver is not responding to unicast frames -- maybe because it has failed or switched the channel. Probably not a common use case, but that was what I was testing...
Quoted 2 lines
I could also imagine this being populated by userspace (wmediumd) but I guess that isn't there now ...
Right, but if you are not using wmediumd but the bare mac80211_hwsim ideal channel, there would be no rate information and thus no rate adaption through the rate control algorithm.
Regards
Timo
Quoted 53 lines · Timo Lindhorst
Quoted 29 lines
Quoted 20 lines
Quoted 20 lines
(quoted diff) +13
-0+ if (!ack) + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + tx_count[i] = txi->control.rates[i].count; + ieee80211_tx_info_clear_status(txi); if (!(txi->flags & IEEE80211_TX_CTL_NO_ACK) && ack) txi->flags |= IEEE80211_TX_STAT_ACK; + + if (ack) { + txi->status.rates[0].count = 1; + txi->status.rates[1].idx = -1; + } else { + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + txi->control.rates[i].count = tx_count[i]; + } +Quoted 8 lines
Quoted 3 lines
ieee80211_tx_status_irqsafe(hw, skb);}
I know: backing up the count values, clearing the status, and restoring the values if necessary is kind of ugly. Would it be better to partly clear the status manually instead of using
ieee80211_tx_info_clear_status()?Quoted 3 lines
Yeah just noticed the
ieee80211_tx_info_clear_status()in there too...OTOH, what are you using this for?
While working on some modifications to the rate control code, I thought it would be handy to use
mac80211_hwsimfor debugging and testing. Thereby I noticed that the tx status does not report any tx attempts, thus the rate control could not work at all.Quoted 3 lines
It seems almost like we should always
just settxi->status.rates[0].count = 1;At least,
txi->status.rates[1].idx = -1;has to be set too, to indicate that only the first rate was used.
Quoted 2 lines
since we never attempted multiple transmits? I'm not really sure though, it's a corner case ...
We would only attempt multiple transmits if the receiver is not responding to unicast frames -- maybe because it has failed or switched the channel. Probably not a common use case, but that was what I was testing...
Quoted 8 lines
Quoted 2 lines
I could also imagine this being populated by userspace (wmediumd) but I guess that isn't there now ...
Right, but if you are not using wmediumd but the bare
mac80211_hwsimideal channel, there would be no rate information and thus no rate adaption through the rate control algorithm.Regards
Timo
Johannes, does this satisfy your concerns?
John
Signature · 2 lines
John W. Linville Someday the world will need a hero, and you [email protected] might be all we have. Be ready.
Johannes Berg
maintainerQuoted 56 lines · John W. Linville
Quoted 55 lines · Timo Lindhorst
Quoted 29 lines
Quoted 20 lines
Quoted 20 lines
(quoted diff) +13
-0+ if (!ack) + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + tx_count[i] = txi->control.rates[i].count; + ieee80211_tx_info_clear_status(txi); if (!(txi->flags & IEEE80211_TX_CTL_NO_ACK) && ack) txi->flags |= IEEE80211_TX_STAT_ACK; + + if (ack) { + txi->status.rates[0].count = 1; + txi->status.rates[1].idx = -1; + } else { + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + txi->control.rates[i].count = tx_count[i]; + } +Quoted 8 lines
Quoted 3 lines
ieee80211_tx_status_irqsafe(hw, skb);}
I know: backing up the count values, clearing the status, and restoring the values if necessary is kind of ugly. Would it be better to partly clear the status manually instead of using
ieee80211_tx_info_clear_status()?Quoted 3 lines
Yeah just noticed the
ieee80211_tx_info_clear_status()in there too...OTOH, what are you using this for?
While working on some modifications to the rate control code, I thought it would be handy to use
mac80211_hwsimfor debugging and testing. Thereby I noticed that the tx status does not report any tx attempts, thus the rate control could not work at all.Quoted 3 lines
It seems almost like we should always
just settxi->status.rates[0].count = 1;At least,
txi->status.rates[1].idx = -1;has to be set too, to indicate that only the first rate was used.
Quoted 2 lines
since we never attempted multiple transmits? I'm not really sure though, it's a corner case ...
We would only attempt multiple transmits if the receiver is not responding to unicast frames -- maybe because it has failed or switched the channel. Probably not a common use case, but that was what I was testing...
Quoted 2 lines
I could also imagine this being populated by userspace (wmediumd) but I guess that isn't there now ...
Quoted 8 lines
Quoted 6 lines
Right, but if you are not using wmediumd but the bare
mac80211_hwsimideal channel, there would be no rate information and thus no rate adaption through the rate control algorithm.Regards
TimoJohannes, does this satisfy your concerns?
Yeah, it seems that this is all needs to be extended to actually be useful, but we can leave that for later.
I'm a little confused about this discussion & Javier's patch though -- are they related?
johannes
Quoted 5 lines · Johannes Berg
Quoted 1 line · John W. Linville
On Wed, Mar 28, 2012 at 03:47:21PM +0200, Timo Lindhorst wrote:
(...)
I'm a little confused about this discussion & Javier's patch though -- are they related?
Yes they are. My patch¹ was fixing the same problem with rate adaptation, but I only handled the case of successful transmission. Timo's patch fixes rate reporting for both successful and failed transmissions, which, in the in-kernel perfect/broken link simulation is all that we need.
I would recommend you ignore my patch and take Timo's.
As for the in-userspace case, the driver is giving the userspace daemon (e.g. wmediumd) control over which rates to report (see hwsim_tx_info_frame_received_nl() for details).
Cheers,
Javier
[1] http://www.spinics.net/lists/linux-wireless/msg89454.html
Signature · 3 lines
Javier Cardona cozybit Inc. http://www.cozybit.com
Quoted 5 lines · Timo Lindhorst
Assuming an ideal channel, always the first transmission is considered successful if an ACK is received. If no ACK is received, the rates/attempts are reported as set by the rate control.
Signed-off-by Timo Lindhorst <[email protected]>
I just tested this patch and rate control in hwsim seems to be operational once again. Attached you'll find a rate plot that shows the data rate evolution over time.
I believe the periodic dips in data rate are due to minstrel sampling different rates, although I'm not certain about that...
Cheers,
Javier
Quoted 50 lines
---
drivers/net/wireless/mac80211_hwsim.c | 15 +++++++++++++++1 files changed, 15 insertions(+), 0 deletions(-)drivers/net/wireless/mac80211_hwsim.c +15
-1@@ -698,6 +698,8 @@ static void mac80211_hwsim_tx(struct ieee80211_hw *hw, struct sk_buff *skb) bool ack; struct ieee80211_tx_info *txi; u32 _pid; + u8 tx_count[IEEE80211_TX_MAX_RATES]; + int i; struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *) skb->data; struct mac80211_hwsim_data *data = hw->priv;@@ -734,9 +736,22 @@ static void mac80211_hwsim_tx(struct ieee80211_hw *hw, struct sk_buff *skb) if (txi->control.sta) hwsim_check_sta_magic(txi->control.sta); + if (!ack) + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + tx_count[i] = txi->control.rates[i].count; + ieee80211_tx_info_clear_status(txi); if (!(txi->flags & IEEE80211_TX_CTL_NO_ACK) && ack) txi->flags |= IEEE80211_TX_STAT_ACK; + + if (ack) { + txi->status.rates[0].count = 1; + txi->status.rates[1].idx = -1; + } else { + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) + txi->control.rates[i].count = tx_count[i]; + } + ieee80211_tx_status_irqsafe(hw, skb); } --1.7.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
Javier Cardona cozybit Inc. http://www.cozybit.com