2010-12-01 11:18:37

by Helmut Schaa

[permalink] [raw]
Subject: [PATCH] mac80211: Update last_tx_rate only for data frames

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 <[email protected]>
Signed-off-by: Helmut Schaa <[email protected]>
---

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



2010-12-01 15:15:15

by Helmut Schaa

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

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

2010-12-01 15:21:49

by Mohammed Shafi

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

On Wed, Dec 1, 2010 at 8:44 PM, Helmut Schaa
<[email protected]> 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
>> > <[email protected]> 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
>

2010-12-01 14:04:25

by Mohammed Shafi

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

On Wed, Dec 1, 2010 at 4:48 PM, Helmut Schaa
<[email protected]> 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 <[email protected]>
> Signed-off-by: Helmut Schaa <[email protected]>
> ---
>
> 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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>

2010-12-01 14:41:53

by Helmut Schaa

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

Hi,

Am Mittwoch 01 Dezember 2010 schrieb Mohammed Shafi:
> On Wed, Dec 1, 2010 at 4:48 PM, Helmut Schaa
> <[email protected]> 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

2010-12-01 16:55:10

by Larry Finger

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

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
>> <[email protected]> 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.

2010-12-01 15:35:02

by Helmut Schaa

[permalink] [raw]
Subject: [PATCHv2] mac80211: Update last_tx_rate only for data frames

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 <[email protected]>
Signed-off-by: Helmut Schaa <[email protected]>
---
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


2010-12-01 15:03:44

by Mohammed Shafi

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

On Wed, Dec 1, 2010 at 8:10 PM, Helmut Schaa
<[email protected]> wrote:
> Hi,
>
> Am Mittwoch 01 Dezember 2010 schrieb Mohammed Shafi:
>> On Wed, Dec 1, 2010 at 4:48 PM, Helmut Schaa
>> <[email protected]> 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
>