Otherwise forwarded frames would keep the retry bit set
from the previous link transmission.
Signed-off-by: Thomas Pedersen <[email protected]>
---
net/mac80211/rx.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 5b4492a..5168f89 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2085,6 +2085,7 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
}
fwd_hdr = (struct ieee80211_hdr *) fwd_skb->data;
+ fwd_hdr->frame_control &= ~cpu_to_le16(IEEE80211_FCTL_RETRY);
info = IEEE80211_SKB_CB(fwd_skb);
memset(info, 0, sizeof(*info));
info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
--
1.7.10.4
Some drivers (like ath9k_htc) will take a mutex in
rate_control_rate_update(), so drop the sta lock before we
get there.
Signed-off-by: Thomas Pedersen <[email protected]>
Can go towards 3.9.
---
net/mac80211/mesh_plink.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index cdd4183..ca284ee 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -379,8 +379,10 @@ static void mesh_sta_info_init(struct ieee80211_sub_if_data *sdata,
sta->last_rx = jiffies;
/* rates and capabilities don't change during peering */
- if (sta->plink_state == NL80211_PLINK_ESTAB)
- goto out;
+ if (sta->plink_state == NL80211_PLINK_ESTAB) {
+ spin_unlock_bh(&sta->lock);
+ return;
+ }
if (sta->sta.supp_rates[band] != rates)
changed |= IEEE80211_RC_SUPP_RATES_CHANGED;
@@ -398,13 +400,12 @@ static void mesh_sta_info_init(struct ieee80211_sub_if_data *sdata,
changed |= IEEE80211_RC_BW_CHANGED;
sta->sta.bandwidth = IEEE80211_STA_RX_BW_20;
}
+ spin_unlock_bh(&sta->lock);
if (insert)
rate_control_rate_init(sta);
else
rate_control_rate_update(local, sband, sta, changed);
-out:
- spin_unlock_bh(&sta->lock);
}
static struct sta_info *
--
1.7.10.4
Have ieee80211_queue_stopped() return a bitmap of enum
queue_stop_reasons while checking queue status. This is
handy when we care about the queue stop reason codes.
Signed-off-by: Thomas Pedersen <[email protected]>
---
include/net/mac80211.h | 10 +++++++---
net/mac80211/util.c | 8 ++++----
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 64faf01..8bbe799 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3589,15 +3589,19 @@ void ieee80211_stop_queue(struct ieee80211_hw *hw, int queue);
/**
* ieee80211_queue_stopped - test status of the queue
+ *
* @hw: pointer as obtained from ieee80211_alloc_hw().
* @queue: queue number (counted from zero).
*
* Drivers should use this function instead of netif_stop_queue.
*
- * Return: %true if the queue is stopped. %false otherwise.
+ * Return 0 if queue not stopped, or else a bitmap of
+ * queue_stop_reasons.
+ *
+ * If @queue doesn't exist, return -1UL.
+ *
*/
-
-int ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue);
+unsigned long ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue);
/**
* ieee80211_stop_queues - stop all queues
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 447e665..739cddb 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -475,17 +475,17 @@ void ieee80211_stop_queues(struct ieee80211_hw *hw)
}
EXPORT_SYMBOL(ieee80211_stop_queues);
-int ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue)
+unsigned long ieee80211_queue_stopped(struct ieee80211_hw *hw, int queue)
{
struct ieee80211_local *local = hw_to_local(hw);
unsigned long flags;
- int ret;
+ unsigned long ret;
if (WARN_ON(queue >= hw->queues))
- return true;
+ return -1UL;
spin_lock_irqsave(&local->queue_stop_reason_lock, flags);
- ret = !!local->queue_stop_reasons[queue];
+ ret = local->queue_stop_reasons[queue];
spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
return ret;
}
--
1.7.10.4
On Wed, 2013-04-10 at 10:36 -0700, Thomas Pedersen wrote:
> > However it seems that ieee80211_queue_stopped() is actually kinda broken
> > and should only return the 'driver-stopped' reason to start with. If you
> > fix that, it's probably good enough for you. I don't think in patch 4
> > you should drop frames for scanning, for example.
>
> It looks like mac80211 already just checks local->queue_stop_reasons
> for being 0 (or not), so that check would still hold. I'm not 100%
> sure the driver calls to ieee80211_queue_stopped() only returning true
> for REASON_DRIVER are ok? What if the queues are being flushed, or say
> REASON_PS is set?
I looked at the drivers, and while (almost?) all of them use it
incorrectly they really want only REASON_DRIVER. In fact, they'd be
better off with just that even in the incorrect usages.
johannes
Since multicast aggregation is not currently supported by
mac80211, we can safely ignore them when reordering
aggregate MPDUs.
This fixes a bug where the expected sequence number might
be reset after processing a multicast frame from a STA
with an established aggregation session. The expected
sequence number would then be incorrect and all aggregated
frames until the sequence number rolled over would be
dropped.
Signed-off-by: Thomas Pedersen <[email protected]>
---
net/mac80211/rx.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 5168f89..cb55ef0 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -864,7 +864,8 @@ static void ieee80211_rx_reorder_ampdu(struct ieee80211_rx_data *rx,
u16 sc;
u8 tid, ack_policy;
- if (!ieee80211_is_data_qos(hdr->frame_control))
+ if (!ieee80211_is_data_qos(hdr->frame_control) ||
+ is_multicast_ether_addr(hdr->addr1))
goto dont_reorder;
/*
--
1.7.10.4
On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
> Some drivers (like ath9k_htc) will take a mutex in
> rate_control_rate_update(), so drop the sta lock before we
> get there.
That driver is broken then. rate_control_rate_update() can be called
from the RX path where this also isn't allowed.
johannes
In case of the HW asking mac80211 to back off, the queues
would be stopped with reason DRIVER.
Limit dropping forwarded mesh data frames by putting them
on the pending queue anyway if the outgoing HW queues
appear to be only momentarily stopped.
Signed-off-by: Thomas Pedersen <[email protected]>
---
net/mac80211/rx.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index cb55ef0..8c7f80c 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1997,6 +1997,7 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
__le16 reason = cpu_to_le16(WLAN_REASON_MESH_PATH_NOFORWARD);
u16 q, hdrlen;
+ unsigned long qreason;
hdr = (struct ieee80211_hdr *) skb->data;
hdrlen = ieee80211_hdrlen(hdr->frame_control);
@@ -2064,7 +2065,9 @@ ieee80211_rx_h_mesh_fwding(struct ieee80211_rx_data *rx)
return RX_CONTINUE;
q = ieee80211_select_queue_80211(sdata, skb, hdr);
- if (ieee80211_queue_stopped(&local->hw, q)) {
+ qreason = ieee80211_queue_stopped(&local->hw, q);
+ if (qreason & ~(IEEE80211_QUEUE_STOP_REASON_SKB_ADD |
+ IEEE80211_QUEUE_STOP_REASON_AGGREGATION)) {
IEEE80211_IFSTA_MESH_CTR_INC(ifmsh, dropped_frames_congestion);
return RX_DROP_MONITOR;
}
--
1.7.10.4
Subject should probably say "reordering" rather than "accounting"
> Since multicast aggregation is not currently supported by
> mac80211, we can safely ignore them when reordering
> aggregate MPDUs.
Multicast may have a lower expectation of ordering than unicast, but is
this really the right thing to do? It seems better to just fix it?
johannes
On Mon, Apr 8, 2013 at 11:37 AM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
>> Have ieee80211_queue_stopped() return a bitmap of enum
>> queue_stop_reasons while checking queue status. This is
>> handy when we care about the queue stop reason codes.
>
> Nack. The driver must not care about the reasons, they're purely
> internal to mac80211.
OK those functions are exported to the drivers. The 'enum
queue_stop_reason' is defined in ieee80211_i.h, so the driver wouldn't
be able to interpret them anyway?
Would you prefer a utility function internal to mac80211 which does
return the reason, or just the following pattern?
if (ieee80211_queue_stopped(hw, queue)) {
qreason = hw_to_local(hw)->queue_stop_reasons[queue];
if (qreason & ~(ALLOWED_QUEUE_STOP_REASONS))
something;
}
Thanks,
--
Thomas
On Mon, 2013-04-08 at 12:30 -0700, Thomas Pedersen wrote:
> OK those functions are exported to the drivers. The 'enum
> queue_stop_reason' is defined in ieee80211_i.h, so the driver wouldn't
> be able to interpret them anyway?
Oh, you're way underestimating the creativity of driver authors :-)
> Would you prefer a utility function internal to mac80211 which does
> return the reason,
That seems fine, although a bit more inefficient?
> or just the following pattern?
>
> if (ieee80211_queue_stopped(hw, queue)) {
> qreason = hw_to_local(hw)->queue_stop_reasons[queue];
> if (qreason & ~(ALLOWED_QUEUE_STOP_REASONS))
> something;
> }
That seems racy? Even asking whether it's stopped is racy though, what
are you even trying to accomplish?
johannes
On Tue, Apr 9, 2013 at 2:37 AM, Johannes Berg <[email protected]> wrote:
> On Mon, 2013-04-08 at 12:57 -0700, Thomas Pedersen wrote:
>
>> > That seems racy? Even asking whether it's stopped is racy though, what
>> > are you even trying to accomplish?
>>
>> Yeah, that's why I'd like to get the reason the first time.
>>
>> _h_mesh_fwding() checks whether the outgoing queue is stopped, to
>> avoid piling frames on the pending queue if the outgoing medium is
>> busy. I guess the idea was to avoid queueing frames faster than the
>> hardware could unload them. Maybe this doesn't actually happen, but we
>> can be a little bit smarter about when to drop forwarded frames. Like
>> if skbs are just being added to the outgoing queue, we probably
>> shouldn't.
>
> Technically I guess that can happen if your inbound link is better than
> the outbound one? But it'll depend on the AC parameters etc. too.
>
> However it seems that ieee80211_queue_stopped() is actually kinda broken
> and should only return the 'driver-stopped' reason to start with. If you
> fix that, it's probably good enough for you. I don't think in patch 4
> you should drop frames for scanning, for example.
It looks like mac80211 already just checks local->queue_stop_reasons
for being 0 (or not), so that check would still hold. I'm not 100%
sure the driver calls to ieee80211_queue_stopped() only returning true
for REASON_DRIVER are ok? What if the queues are being flushed, or say
REASON_PS is set?
--
Thomas
On Tue, Apr 9, 2013 at 2:47 AM, Johannes Berg <[email protected]> wrote:
> Subject should probably say "reordering" rather than "accounting"
>
>> Since multicast aggregation is not currently supported by
>> mac80211, we can safely ignore them when reordering
>> aggregate MPDUs.
>
> Multicast may have a lower expectation of ordering than unicast, but is
> this really the right thing to do? It seems better to just fix it?
It appears one of our changes actually introduced this bug! It is not
reproducible with mac80211-next. Thanks for making me take a closer
look at this.
--
Thomas
On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
> Otherwise forwarded frames would keep the retry bit set
> from the previous link transmission.
Applied.
johannes
Hi Thomas,
On Mon, Apr 08, 2013 at 11:06:14AM -0700, Thomas Pedersen wrote:
> Have ieee80211_queue_stopped() return a bitmap of enum
> queue_stop_reasons while checking queue status. This is
> handy when we care about the queue stop reason codes.
>
> Signed-off-by: Thomas Pedersen <[email protected]>
> ---
> include/net/mac80211.h | 10 +++++++---
> net/mac80211/util.c | 8 ++++----
> 2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 64faf01..8bbe799 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -3589,15 +3589,19 @@ void ieee80211_stop_queue(struct ieee80211_hw *hw, int queue);
>
> /**
> * ieee80211_queue_stopped - test status of the queue
> + *
I think you added this new line by mistake.
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
On Mon, Apr 8, 2013 at 12:38 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2013-04-08 at 12:30 -0700, Thomas Pedersen wrote:
>
>> OK those functions are exported to the drivers. The 'enum
>> queue_stop_reason' is defined in ieee80211_i.h, so the driver wouldn't
>> be able to interpret them anyway?
>
> Oh, you're way underestimating the creativity of driver authors :-)
>
>> Would you prefer a utility function internal to mac80211 which does
>> return the reason,
>
> That seems fine, although a bit more inefficient?
>
>> or just the following pattern?
>>
>> if (ieee80211_queue_stopped(hw, queue)) {
>> qreason = hw_to_local(hw)->queue_stop_reasons[queue];
>> if (qreason & ~(ALLOWED_QUEUE_STOP_REASONS))
>> something;
>> }
>
> That seems racy? Even asking whether it's stopped is racy though, what
> are you even trying to accomplish?
Yeah, that's why I'd like to get the reason the first time.
_h_mesh_fwding() checks whether the outgoing queue is stopped, to
avoid piling frames on the pending queue if the outgoing medium is
busy. I guess the idea was to avoid queueing frames faster than the
hardware could unload them. Maybe this doesn't actually happen, but we
can be a little bit smarter about when to drop forwarded frames. Like
if skbs are just being added to the outgoing queue, we probably
shouldn't.
This patch and the next one show a small improvement in throughput and
loss % in the forwarding path.
--
Thomas
On Mon, 2013-04-08 at 12:57 -0700, Thomas Pedersen wrote:
> > That seems racy? Even asking whether it's stopped is racy though, what
> > are you even trying to accomplish?
>
> Yeah, that's why I'd like to get the reason the first time.
>
> _h_mesh_fwding() checks whether the outgoing queue is stopped, to
> avoid piling frames on the pending queue if the outgoing medium is
> busy. I guess the idea was to avoid queueing frames faster than the
> hardware could unload them. Maybe this doesn't actually happen, but we
> can be a little bit smarter about when to drop forwarded frames. Like
> if skbs are just being added to the outgoing queue, we probably
> shouldn't.
Technically I guess that can happen if your inbound link is better than
the outbound one? But it'll depend on the AC parameters etc. too.
However it seems that ieee80211_queue_stopped() is actually kinda broken
and should only return the 'driver-stopped' reason to start with. If you
fix that, it's probably good enough for you. I don't think in patch 4
you should drop frames for scanning, for example.
johannes
The patch "mac80211: stringify mesh peering events" missed
an opportunity to print the peering state as a string.
Signed-off-by: Thomas Pedersen <[email protected]>
---
net/mac80211/mesh_plink.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mac80211/mesh_plink.c b/net/mac80211/mesh_plink.c
index 937e06f..cdd4183 100644
--- a/net/mac80211/mesh_plink.c
+++ b/net/mac80211/mesh_plink.c
@@ -544,8 +544,8 @@ static void mesh_plink_timer(unsigned long data)
return;
}
mpl_dbg(sta->sdata,
- "Mesh plink timer for %pM fired on state %d\n",
- sta->sta.addr, sta->plink_state);
+ "Mesh plink timer for %pM fired on state %s\n",
+ sta->sta.addr, mplstates[sta->plink_state]);
reason = 0;
llid = sta->llid;
plid = sta->plid;
--
1.7.10.4
On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
> Have ieee80211_queue_stopped() return a bitmap of enum
> queue_stop_reasons while checking queue status. This is
> handy when we care about the queue stop reason codes.
Nack. The driver must not care about the reasons, they're purely
internal to mac80211.
johannes
On Tue, 2013-04-09 at 11:38 +0200, Johannes Berg wrote:
> On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
> > Some drivers (like ath9k_htc) will take a mutex in
> > rate_control_rate_update(), so drop the sta lock before we
> > get there.
>
> That driver is broken then. rate_control_rate_update() can be called
> from the RX path where this also isn't allowed.
* @sta_rc_update: Notifies the driver of changes to the bitrates that
can be
* used to transmit to the station. The changes are advertised with
bits
* from &enum ieee80211_rate_control_changed and the values are
reflected
* in the station data. This callback should only be used when the
driver
* uses hardware rate control (%IEEE80211_HW_HAS_RATE_CONTROL)
since
* otherwise the rate control algorithm is notified directly.
* Must be atomic.
(note last line)
johannes
On Tue, Apr 9, 2013 at 2:42 AM, Johannes Berg <[email protected]> wrote:
> On Tue, 2013-04-09 at 11:38 +0200, Johannes Berg wrote:
>> On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
>> > Some drivers (like ath9k_htc) will take a mutex in
>> > rate_control_rate_update(), so drop the sta lock before we
>> > get there.
>>
>> That driver is broken then. rate_control_rate_update() can be called
>> from the RX path where this also isn't allowed.
>
> * @sta_rc_update: Notifies the driver of changes to the bitrates that
> can be
> * used to transmit to the station. The changes are advertised with
> bits
> * from &enum ieee80211_rate_control_changed and the values are
> reflected
> * in the station data. This callback should only be used when the
> driver
> * uses hardware rate control (%IEEE80211_HW_HAS_RATE_CONTROL)
> since
> * otherwise the rate control algorithm is notified directly.
> * Must be atomic.
>
> (note last line)
Ah alright. Thanks.
--
Thomas
On Mon, 2013-04-08 at 11:06 -0700, Thomas Pedersen wrote:
> The patch "mac80211: stringify mesh peering events" missed
> an opportunity to print the peering state as a string.
Applied.
johannes