[PATCH] mac80211: Update last_tx_rate only for data frames

[PATCH] mac80211: Update last_tx_rate only for data frames

[Helmut Schaa]

The last_tx_rate field was also updated for non-data frames that are
often sent with a lower rate (for example management frames at 1 Mbps).
This is confusing when the data rate is actually much higher.

Hence, only update the last_tx_rate field with tx rate information
gathered from the last data frames.

Cc: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---

Not sure if there are any reasons why somebody would like to see the "real"
last tx rate including management frames?

 net/mac80211/tx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index e694836..f753081 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -669,7 +669,7 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
 	if (txrc.reported_rate.idx < 0)
 		txrc.reported_rate = info->control.rates[0];
 
-	if (tx->sta)
+	if (tx->sta && ieee80211_is_data(hdr->frame_control))
 		tx->sta->last_tx_rate = txrc.reported_rate;
 
 	if (unlikely(!info->control.rates[0].count))
-- 
1.7.1

Re: [PATCH] mac80211: Update last_tx_rate only for data frames

[Mohammed Shafi]

On Wed, Dec 1, 2010 at 4:48 PM, Helmut Schaa
<helmut.schaa@googlemail.com> wrote:
> The last_tx_rate field was also updated for non-data frames that are
> often sent with a lower rate (for example management frames at 1 Mbps).
> This is confusing when the data rate is actually much higher.
>
> Hence, only update the last_tx_rate field with tx rate information
> gathered from the last data frames.
Hi Helmut,
            I have a doubt,ideally should not this be taken care by the driver ?
thanks,
shafi
>
> Cc: Johannes Berg <johannes@sipsolutions.net>
> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
> ---
>
> Not sure if there are any reasons why somebody would like to see the "real"
> last tx rate including management frames?
>
>  net/mac80211/tx.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index e694836..f753081 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -669,7 +669,7 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
>        if (txrc.reported_rate.idx < 0)
>                txrc.reported_rate = info->control.rates[0];
>
> -       if (tx->sta)
> +       if (tx->sta && ieee80211_is_data(hdr->frame_control))
>                tx->sta->last_tx_rate = txrc.reported_rate;
>
>        if (unlikely(!info->control.rates[0].count))
> --
> 1.7.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
>

Re: [PATCH] mac80211: Update last_tx_rate only for data frames

[Helmut Schaa]

Hi,

Am Mittwoch 01 Dezember 2010 schrieb Mohammed Shafi:
> On Wed, Dec 1, 2010 at 4:48 PM, Helmut Schaa
> <helmut.schaa@googlemail.com> wrote:
> > The last_tx_rate field was also updated for non-data frames that are
> > often sent with a lower rate (for example management frames at 1 Mbps).
> > This is confusing when the data rate is actually much higher.
> >
> > Hence, only update the last_tx_rate field with tx rate information
> > gathered from the last data frames.
>
> Hi Helmut,
>             I have a doubt,ideally should not this be taken care by the driver ?

Sorry, I don't get your point. How should that be handled by the driver? Could
you please elaborate?

last_tx_rate is part of the sta_info struct and is documented as:

207  * @last_tx_rate: rate used for last transmit, to report to userspace as
208  *      "the" transmit rate

So, the fields sole purpose is to report the "current" tx rate to user space.
A normal user (IMO) would like to see the current tx rate that is used for
data frames and not occasionally a 1Mbps because a management frame was the
last sent frame.

Helmut

Re: [PATCH] mac80211: Update last_tx_rate only for data frames

[Mohammed Shafi]

On Wed, Dec 1, 2010 at 8:10 PM, Helmut Schaa
<helmut.schaa@googlemail.com> wrote:
> Hi,
>
> Am Mittwoch 01 Dezember 2010 schrieb Mohammed Shafi:
>> On Wed, Dec 1, 2010 at 4:48 PM, Helmut Schaa
>> <helmut.schaa@googlemail.com> wrote:
>> > The last_tx_rate field was also updated for non-data frames that are
>> > often sent with a lower rate (for example management frames at 1 Mbps).
>> > This is confusing when the data rate is actually much higher.
>> >
>> > Hence, only update the last_tx_rate field with tx rate information
>> > gathered from the last data frames.
>>
>> Hi Helmut,
>>             I have a doubt,ideally should not this be taken care by the driver ?
>
> Sorry, I don't get your point. How should that be handled by the driver? Could
> you please elaborate?

Thanks for your reply.I don't know really ,just asked  , but saw in
ath9k "ath_tx_status"(call back to mac80211) which might be taking
care of it

static void ath_tx_status(void *priv, struct ieee80211_supported_band *sband,
                          struct ieee80211_sta *sta, void *priv_sta,
                          struct sk_buff *skb)
{
........
        for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
                struct ieee80211_tx_rate *rate = &tx_info->status.rates[i];
                if (!rate->count)
                        break;

                final_ts_idx = i;
                long_retry = rate->count - 1;
        }

        if (!priv_sta || !ieee80211_is_data(fc))
                return;
....etc

>
> last_tx_rate is part of the sta_info struct and is documented as:
>
> 207  * @last_tx_rate: rate used for last transmit, to report to userspace as
> 208  *      "the" transmit rate
>
> So, the fields sole purpose is to report the "current" tx rate to user space.
> A normal user (IMO) would like to see the current tx rate that is used for
> data frames and not occasionally a 1Mbps because a management frame was the
> last sent frame.
>
> Helmut
>

Re: [PATCH] mac80211: Update last_tx_rate only for data frames

[Helmut Schaa]

Am Mittwoch 01 Dezember 2010 schrieb Helmut Schaa:
> Am Mittwoch 01 Dezember 2010 schrieb Mohammed Shafi:
> > On Wed, Dec 1, 2010 at 4:48 PM, Helmut Schaa
> > <helmut.schaa@googlemail.com> wrote:
> > > The last_tx_rate field was also updated for non-data frames that are
> > > often sent with a lower rate (for example management frames at 1 Mbps).
> > > This is confusing when the data rate is actually much higher.
> > >
> > > Hence, only update the last_tx_rate field with tx rate information
> > > gathered from the last data frames.
> >
> > Hi Helmut,
> >             I have a doubt,ideally should not this be taken care by the driver ?
> 
> Sorry, I don't get your point. How should that be handled by the driver? Could
> you please elaborate?

Ahh, did you mean that should be taken care of by the rate control algorithm?

I agree, if the rate control algortihm returns a rate via txrc.reported_rate
we should just use that without further validation.

Nevertheless, if the rate control algortihm doesn't fill in txrc.reported_rate
we should only account data frames.

Will send an updated patch.

Helmut

Re: [PATCH] mac80211: Update last_tx_rate only for data frames

[Mohammed Shafi]

On Wed, Dec 1, 2010 at 8:44 PM, Helmut Schaa
<helmut.schaa@googlemail.com> wrote:
> Am Mittwoch 01 Dezember 2010 schrieb Helmut Schaa:
>> Am Mittwoch 01 Dezember 2010 schrieb Mohammed Shafi:
>> > On Wed, Dec 1, 2010 at 4:48 PM, Helmut Schaa
>> > <helmut.schaa@googlemail.com> wrote:
>> > > The last_tx_rate field was also updated for non-data frames that are
>> > > often sent with a lower rate (for example management frames at 1 Mbps).
>> > > This is confusing when the data rate is actually much higher.
>> > >
>> > > Hence, only update the last_tx_rate field with tx rate information
>> > > gathered from the last data frames.
>> >
>> > Hi Helmut,
>> >             I have a doubt,ideally should not this be taken care by the driver ?
>>
>> Sorry, I don't get your point. How should that be handled by the driver? Could
>> you please elaborate?
>
> Ahh, did you mean that should be taken care of by the rate control algorithm?
>
I think so but not sure..(for ex: if ministrel is used)

> I agree, if the rate control algortihm returns a rate via txrc.reported_rate
> we should just use that without further validation.
>
> Nevertheless, if the rate control algortihm doesn't fill in txrc.reported_rate
> we should only account data frames.

I agree.

>
> Will send an updated patch.
>
> Helmut
>

[PATCHv2] mac80211: Update last_tx_rate only for data frames

[Helmut Schaa]

The last_tx_rate field was also updated for non-data frames that are
often sent with a lower rate (for example management frames at 1 Mbps).
This is confusing when the data rate is actually much higher.

Hence, only update the last_tx_rate field with tx rate information
gathered from last data frames.

If the rate control algorithm filled in txrc.reported_rate we don't need
to verify this information.

Cc: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>
---
 net/mac80211/tx.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index e694836..41bfbbf 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -666,10 +666,11 @@ ieee80211_tx_h_rate_ctrl(struct ieee80211_tx_data *tx)
 	if (unlikely(info->control.rates[0].idx < 0))
 		return TX_DROP;
 
-	if (txrc.reported_rate.idx < 0)
+	if (txrc.reported_rate.idx < 0) {
 		txrc.reported_rate = info->control.rates[0];
-
-	if (tx->sta)
+		if (tx->sta && ieee80211_is_data(hdr->frame_control))
+			tx->sta->last_tx_rate = txrc.reported_rate;
+	} else if (tx->sta)
 		tx->sta->last_tx_rate = txrc.reported_rate;
 
 	if (unlikely(!info->control.rates[0].count))
-- 
1.7.1

Re: [PATCH] mac80211: Update last_tx_rate only for data frames

[Larry Finger]

On 12/01/2010 08:40 AM, Helmut Schaa wrote:
> Hi,
> 
> Am Mittwoch 01 Dezember 2010 schrieb Mohammed Shafi:
>> On Wed, Dec 1, 2010 at 4:48 PM, Helmut Schaa
>> <helmut.schaa@googlemail.com> wrote:
>>> The last_tx_rate field was also updated for non-data frames that are
>>> often sent with a lower rate (for example management frames at 1 Mbps).
>>> This is confusing when the data rate is actually much higher.
>>>
>>> Hence, only update the last_tx_rate field with tx rate information
>>> gathered from the last data frames.
>>
>> Hi Helmut,
>>             I have a doubt,ideally should not this be taken care by the driver ?
> 
> Sorry, I don't get your point. How should that be handled by the driver? Could
> you please elaborate?
> 
> last_tx_rate is part of the sta_info struct and is documented as:
> 
> 207  * @last_tx_rate: rate used for last transmit, to report to userspace as
> 208  *      "the" transmit rate
> 
> So, the fields sole purpose is to report the "current" tx rate to user space.
> A normal user (IMO) would like to see the current tx rate that is used for
> data frames and not occasionally a 1Mbps because a management frame was the
> last sent frame.

In the openSUSE forums, we occasionally get a confused user that says "the
wireless only runs at 1 Mb/s" because that is what iwconfig says. On inspection,
the throughput is much higher. I vote for reporting the last data frame rate and
ignore management frames.