Re: [PATCHv3 4/5] mac80211: implement codel on fair queuing flows
On Tue, 2016-04-19 at 11:31 +0200, Michal Kazior wrote: > > We should just get rid of all the rate stuff and convert everything > > to use rate tables, but ... :) > I'm guessing it's not trivial either and you risk breaking a lot of > stuff? :) It's not "tricky" in the same sense - but we'd have to convert drivers to use a different data structure etc. Quite a bit of work. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/5] mac80211: implement codel on fair queuing flows
On 19 April 2016 at 11:06, Johannes Berg wrote: > On Mon, 2016-04-18 at 14:38 +0200, Michal Kazior wrote: >> On 18 April 2016 at 07:31, Michal Kazior >> wrote: >> > >> > On 17 April 2016 at 00:29, Johannes Berg > > > wrote: >> > > >> > > On Thu, 2016-04-14 at 14:18 +0200, Michal Kazior wrote: >> > > > >> > > > >> > > > + struct ieee80211_vif *vif; >> > > > + >> > > > + /* When packets are enqueued on >> > > > txq >> > > > it's easy >> > > > + * to re-construct the vif >> > > > pointer. >> > > > There's no >> > > > + * more space in tx_info so it >> > > > can >> > > > be used to >> > > > + * store the necessary enqueue >> > > > time >> > > > for packet >> > > > + * sojourn time computation. >> > > > + */ >> > > > + u64 enqueue_time; >> > > > + }; >> > > I wonder if we could move something like the hw_key into >> > > tx_control >> > > instead? >> > Hmm.. It's probably doable. From a quick look it'll require quite >> > some >> > change here and there (e.g. tdls_channel_switch op will need to be >> > extended to pass tx_control). I'll play with the idea.. >> This is actually far more than I thought initially. > > Fair enough. Perhaps it could be done for the vif? But ISTR there were > issues with that when I looked. Still tricky in a similar fashion as hw_key. > We should just get rid of all the rate stuff and convert everything to > use rate tables, but ... :) I'm guessing it's not trivial either and you risk breaking a lot of stuff? :) >> A lot of drivers >> (b43, b43legacy, rtlwifi, wl, cw1200) access hw_key outside of tx >> op context (tx workers, tx completions). I'm not even sure this is >> safe (keys can be freed in the meantime by mac80211 hence invaliding >> the pointer inside skb, no?). >> > > Hm, yeah, that does seem problematic unless they synchronize against > key removal somehow? I didn't see any explicit synchronization but maybe I missed something. Michał -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/5] mac80211: implement codel on fair queuing flows
On Mon, 2016-04-18 at 14:38 +0200, Michal Kazior wrote: > On 18 April 2016 at 07:31, Michal Kazior > wrote: > > > > On 17 April 2016 at 00:29, Johannes Berg > > wrote: > > > > > > On Thu, 2016-04-14 at 14:18 +0200, Michal Kazior wrote: > > > > > > > > > > > > + struct ieee80211_vif *vif; > > > > + > > > > + /* When packets are enqueued on > > > > txq > > > > it's easy > > > > + * to re-construct the vif > > > > pointer. > > > > There's no > > > > + * more space in tx_info so it > > > > can > > > > be used to > > > > + * store the necessary enqueue > > > > time > > > > for packet > > > > + * sojourn time computation. > > > > + */ > > > > + u64 enqueue_time; > > > > + }; > > > I wonder if we could move something like the hw_key into > > > tx_control > > > instead? > > Hmm.. It's probably doable. From a quick look it'll require quite > > some > > change here and there (e.g. tdls_channel_switch op will need to be > > extended to pass tx_control). I'll play with the idea.. > This is actually far more than I thought initially. Fair enough. Perhaps it could be done for the vif? But ISTR there were issues with that when I looked. We should just get rid of all the rate stuff and convert everything to use rate tables, but ... :) > A lot of drivers > (b43, b43legacy, rtlwifi, wl, cw1200) access hw_key outside of tx > op context (tx workers, tx completions). I'm not even sure this is > safe (keys can be freed in the meantime by mac80211 hence invaliding > the pointer inside skb, no?). > Hm, yeah, that does seem problematic unless they synchronize against key removal somehow? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/5] mac80211: implement codel on fair queuing flows
On 18 April 2016 at 07:31, Michal Kazior wrote: > On 17 April 2016 at 00:29, Johannes Berg wrote: >> On Thu, 2016-04-14 at 14:18 +0200, Michal Kazior wrote: >>> >>> + struct ieee80211_vif *vif; >>> + >>> + /* When packets are enqueued on txq >>> it's easy >>> + * to re-construct the vif pointer. >>> There's no >>> + * more space in tx_info so it can >>> be used to >>> + * store the necessary enqueue time >>> for packet >>> + * sojourn time computation. >>> + */ >>> + u64 enqueue_time; >>> + }; >> >> I wonder if we could move something like the hw_key into tx_control >> instead? > > Hmm.. It's probably doable. From a quick look it'll require quite some > change here and there (e.g. tdls_channel_switch op will need to be > extended to pass tx_control). I'll play with the idea.. This is actually far more than I thought initially. A lot of drivers (b43, b43legacy, rtlwifi, wl, cw1200) access hw_key outside of tx op context (tx workers, tx completions). I'm not even sure this is safe (keys can be freed in the meantime by mac80211 hence invaliding the pointer inside skb, no?). Michał -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/5] mac80211: implement codel on fair queuing flows
On 17 April 2016 at 00:29, Johannes Berg wrote: > On Thu, 2016-04-14 at 14:18 +0200, Michal Kazior wrote: >> >> + struct ieee80211_vif *vif; >> + >> + /* When packets are enqueued on txq >> it's easy >> + * to re-construct the vif pointer. >> There's no >> + * more space in tx_info so it can >> be used to >> + * store the necessary enqueue time >> for packet >> + * sojourn time computation. >> + */ >> + u64 enqueue_time; >> + }; > > I wonder if we could move something like the hw_key into tx_control > instead? Hmm.. It's probably doable. From a quick look it'll require quite some change here and there (e.g. tdls_channel_switch op will need to be extended to pass tx_control). I'll play with the idea.. Michał -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3 4/5] mac80211: implement codel on fair queuing flows
On Thu, 2016-04-14 at 14:18 +0200, Michal Kazior wrote: > > + struct ieee80211_vif *vif; > + > + /* When packets are enqueued on txq > it's easy > + * to re-construct the vif pointer. > There's no > + * more space in tx_info so it can > be used to > + * store the necessary enqueue time > for packet > + * sojourn time computation. > + */ > + u64 enqueue_time; > + }; I wonder if we could move something like the hw_key into tx_control instead? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv3 4/5] mac80211: implement codel on fair queuing flows
There is no other limit other than a global
packet count limit when using software queuing.
This means a single flow queue can grow insanely
long. This is particularly bad for TCP congestion
algorithms which requires a little more
sophisticated frame dropping scheme than a mere
headdrop on limit overflow.
Hence apply (a slighly modified, to fit the knobs)
CoDel5 on flow queues. This improves TCP
convergence and stability when combined with
wireless driver which keeps its own tx queue/fifo
at a minimum fill level for given link conditions.
Signed-off-by: Michal Kazior
---
include/net/mac80211.h | 13 ++-
net/mac80211/codel.h | 265 +
net/mac80211/codel_i.h | 100 +
net/mac80211/ieee80211_i.h | 5 +
net/mac80211/tx.c | 99 -
5 files changed, 480 insertions(+), 2 deletions(-)
create mode 100644 net/mac80211/codel.h
create mode 100644 net/mac80211/codel_i.h
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index c24d0b8e4deb..d53b14bc4e79 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -889,7 +889,18 @@ struct ieee80211_tx_info {
unsigned long jiffies;
};
/* NB: vif can be NULL for injected frames */
- struct ieee80211_vif *vif;
+ union {
+ /* NB: vif can be NULL for injected frames */
+ struct ieee80211_vif *vif;
+
+ /* When packets are enqueued on txq it's easy
+* to re-construct the vif pointer. There's no
+* more space in tx_info so it can be used to
+* store the necessary enqueue time for packet
+* sojourn time computation.
+*/
+ u64 enqueue_time;
+ };
struct ieee80211_key_conf *hw_key;
u32 flags;
/* 4 bytes free */
diff --git a/net/mac80211/codel.h b/net/mac80211/codel.h
new file mode 100644
index ..63ccedcbce04
--- /dev/null
+++ b/net/mac80211/codel.h
@@ -0,0 +1,265 @@
+#ifndef __NET_MAC80211_CODEL_H
+#define __NET_MAC80211_CODEL_H
+
+/*
+ * Codel - The Controlled-Delay Active Queue Management algorithm
+ *
+ * Copyright (C) 2011-2012 Kathleen Nichols
+ * Copyright (C) 2011-2012 Van Jacobson
+ * Copyright (C) 2016 Michael D. Taht
+ * Copyright (C) 2012 Eric Dumazet
+ * Copyright (C) 2015 Jonathan Morton
+ * Copyright (C) 2016 Michal Kazior
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions, and the following disclaimer,
+ *without modification.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ * 3. The names of the authors may not be used to endorse or promote products
+ *derived from this software without specific prior written permission.
+ *
+ * Alternatively, provided that this notice is retained in full, this
+ * software may be distributed under the terms of the GNU General
+ * Public License ("GPL") version 2, in which case the provisions of the
+ * GPL apply INSTEAD OF those given above.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
+ * DAMAGE.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "codel_i.h"
+
+/* Controlling Queue Delay (CoDel) algorithm
+ * =
+ * Source : Kathleen Nichols and Van Jacobson
+ * http://queue.acm.org/detail.cfm?id=2209336
+ *
+ * Implemented on linux by Dave Taht and Eric Dumazet
+ */
+
+/* CoDel5 uses a real clock, unlike codel */
+
+static inline u64 codel_get_time(void)
+{
+ return ktime_get_ns();
+}
+
