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]>
---
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
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
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
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
> > > + 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
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
[email protected] might be all we have. Be ready.
On Thu, May 3, 2012 at 12:33 PM, Johannes Berg
<[email protected]> 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
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
On Wed, Mar 28, 2012 at 2:17 AM, Timo Lindhorst <[email protected]> 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 <[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...
Signed-off-by: Javier Cardona <[email protected]>
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 [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
--
Javier Cardona
cozybit Inc.
http://www.cozybit.com