* [PATCH] mac80211_hwsim: Report rate info in tx status
@ 2012-03-28 9:17 Timo Lindhorst
2012-03-28 9:28 ` Johannes Berg
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Timo Lindhorst @ 2012-03-28 9:17 UTC (permalink / raw)
To: John W. Linville; +Cc: linux-wireless, Johannes Berg, Jouni Malinen
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 <tlnd@online.de>
---
drivers/net/wireless/mac80211_hwsim.c | 15 +++++++++++++++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/drivers/net/wireless/mac80211_hwsim.c
b/drivers/net/wireless/mac80211_hwsim.c
index b7ce6a6..64adb3c 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH] mac80211_hwsim: Report rate info in tx status
2012-03-28 9:17 [PATCH] mac80211_hwsim: Report rate info in tx status Timo Lindhorst
@ 2012-03-28 9:28 ` Johannes Berg
2012-03-28 9:28 ` Timo Lindhorst
2012-05-07 23:17 ` Javier Cardona
2 siblings, 0 replies; 9+ messages in thread
From: Johannes Berg @ 2012-03-28 9:28 UTC (permalink / raw)
To: Timo Lindhorst; +Cc: John W. Linville, linux-wireless, Jouni Malinen
On Wed, 2012-03-28 at 11:17 +0200, Timo Lindhorst wrote:
>
> + if (!ack)
> + for (i = 0; i < IEEE80211_TX_MAX_RATES; i++)
> + tx_count[i] = txi->control.rates[i].count;
> +
> +
> + if (ack) {
...
> + } 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mac80211_hwsim: Report rate info in tx status
2012-03-28 9:17 [PATCH] mac80211_hwsim: Report rate info in tx status Timo Lindhorst
2012-03-28 9:28 ` Johannes Berg
@ 2012-03-28 9:28 ` Timo Lindhorst
2012-03-28 9:32 ` Johannes Berg
2012-05-07 23:17 ` Javier Cardona
2 siblings, 1 reply; 9+ messages in thread
From: Timo Lindhorst @ 2012-03-28 9:28 UTC (permalink / raw)
To: Timo Lindhorst
Cc: John W. Linville, linux-wireless, Johannes Berg, Jouni Malinen
Hey,
> + 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);
> }
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mac80211_hwsim: Report rate info in tx status
2012-03-28 9:28 ` Timo Lindhorst
@ 2012-03-28 9:32 ` Johannes Berg
2012-03-28 13:47 ` Timo Lindhorst
0 siblings, 1 reply; 9+ messages in thread
From: Johannes Berg @ 2012-03-28 9:32 UTC (permalink / raw)
To: Timo Lindhorst; +Cc: John W. Linville, linux-wireless, Jouni Malinen
On Wed, 2012-03-28 at 11:28 +0200, Timo Lindhorst wrote:
> Hey,
>
> > + 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);
> > }
>
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mac80211_hwsim: Report rate info in tx status
2012-03-28 9:32 ` Johannes Berg
@ 2012-03-28 13:47 ` Timo Lindhorst
2012-04-30 18:34 ` John W. Linville
0 siblings, 1 reply; 9+ messages in thread
From: Timo Lindhorst @ 2012-03-28 13:47 UTC (permalink / raw)
To: Johannes Berg; +Cc: John W. Linville, linux-wireless, Jouni Malinen
> > > + 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);
> > >
> > > }
> >
> > 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?
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.
> It seems almost like we should always
> just set
> txi->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.
> 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...
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mac80211_hwsim: Report rate info in tx status
2012-03-28 13:47 ` Timo Lindhorst
@ 2012-04-30 18:34 ` John W. Linville
2012-05-03 19:33 ` Johannes Berg
0 siblings, 1 reply; 9+ messages in thread
From: John W. Linville @ 2012-04-30 18:34 UTC (permalink / raw)
To: Timo Lindhorst; +Cc: Johannes Berg, linux-wireless, Jouni Malinen
On Wed, Mar 28, 2012 at 03:47:21PM +0200, Timo Lindhorst wrote:
> > > > + 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);
> > > >
> > > > }
> > >
> > > 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?
> 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.
>
> > It seems almost like we should always
> > just set
> > txi->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.
>
>
> > 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...
>
>
> > 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
Johannes, does this satisfy your concerns?
John
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mac80211_hwsim: Report rate info in tx status
2012-04-30 18:34 ` John W. Linville
@ 2012-05-03 19:33 ` Johannes Berg
2012-05-03 21:34 ` Javier Cardona
0 siblings, 1 reply; 9+ messages in thread
From: Johannes Berg @ 2012-05-03 19:33 UTC (permalink / raw)
To: John W. Linville
Cc: Timo Lindhorst, linux-wireless, Jouni Malinen, Javier Cardona
On Mon, 2012-04-30 at 14:34 -0400, John W. Linville wrote:
> On Wed, Mar 28, 2012 at 03:47:21PM +0200, Timo Lindhorst wrote:
> > > > > + 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);
> > > > >
> > > > > }
> > > >
> > > > 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?
> > 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.
> >
> > > It seems almost like we should always
> > > just set
> > > txi->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.
> >
> >
> > > 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...
> >
> >
> > > 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
>
> Johannes, 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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] mac80211_hwsim: Report rate info in tx status
2012-05-03 19:33 ` Johannes Berg
@ 2012-05-03 21:34 ` Javier Cardona
0 siblings, 0 replies; 9+ messages in thread
From: Javier Cardona @ 2012-05-03 21:34 UTC (permalink / raw)
To: Johannes Berg
Cc: John W. Linville, Timo Lindhorst, linux-wireless, Jouni Malinen
On Thu, May 3, 2012 at 12:33 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2012-04-30 at 14:34 -0400, John W. Linville wrote:
>> 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
--
Javier Cardona
cozybit Inc.
http://www.cozybit.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mac80211_hwsim: Report rate info in tx status
2012-03-28 9:17 [PATCH] mac80211_hwsim: Report rate info in tx status Timo Lindhorst
2012-03-28 9:28 ` Johannes Berg
2012-03-28 9:28 ` Timo Lindhorst
@ 2012-05-07 23:17 ` Javier Cardona
2 siblings, 0 replies; 9+ messages in thread
From: Javier Cardona @ 2012-05-07 23:17 UTC (permalink / raw)
To: Timo Lindhorst
Cc: John W. Linville, linux-wireless, Johannes Berg, Jouni Malinen
[-- Attachment #1: Type: text/plain, Size: 2606 bytes --]
On Wed, Mar 28, 2012 at 2:17 AM, Timo Lindhorst <tlnd@online.de> wrote:
> 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 <tlnd@online.de>
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...
Signed-off-by: Javier Cardona <javier@cozybit.com>
Cheers,
Javier
> ---
> drivers/net/wireless/mac80211_hwsim.c | 15 +++++++++++++++
> 1 files changed, 15 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/wireless/mac80211_hwsim.c
> b/drivers/net/wireless/mac80211_hwsim.c
> index b7ce6a6..64adb3c 100644
> --- a/drivers/net/wireless/mac80211_hwsim.c
> +++ b/drivers/net/wireless/mac80211_hwsim.c
> @@ -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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Javier Cardona
cozybit Inc.
http://www.cozybit.com
[-- Attachment #2: rate.png --]
[-- Type: image/png, Size: 30849 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-05-07 23:17 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-28 9:17 [PATCH] mac80211_hwsim: Report rate info in tx status Timo Lindhorst
2012-03-28 9:28 ` Johannes Berg
2012-03-28 9:28 ` Timo Lindhorst
2012-03-28 9:32 ` Johannes Berg
2012-03-28 13:47 ` Timo Lindhorst
2012-04-30 18:34 ` John W. Linville
2012-05-03 19:33 ` Johannes Berg
2012-05-03 21:34 ` Javier Cardona
2012-05-07 23:17 ` Javier Cardona
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).
![[-- Attachment #2: rate.png --] [-- Type: image/png, Size: 30849 bytes --]](../../CAPjQAd-T7nKZtZ0pzVibMcw-ykCDF3HnW-yAiGmTLY2iLeq2XQ@mail.gmail.com/2-rate.png){kind=link}