Re: [PATCHv3 1/5] mac80211: skip netdev queue control with software queuing

2016-04-17 Thread Michal Kazior 
On 17 April 2016 at 00:21, Johannes Berg  wrote:
>> +static void ieee80211_txq_enqueue(struct ieee80211_local *local,
>> +   struct txq_info *txqi,
>> +   struct sk_buff *skb)
>> +{
>> + lockdep_assert_held(>queue.lock);
> [...]
>> + atomic_inc(>num_tx_queued);
>
> This global kinda bothers me - anything we can do about removing it?

I don't think so. Re-counting via sta/vif/txq iteration every time is
rather a bad idea.

FWIW This is removed by the "fq" patch. The main purpose of taildrop
patch is to make some comparisons easier.


> We obviously didn't have it now - just one (even bigger limit!) per
> queue, so that's 4000 frames default per interface ... now you're down
> to 512 for the entire hardware. Perhaps keeping it per interface at
> least gets away the worst of the contention here?

The default qdisc limits were arguably already too big anyway.
Nevertheless it makes sense to have the 512 limit per interface
instead of per radio. I'll move num_tx_queued to sdata.


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 1/5] mac80211: skip netdev queue control with software queuing

2016-04-16 Thread Johannes Berg 
> +static void ieee80211_txq_enqueue(struct ieee80211_local *local,
> +   struct txq_info *txqi,
> +   struct sk_buff *skb)
> +{
> + lockdep_assert_held(>queue.lock);
[...]
> + atomic_inc(>num_tx_queued);

This global kinda bothers me - anything we can do about removing it?

We obviously didn't have it now - just one (even bigger limit!) per
queue, so that's 4000 frames default per interface ... now you're down
to 512 for the entire hardware. Perhaps keeping it per interface at
least gets away the worst of the contention here?

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 1/5] mac80211: skip netdev queue control with software queuing

2016-04-14 Thread Michal Kazior 
Qdiscs are designed with no regard to 802.11
aggregation requirements and hand out
packet-by-packet with no guarantee they are
destined to the same tid. This does more bad than
good no matter how fairly a given qdisc may behave
on an ethernet interface.

Software queuing used per-AC netdev subqueue
congestion control whenever a global AC limit was
hit. This meant in practice a single station or
tid queue could starve others rather easily. This
could resonate with qdiscs in a bad way or could
just end up with poor aggregation performance.
Increasing the AC limit would increase induced
latency which is also bad.

Disabling qdiscs by default and performing
taildrop instead of netdev subqueue congestion
control on the other hand makes it possible for
tid queues to fill up "in the meantime" while
preventing stations starving each other.

This increases aggregation opportunities and
should allow software queuing based drivers
achieve better performance by utilizing airtime
more efficiently with big aggregates.

Signed-off-by: Michal Kazior 
---
 include/net/mac80211.h |  4 ---
 net/mac80211/ieee80211_i.h |  2 +-
 net/mac80211/iface.c   | 18 +--
 net/mac80211/main.c|  3 --
 net/mac80211/sta_info.c|  2 +-
 net/mac80211/tx.c  | 80 --
 net/mac80211/util.c| 11 ---
 7 files changed, 65 insertions(+), 55 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index a5cb1528..c24d0b8e4deb 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -2113,9 +2113,6 @@ enum ieee80211_hw_flags {
  * @n_cipher_schemes: a size of an array of cipher schemes definitions.
  * @cipher_schemes: a pointer to an array of cipher scheme definitions
  * supported by HW.
- *
- * @txq_ac_max_pending: maximum number of frames per AC pending in all txq
- * entries for a vif.
  */
 struct ieee80211_hw {
struct ieee80211_conf conf;
@@ -2145,7 +2142,6 @@ struct ieee80211_hw {
u8 uapsd_max_sp_len;
u8 n_cipher_schemes;
const struct ieee80211_cipher_scheme *cipher_schemes;
-   int txq_ac_max_pending;
 };
 
 static inline bool _ieee80211_hw_check(struct ieee80211_hw *hw,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c6830fbe7d68..b2570aa66d33 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -855,7 +855,6 @@ struct ieee80211_sub_if_data {
bool control_port_no_encrypt;
int encrypt_headroom;
 
-   atomic_t txqs_len[IEEE80211_NUM_ACS];
struct ieee80211_tx_queue_params tx_conf[IEEE80211_NUM_ACS];
struct mac80211_qos_map __rcu *qos_map;
 
@@ -1118,6 +1117,7 @@ struct ieee80211_local {
fif_probe_req;
int probe_req_reg;
unsigned int filter_flags; /* FIF_* */
+   atomic_t num_tx_queued;
 
bool wiphy_ciphers_allocated;
 
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 453b4e741780..67c9b1e565ad 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -976,13 +976,13 @@ static void ieee80211_do_stop(struct 
ieee80211_sub_if_data *sdata,
 
if (sdata->vif.txq) {
struct txq_info *txqi = to_txq_info(sdata->vif.txq);
+   int n = skb_queue_len(>queue);
 
spin_lock_bh(>queue.lock);
ieee80211_purge_tx_queue(>hw, >queue);
+   atomic_sub(n, >num_tx_queued);
txqi->byte_cnt = 0;
spin_unlock_bh(>queue.lock);
-
-   atomic_set(>txqs_len[txqi->txq.ac], 0);
}
 
if (local->open_count == 0)
@@ -1198,6 +1198,12 @@ static void ieee80211_if_setup(struct net_device *dev)
dev->destructor = ieee80211_if_free;
 }
 
+static void ieee80211_if_setup_no_queue(struct net_device *dev)
+{
+   ieee80211_if_setup(dev);
+   dev->priv_flags |= IFF_NO_QUEUE;
+}
+
 static void ieee80211_iface_work(struct work_struct *work)
 {
struct ieee80211_sub_if_data *sdata =
@@ -1707,6 +1713,7 @@ int ieee80211_if_add(struct ieee80211_local *local, const 
char *name,
struct net_device *ndev = NULL;
struct ieee80211_sub_if_data *sdata = NULL;
struct txq_info *txqi;
+   void (*if_setup)(struct net_device *dev);
int ret, i;
int txqs = 1;
 
@@ -1734,12 +1741,17 @@ int ieee80211_if_add(struct ieee80211_local *local, 
const char *name,
txq_size += sizeof(struct txq_info) +
local->hw.txq_data_size;
 
+   if (local->ops->wake_tx_queue)
+   if_setup = ieee80211_if_setup_no_queue;
+   else
+   if_setup = ieee80211_if_setup;
+
if (local->hw.queues >= IEEE80211_NUM_ACS)
txqs = IEEE80211_NUM_ACS;
 
ndev = alloc_netdev_mqs(size + txq_size,
name,