[PATCH] mac80211_hwsim: Report rate info in tx status

[PATCH] mac80211_hwsim: Report rate info in tx status

[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 <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

Re: [PATCH] mac80211_hwsim: Report rate info in tx status

[Johannes Berg]

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

Re: [PATCH] mac80211_hwsim: Report rate info in tx status

[Timo Lindhorst]

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

Re: [PATCH] mac80211_hwsim: Report rate info in tx status

[Johannes Berg]

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

Re: [PATCH] mac80211_hwsim: Report rate info in tx status

[Timo Lindhorst]

> > > +     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

Re: [PATCH] mac80211_hwsim: Report rate info in tx status

[John W. Linville]

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.

Re: [PATCH] mac80211_hwsim: Report rate info in tx status

[Johannes Berg]

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

Re: [PATCH] mac80211_hwsim: Report rate info in tx status

[Javier Cardona]

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

Re: [PATCH] mac80211_hwsim: Report rate info in tx status

[Javier Cardona]

[-- 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 --]