Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Michal Kazior]

On 1 February 2017 at 09:33, Pali Rohár  wrote:
> On Tuesday 31 January 2017 07:59:18 Tony Lindgren wrote:
>> * Kalle Valo  [170130 22:36]:
[...]
>> > * before distro updates linux-firmware create yours own deb/rpm/whatever
>> >   package "wl1251-firmware" which installs your flavor of nvs file (or
>> >   the user fallback helper if more dynamic functionality is preferred)
>>
>> And that won't work when using the same file system on other machines.
>>
>> Think NFSroot for example. At least I'm using the same NFSroot across
>> about 15 different machines including one n900 macro board with smc91x
>> Ethernet.
>
> Exactly problem which we already discussed in previous emails. You
> cannot serve one file (loaded by direct request_firmware) when your
> rootfs is readonly, e.g. comes via NFS shared for more devices...

You can extract the nvs blob, put it in tmpfs and bind-mount (or
symlink) it to /lib/firmware/ via modprobe install hook (or init
scripts).


Michał

Re: [PATCH 3/5] ath10k: Remove unused wmi_p2p_noa_descriptor 'noa' in wmi-tlv

[Michal Kazior]

On 24 November 2016 at 09:01, Kirtika Ruchandani
<kirtika.ruchand...@gmail.com> wrote:
> Commit ca996ec56608 (ath10k: implement wmi-tlv backend)
> introduced ath10k_wmi_tlv_op_gen_vdev_start() where
> 'struct wmi_p2p_noa_descriptor *noa' is defined and set but not used.
> Compiling with W=1 gives the following warning, fix it.
> drivers/net/wireless/ath/ath10k/wmi-tlv.c: In function 
> ‘ath10k_wmi_tlv_op_gen_vdev_start’:
> drivers/net/wireless/ath/ath10k/wmi-tlv.c:1647:33: warning: variable ‘noa’ 
> set but not used [-Wunused-but-set-variable]
>
> Fixes: ca996ec56608 ("ath10k: implement wmi-tlv backend")
> Cc: Michal Kazior <michal.kaz...@tieto.com>
> Cc: Kalle Valo <kv...@qca.qualcomm.com>
> Signed-off-by: Kirtika Ruchandani <kirt...@chromium.org>
> ---
>  drivers/net/wireless/ath/ath10k/wmi-tlv.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c 
> b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> index e64f593..0e4bd29 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> @@ -1644,7 +1644,6 @@ ath10k_wmi_tlv_op_gen_vdev_start(struct ath10k *ar,
>  {
> struct wmi_tlv_vdev_start_cmd *cmd;
> struct wmi_channel *ch;
> -   struct wmi_p2p_noa_descriptor *noa;
> struct wmi_tlv *tlv;
> struct sk_buff *skb;
> size_t len;
> @@ -1702,7 +1701,6 @@ ath10k_wmi_tlv_op_gen_vdev_start(struct ath10k *ar,
> tlv = ptr;
> tlv->tag = __cpu_to_le16(WMI_TLV_TAG_ARRAY_STRUCT);
> tlv->len = 0;
> -   noa = (void *)tlv->value;
>
> /* Note: This is a nested TLV containing:
>  * [wmi_tlv][wmi_p2p_noa_descriptor][wmi_tlv]..

I would rather keep this one as it serves as documentation. Would
"(void) noa;" be enough satisfy the compiler?


Michał

Re: wl1251 & mac address & calibration data

[Michal Kazior]

On 22 November 2016 at 16:31, Pali Rohár <pali.ro...@gmail.com> wrote:
> On Tuesday 22 November 2016 16:22:57 Michal Kazior wrote:
>> On 21 November 2016 at 16:51, Pali Rohár <pali.ro...@gmail.com> wrote:
>> > On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
>> >> Hi! I will open discussion about mac address and calibration data for
>> >> wl1251 wireless chip again...
>> >>
>> >> Problem: Mac address & calibration data for wl1251 chip on Nokia N900
>> >> are stored on second nand partition (mtd1) in special proprietary format
>> >> which is used only for Nokia N900 (probably on N8x0 and N9 too).
>> >> Wireless driver wl1251.ko cannot work without mac address and
>> >> calibration data.
>>
>> Same problem applies to some ath9k/ath10k supported routers. Some even
>> carry mac address as implicit offset from ethernet mac address. As far
>> as I understand OpenWRT cooks cal blobs on first boot prior to loading
>> modules.
>
> So... wl1251 on Nokia N900 is not alone and this problem is there for
> more drivers and devices. Which means we should come up with some
> generic solution.

This isn't particularly a problem for ath9k/ath10k.

Let me give you more background on ath10k.

ath10k devices can come with caldata and macaddr stored in their
OTP/EEPROM. In that case a generic "template" board file is used.
Userspace doesn't need to do anything special.

Some vendors however decide to use flash partition to store caldata.
In that case ath10k expects userspace to prepare cal-$bus-$devname.bin
files, each for a different radio (you can have multiple radios on a
system).

Now translating this for wl1251 I would expect it should also use
something like wl1251-nvs-sdio-0x0001.bin for devices like N900 that
have caldata on flash partition (instead of the generic
wl1251-nvs.bin). I'm not sure if wl1251-nvs.bin is something
comparable to (the generic) board.bin ath10k has though. Maybe the
entire idea behind wl1251-nvs.bin is flawed as it's supposed to be
device specific and is oblivious to possibility of having multiple
wl1251 radios on one system (probably sane assumption from practical
standpoint but still).


>> >> Absence of mac address cause that driver generates random mac address at
>> >> every kernel boot which has couple of problems (unstable identifier of
>> >> wireless device due to udev permanent storage rules; unpredictable
>> >> behaviour for dhcp mac address assignment, mac address filtering, ...).
>> >>
>> >> Currently there is no way to set (permanent) mac address for network
>> >> interface from userspace. And it does not make sense to implement in
>> >> linux kernel large parser for proprietary format of second nand
>> >> partition where is mac address stored only for one device -- Nokia N900.
>> >>
>> >> Driver wl1251.ko loads calibration data via request_firmware() for file
>> >> wl1251-nvs.bin. There are some "example" calibration file in linux-
>> >> firmware repository, but it is not suitable for normal usage as real
>> >> calibration data are per-device specific.
>>
>> You could hook up a script that cooks up the cal/mac file via
>> modprobe's install hook, no?
>
> Via modprobe hook I can either pass custom module parameter or call any
> other system (shell) commands.
>
> As wl1251.ko does not accept mac_address as module parameter, such
> modprobe hook does not help -- as there is absolutely no way from
> userspace to set or change (permanent) mac address.

Quoting modprobe.d manual:

>   install modulename command...
>   This command instructs modprobe to run your
>   command instead of inserting the module in the
>   kernel as normal. The command can be any shell
>   command: this allows you to do any kind of
>   complex processing you might wish. [...]

You can hook up a script that cooks up wl1251-nvs.bin (caldata,
macaddr) and then insmod the actual wl1251.ko module. Or you can just
cook up the nvs on first device boot and store it in /lib/firmware
(possibly overwriting the "generic" wl1251 from linux-firmware).


Michal

Re: wl1251 & mac address & calibration data

[Michal Kazior]

On 21 November 2016 at 16:51, Pali Rohár  wrote:
> On Friday 11 November 2016 18:20:50 Pali Rohár wrote:
>> Hi! I will open discussion about mac address and calibration data for
>> wl1251 wireless chip again...
>>
>> Problem: Mac address & calibration data for wl1251 chip on Nokia N900
>> are stored on second nand partition (mtd1) in special proprietary format
>> which is used only for Nokia N900 (probably on N8x0 and N9 too).
>> Wireless driver wl1251.ko cannot work without mac address and
>> calibration data.

Same problem applies to some ath9k/ath10k supported routers. Some even
carry mac address as implicit offset from ethernet mac address. As far
as I understand OpenWRT cooks cal blobs on first boot prior to loading
modules.


>> Absence of mac address cause that driver generates random mac address at
>> every kernel boot which has couple of problems (unstable identifier of
>> wireless device due to udev permanent storage rules; unpredictable
>> behaviour for dhcp mac address assignment, mac address filtering, ...).
>>
>> Currently there is no way to set (permanent) mac address for network
>> interface from userspace. And it does not make sense to implement in
>> linux kernel large parser for proprietary format of second nand
>> partition where is mac address stored only for one device -- Nokia N900.
>>
>> Driver wl1251.ko loads calibration data via request_firmware() for file
>> wl1251-nvs.bin. There are some "example" calibration file in linux-
>> firmware repository, but it is not suitable for normal usage as real
>> calibration data are per-device specific.

You could hook up a script that cooks up the cal/mac file via
modprobe's install hook, no?


Michał

[PATCH] fq: split out backlog update logic

[Michal Kazior]

mac80211 (which will be the first user of the
fq.h) recently started to support software A-MSDU
aggregation. It glues skbuffs together into a
single one so the backlog accounting needs to be
more fine-grained.

To avoid backlog sorting logic duplication split
it up for re-use.

Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
---

While preparing a re-spin of fq_codel for
mac80211-next/master I've noticed that it's
current head has the new software A-MSDU
aggregation.

I'm aware I just recently submitted fq.h.

Sorry for the noise!


 include/net/fq_impl.h | 30 +++---
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/include/net/fq_impl.h b/include/net/fq_impl.h
index 02eab7c51adb..163f3ed0f05a 100644
--- a/include/net/fq_impl.h
+++ b/include/net/fq_impl.h
@@ -120,6 +120,24 @@ static struct fq_flow *fq_flow_classify(struct fq *fq,
return flow;
 }
 
+static void fq_recalc_backlog(struct fq *fq,
+ struct fq_tin *tin,
+ struct fq_flow *flow)
+{
+   struct fq_flow *i;
+
+   if (list_empty(>backlogchain))
+   list_add_tail(>backlogchain, >backlogs);
+
+   i = flow;
+   list_for_each_entry_continue_reverse(i, >backlogs,
+backlogchain)
+   if (i->backlog > flow->backlog)
+   break;
+
+   list_move(>backlogchain, >backlogchain);
+}
+
 static void fq_tin_enqueue(struct fq *fq,
   struct fq_tin *tin,
   struct sk_buff *skb,
@@ -127,7 +145,6 @@ static void fq_tin_enqueue(struct fq *fq,
   fq_flow_get_default_t get_default_func)
 {
struct fq_flow *flow;
-   struct fq_flow *i;
 
lockdep_assert_held(>lock);
 
@@ -139,16 +156,7 @@ static void fq_tin_enqueue(struct fq *fq,
tin->backlog_packets++;
fq->backlog++;
 
-   if (list_empty(>backlogchain))
-   list_add_tail(>backlogchain, >backlogs);
-
-   i = flow;
-   list_for_each_entry_continue_reverse(i, >backlogs,
-backlogchain)
-   if (i->backlog > flow->backlog)
-   break;
-
-   list_move(>backlogchain, >backlogchain);
+   fq_recalc_backlog(fq, tin, flow);
 
if (list_empty(>flowchain)) {
flow->deficit = fq->quantum;
-- 
2.1.4

Re: codel: split into multiple files

[Michal Kazior]

On 26 April 2016 at 08:43, Sedat Dilek <sedat.di...@gmail.com> wrote:
> On 4/26/16, Michal Kazior <michal.kaz...@tieto.com> wrote:
>> On 26 April 2016 at 08:09, Sedat Dilek <sedat.di...@gmail.com> wrote:
>>> Hi,
>>>
>>> I had a very quick view on net-next.git#master (up to commit
>>> fab7b629a82da1b59620470d13152aff975239f6).
>>>
>>> Commit in [1] aka "codel: split into multiple files" removed codel.h
>>> but [2] and [3] have relicts to it.
>>> Forgot to remove?
>>
>> codel.h was not removed. diffstat for codel.h is all red which I
>> presume is why you thought of it as removed, see:
>>
>> http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/include/net/codel.h?id=d068ca2ae2e614b9a418fb3b5f1fd4cf996ff032
>>
>
> [ CC Jens ]
>
> OK.
> So what are the plans in the future?
> Keep a "generic" codel.h (compatibility reasons?) for net or is it your split?

I'm interested in re-using codel in mac80211 for wireless. cfg80211
drivers may want to do that as well later. Even vendor drivers could
start to use it (I can dream :).

I plan to re-spin my patches soonish re-based on the new codel.h/fq.h
approach. There's quite a few spins already[1].


> AFAICS I have seen a codel-implementation in block.git#wb-buf-throttle.
> Does it make sense to have a more "super-generic" codel.h for re-use
> (not only for net and block)?
> Just a thought.

Oh, I'm not really familiar with block and problems around it but it
sounds reasonable and interesting. It doesn't look like it blatantly
copies codel though (I did that in my initial mac80211 patches with
some adjustments, you can check that in the link[1] which you can
lookup via my patchset's cover letter[2]; I've based off of codel5[3]
back then).

[1]: https://www.spinics.net/lists/linux-wireless/msg148714.html
[2]: https://www.spinics.net/lists/netdev/msg374308.html
[3]: https://github.com/dtaht/bcake/blob/master/codel5.h


Michał

Re: codel: split into multiple files

[Michal Kazior]

On 26 April 2016 at 08:09, Sedat Dilek  wrote:
> Hi,
>
> I had a very quick view on net-next.git#master (up to commit
> fab7b629a82da1b59620470d13152aff975239f6).
>
> Commit in [1] aka "codel: split into multiple files" removed codel.h
> but [2] and [3] have relicts to it.
> Forgot to remove?

codel.h was not removed. diffstat for codel.h is all red which I
presume is why you thought of it as removed, see:

http://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/tree/include/net/codel.h?id=d068ca2ae2e614b9a418fb3b5f1fd4cf996ff032


Michał

[PATCH] fq: add fair queuing framework

[Michal Kazior]

This works on the same implementation principle as
codel*.h, i.e. there's a generic header with
structures and macros and a implementation header
carrying function definitions to include in given,
e.g. driver or module.

The fairness logic comes from
net/sched/sch_fq_codel.c but is generalized so it
is more flexible and easier to re-use.

Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
---
This will be used by mac80211.

For more background please see:

  https://www.spinics.net/lists/linux-wireless/msg149976.html


 include/net/fq.h  |  95 ++
 include/net/fq_impl.h | 269 ++
 2 files changed, 364 insertions(+)
 create mode 100644 include/net/fq.h
 create mode 100644 include/net/fq_impl.h

diff --git a/include/net/fq.h b/include/net/fq.h
new file mode 100644
index ..268b49049c37
--- /dev/null
+++ b/include/net/fq.h
@@ -0,0 +1,95 @@
+/*
+ * Copyright (c) 2016 Qualcomm Atheros, Inc
+ *
+ * GPL v2
+ *
+ * Based on net/sched/sch_fq_codel.c
+ */
+#ifndef __NET_SCHED_FQ_H
+#define __NET_SCHED_FQ_H
+
+struct fq_tin;
+
+/**
+ * struct fq_flow - per traffic flow queue
+ *
+ * @tin: owner of this flow. Used to manage collisions, i.e. when a packet
+ * hashes to an index which points to a flow that is already owned by a
+ * different tin the packet is destined to. In such case the implementer
+ * must provide a fallback flow
+ * @flowchain: can be linked to fq_tin's new_flows or old_flows. Used for DRR++
+ * (deficit round robin) based round robin queuing similar to the one
+ * found in net/sched/sch_fq_codel.c
+ * @backlogchain: can be linked to other fq_flow and fq. Used to keep track of
+ * fat flows and efficient head-dropping if packet limit is reached
+ * @queue: sk_buff queue to hold packets
+ * @backlog: number of bytes pending in the queue. The number of packets can be
+ * found in @queue.qlen
+ * @deficit: used for DRR++
+ */
+struct fq_flow {
+   struct fq_tin *tin;
+   struct list_head flowchain;
+   struct list_head backlogchain;
+   struct sk_buff_head queue;
+   u32 backlog;
+   int deficit;
+};
+
+/**
+ * struct fq_tin - a logical container of fq_flows
+ *
+ * Used to group fq_flows into a logical aggregate. DRR++ scheme is used to
+ * pull interleaved packets out of the associated flows.
+ *
+ * @new_flows: linked list of fq_flow
+ * @old_flows: linked list of fq_flow
+ */
+struct fq_tin {
+   struct list_head new_flows;
+   struct list_head old_flows;
+   u32 backlog_bytes;
+   u32 backlog_packets;
+   u32 overlimit;
+   u32 collisions;
+   u32 flows;
+   u32 tx_bytes;
+   u32 tx_packets;
+};
+
+/**
+ * struct fq - main container for fair queuing purposes
+ *
+ * @backlogs: linked to fq_flows. Used to maintain fat flows for efficient
+ * head-dropping when @backlog reaches @limit
+ * @limit: max number of packets that can be queued across all flows
+ * @backlog: number of packets queued across all flows
+ */
+struct fq {
+   struct fq_flow *flows;
+   struct list_head backlogs;
+   spinlock_t lock;
+   u32 flows_cnt;
+   u32 perturbation;
+   u32 limit;
+   u32 quantum;
+   u32 backlog;
+   u32 overlimit;
+   u32 collisions;
+};
+
+typedef struct sk_buff *fq_tin_dequeue_t(struct fq *,
+struct fq_tin *,
+struct fq_flow *flow);
+
+typedef void fq_skb_free_t(struct fq *,
+  struct fq_tin *,
+  struct fq_flow *,
+  struct sk_buff *);
+
+typedef struct fq_flow *fq_flow_get_default_t(struct fq *,
+ struct fq_tin *,
+ int idx,
+ struct sk_buff *);
+
+#endif
diff --git a/include/net/fq_impl.h b/include/net/fq_impl.h
new file mode 100644
index ..02eab7c51adb
--- /dev/null
+++ b/include/net/fq_impl.h
@@ -0,0 +1,269 @@
+/*
+ * Copyright (c) 2016 Qualcomm Atheros, Inc
+ *
+ * GPL v2
+ *
+ * Based on net/sched/sch_fq_codel.c
+ */
+#ifndef __NET_SCHED_FQ_IMPL_H
+#define __NET_SCHED_FQ_IMPL_H
+
+#include 
+
+/* functions that are embedded into includer */
+
+static struct sk_buff *fq_flow_dequeue(struct fq *fq,
+  struct fq_flow *flow)
+{
+   struct fq_tin *tin = flow->tin;
+   struct fq_flow *i;
+   struct sk_buff *skb;
+
+   lockdep_assert_held(>lock);
+
+   skb = __skb_dequeue(>queue);
+   if (!skb)
+   return NULL;
+
+   tin->backlog_bytes -= skb->len;
+   tin->backlog_packets--;
+   flow->backlog -= skb->len;
+   fq->backlog--;
+
+   if (flow->backlog == 0) {
+   list_del_init(>backlogchain);
+   } else {
+   i = flow;
+
+   li

[PATCH 1/2] codel: generalize the implementation

[Michal Kazior]

This strips out qdisc specific bits from the code
and makes it slightly more reusable. Codel will be
used by wireless/mac80211 in the future.

Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
---
 include/net/codel.h  | 64 +---
 net/sched/sch_codel.c| 20 ---
 net/sched/sch_fq_codel.c | 19 +++---
 3 files changed, 71 insertions(+), 32 deletions(-)

diff --git a/include/net/codel.h b/include/net/codel.h
index d168aca115cc..06ac687b4909 100644
--- a/include/net/codel.h
+++ b/include/net/codel.h
@@ -176,12 +176,10 @@ struct codel_stats {
 
 #define CODEL_DISABLED_THRESHOLD INT_MAX
 
-static void codel_params_init(struct codel_params *params,
- const struct Qdisc *sch)
+static void codel_params_init(struct codel_params *params)
 {
params->interval = MS2TIME(100);
params->target = MS2TIME(5);
-   params->mtu = psched_mtu(qdisc_dev(sch));
params->ce_threshold = CODEL_DISABLED_THRESHOLD;
params->ecn = false;
 }
@@ -226,28 +224,38 @@ static codel_time_t codel_control_law(codel_time_t t,
return t + reciprocal_scale(interval, rec_inv_sqrt << 
REC_INV_SQRT_SHIFT);
 }
 
+typedef u32 (*codel_skb_len_t)(const struct sk_buff *skb);
+typedef codel_time_t (*codel_skb_time_t)(const struct sk_buff *skb);
+typedef void (*codel_skb_drop_t)(struct sk_buff *skb, void *ctx);
+typedef struct sk_buff * (*codel_skb_dequeue_t)(struct codel_vars *vars,
+   void *ctx);
+
 static bool codel_should_drop(const struct sk_buff *skb,
- struct Qdisc *sch,
+ void *ctx,
  struct codel_vars *vars,
  struct codel_params *params,
  struct codel_stats *stats,
+ codel_skb_len_t skb_len_func,
+ codel_skb_time_t skb_time_func,
+ u32 *backlog,
  codel_time_t now)
 {
bool ok_to_drop;
+   u32 skb_len;
 
if (!skb) {
vars->first_above_time = 0;
return false;
}
 
-   vars->ldelay = now - codel_get_enqueue_time(skb);
-   sch->qstats.backlog -= qdisc_pkt_len(skb);
+   skb_len = skb_len_func(skb);
+   vars->ldelay = now - skb_time_func(skb);
 
-   if (unlikely(qdisc_pkt_len(skb) > stats->maxpacket))
-   stats->maxpacket = qdisc_pkt_len(skb);
+   if (unlikely(skb_len > stats->maxpacket))
+   stats->maxpacket = skb_len;
 
if (codel_time_before(vars->ldelay, params->target) ||
-   sch->qstats.backlog <= params->mtu) {
+   *backlog <= params->mtu) {
/* went below - stay below for at least interval */
vars->first_above_time = 0;
return false;
@@ -264,16 +272,17 @@ static bool codel_should_drop(const struct sk_buff *skb,
return ok_to_drop;
 }
 
-typedef struct sk_buff * (*codel_skb_dequeue_t)(struct codel_vars *vars,
-   struct Qdisc *sch);
-
-static struct sk_buff *codel_dequeue(struct Qdisc *sch,
+static struct sk_buff *codel_dequeue(void *ctx,
+u32 *backlog,
 struct codel_params *params,
 struct codel_vars *vars,
 struct codel_stats *stats,
+codel_skb_len_t skb_len_func,
+codel_skb_time_t skb_time_func,
+codel_skb_drop_t drop_func,
 codel_skb_dequeue_t dequeue_func)
 {
-   struct sk_buff *skb = dequeue_func(vars, sch);
+   struct sk_buff *skb = dequeue_func(vars, ctx);
codel_time_t now;
bool drop;
 
@@ -282,7 +291,8 @@ static struct sk_buff *codel_dequeue(struct Qdisc *sch,
return skb;
}
now = codel_get_time();
-   drop = codel_should_drop(skb, sch, vars, params, stats, now);
+   drop = codel_should_drop(skb, ctx, vars, params, stats,
+skb_len_func, skb_time_func, backlog, now);
if (vars->dropping) {
if (!drop) {
/* sojourn time below target - leave dropping state */
@@ -310,12 +320,15 @@ static struct sk_buff *codel_dequeue(struct Qdisc *sch,
  
vars->rec_inv_sqrt);
goto end;
}
-   stats->drop_len += qdisc_pkt_len(skb);
-   qdisc_drop(skb, sch);
+ 

[PATCH 0/2] codel: make it reuseable beyond qdiscs

[Michal Kazior]

Hi,

There's an ongoing effort in fixing wireless
bufferbloat. As part of that fq_codel is being
ported into mac80211. To prevent code duplication
codel.h needs to be slightly modified before it
can be used in mac80211 (or other drivers FWIW).

For more background please see:

  https://www.spinics.net/lists/linux-wireless/msg149976.html


Michal Kazior (2):
  codel: generalize the implementation
  codel: split into multiple files

 include/net/codel.h   | 217 +--
 include/net/codel_impl.h  | 255 ++
 include/net/codel_qdisc.h |  73 +
 net/sched/sch_codel.c |  22 +++-
 net/sched/sch_fq_codel.c  |  21 +++-
 5 files changed, 368 insertions(+), 220 deletions(-)
 create mode 100644 include/net/codel_impl.h
 create mode 100644 include/net/codel_qdisc.h

-- 
2.1.4

[PATCH 2/2] codel: split into multiple files

[Michal Kazior]

It was impossible to include codel.h for the
purpose of having access to codel_params or
codel_vars structure definitions and using them
for embedding in other more complex structures.

This splits allows codel.h itself to be treated
like any other header file while codel_qdisc.h and
codel_impl.h contain function definitions with
logic that was previously in codel.h.

This copies over copyrights and doesn't involve
code changes other than adding a few additional
include directives to net/sched/sch*codel.c.

Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
---
 include/net/codel.h   | 223 
 include/net/codel_impl.h  | 255 ++
 include/net/codel_qdisc.h |  73 +
 net/sched/sch_codel.c |   2 +
 net/sched/sch_fq_codel.c  |   2 +
 5 files changed, 332 insertions(+), 223 deletions(-)
 create mode 100644 include/net/codel_impl.h
 create mode 100644 include/net/codel_qdisc.h

diff --git a/include/net/codel.h b/include/net/codel.h
index 06ac687b4909..a6e428f80135 100644
--- a/include/net/codel.h
+++ b/include/net/codel.h
@@ -87,27 +87,6 @@ static inline codel_time_t codel_get_time(void)
 ((s32)((a) - (b)) >= 0))
 #define codel_time_before_eq(a, b) codel_time_after_eq(b, a)
 
-/* Qdiscs using codel plugin must use codel_skb_cb in their own cb[] */
-struct codel_skb_cb {
-   codel_time_t enqueue_time;
-};
-
-static struct codel_skb_cb *get_codel_cb(const struct sk_buff *skb)
-{
-   qdisc_cb_private_validate(skb, sizeof(struct codel_skb_cb));
-   return (struct codel_skb_cb *)qdisc_skb_cb(skb)->data;
-}
-
-static codel_time_t codel_get_enqueue_time(const struct sk_buff *skb)
-{
-   return get_codel_cb(skb)->enqueue_time;
-}
-
-static void codel_set_enqueue_time(struct sk_buff *skb)
-{
-   get_codel_cb(skb)->enqueue_time = codel_get_time();
-}
-
 static inline u32 codel_time_to_us(codel_time_t val)
 {
u64 valns = ((u64)val << CODEL_SHIFT);
@@ -176,212 +155,10 @@ struct codel_stats {
 
 #define CODEL_DISABLED_THRESHOLD INT_MAX
 
-static void codel_params_init(struct codel_params *params)
-{
-   params->interval = MS2TIME(100);
-   params->target = MS2TIME(5);
-   params->ce_threshold = CODEL_DISABLED_THRESHOLD;
-   params->ecn = false;
-}
-
-static void codel_vars_init(struct codel_vars *vars)
-{
-   memset(vars, 0, sizeof(*vars));
-}
-
-static void codel_stats_init(struct codel_stats *stats)
-{
-   stats->maxpacket = 0;
-}
-
-/*
- * 
http://en.wikipedia.org/wiki/Methods_of_computing_square_roots#Iterative_methods_for_reciprocal_square_roots
- * new_invsqrt = (invsqrt / 2) * (3 - count * invsqrt^2)
- *
- * Here, invsqrt is a fixed point number (< 1.0), 32bit mantissa, aka Q0.32
- */
-static void codel_Newton_step(struct codel_vars *vars)
-{
-   u32 invsqrt = ((u32)vars->rec_inv_sqrt) << REC_INV_SQRT_SHIFT;
-   u32 invsqrt2 = ((u64)invsqrt * invsqrt) >> 32;
-   u64 val = (3LL << 32) - ((u64)vars->count * invsqrt2);
-
-   val >>= 2; /* avoid overflow in following multiply */
-   val = (val * invsqrt) >> (32 - 2 + 1);
-
-   vars->rec_inv_sqrt = val >> REC_INV_SQRT_SHIFT;
-}
-
-/*
- * CoDel control_law is t + interval/sqrt(count)
- * We maintain in rec_inv_sqrt the reciprocal value of sqrt(count) to avoid
- * both sqrt() and divide operation.
- */
-static codel_time_t codel_control_law(codel_time_t t,
- codel_time_t interval,
- u32 rec_inv_sqrt)
-{
-   return t + reciprocal_scale(interval, rec_inv_sqrt << 
REC_INV_SQRT_SHIFT);
-}
-
 typedef u32 (*codel_skb_len_t)(const struct sk_buff *skb);
 typedef codel_time_t (*codel_skb_time_t)(const struct sk_buff *skb);
 typedef void (*codel_skb_drop_t)(struct sk_buff *skb, void *ctx);
 typedef struct sk_buff * (*codel_skb_dequeue_t)(struct codel_vars *vars,
void *ctx);
 
-static bool codel_should_drop(const struct sk_buff *skb,
- void *ctx,
- struct codel_vars *vars,
- struct codel_params *params,
- struct codel_stats *stats,
- codel_skb_len_t skb_len_func,
- codel_skb_time_t skb_time_func,
- u32 *backlog,
- codel_time_t now)
-{
-   bool ok_to_drop;
-   u32 skb_len;
-
-   if (!skb) {
-   vars->first_above_time = 0;
-   return false;
-   }
-
-   skb_len = skb_len_func(skb);
-   vars->ldelay = now - skb_time_func(skb);
-
-   if (unlikely(skb_len > stats->maxpacket))
-   stats->maxpacket = skb_len;
-
-   if (codel_time_before(vars->ldelay, params->target) ||
-   *

Re: [PATCH] ipv6: allow bypassing cross-intf routing limits

[Michal Kazior]

On 17 April 2016 at 00:49, David Miller <da...@davemloft.net> wrote:
> From: Michal Kazior <michal.kaz...@tieto.com>
> Date: Thu, 14 Apr 2016 14:46:28 +0200
>
>> There are some use-cases to allow link-local
>> routing for bridging purposes.
>>
>> One of these is allowing transparent 802.11
>> bridging. Due to 802.11 framing limitations many
>> Access Points make it impossible to create bridges
>> on Client endpoints because they can't maintain
>> Destination/Source/Transmitter/Receiver address
>> distinction with only 3 addresses in frame header.
>>
>> The default behavior, i.e. link-local traffic
>> being non-routable, remains. The user has to
>> explicitly enable the bypass when defining a given
>> route.
>>
>> Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
>
> Sorry, whilst I realize your problem, I'm not going to add what is
> explicitly a violation of the way link-local addresses are meant to
> work and the very much intentional restrictions the RFCs place upon
> them (they MUST not be routed).
>
> I also didn't see any real discussions in response to your original
> proposals, not from even one person I know is knowledgable about ipv6
> and the implications your change would have, and that is extremely
> troubling.
>
> I tried to let your patches sit for several days in order to let that
> kind of discussion happen, but it didn't.

I totally understand. Thanks anyway.


> So, you'll need to find another way to achieve your goals.

Hmm.. I actually do have another idea in mind already.

I was wondering about about implementing a link device in a similar
fashion to macvlan, bridge, et al, i.e. the device would take an
interface (wlan interface in my case) as a slave and perform L2
address mangling (which should be enough to accommodate the addressing
deficiency in wireless client modes). The device would obviously need
to do some L3/L4 packet inspection and manipulation.

Would something like that be acceptable? Thoughts?


Michał

[PATCH] ipv6: allow bypassing cross-intf routing limits

[Michal Kazior]

There are some use-cases to allow link-local
routing for bridging purposes.

One of these is allowing transparent 802.11
bridging. Due to 802.11 framing limitations many
Access Points make it impossible to create bridges
on Client endpoints because they can't maintain
Destination/Source/Transmitter/Receiver address
distinction with only 3 addresses in frame header.

The default behavior, i.e. link-local traffic
being non-routable, remains. The user has to
explicitly enable the bypass when defining a given
route.

Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
---
For more background see:

  http://www.spinics.net/lists/netdev/msg371022.html
  https://lists.openwrt.org/pipermail/openwrt-devel/2016-April/040783.html

 include/uapi/linux/rtnetlink.h |  8 ++--
 net/ipv6/ip6_output.c  | 11 +--
 net/ipv6/route.c   |  4 
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index ca764b5da86d..a577eec0e56e 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -424,9 +424,13 @@ enum {
 #define RTAX_FEATURE_SACK  (1 << 1)
 #define RTAX_FEATURE_TIMESTAMP (1 << 2)
 #define RTAX_FEATURE_ALLFRAG   (1 << 3)
+#define RTAX_FEATURE_XFACE (1 << 4)
 
-#define RTAX_FEATURE_MASK  (RTAX_FEATURE_ECN | RTAX_FEATURE_SACK | \
-RTAX_FEATURE_TIMESTAMP | RTAX_FEATURE_ALLFRAG)
+#define RTAX_FEATURE_MASK  (RTAX_FEATURE_ECN | \
+RTAX_FEATURE_SACK | \
+RTAX_FEATURE_TIMESTAMP | \
+RTAX_FEATURE_ALLFRAG | \
+RTAX_FEATURE_XFACE)
 
 struct rta_session {
__u8proto;
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 9428345d3a07..9abb42acb6ad 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -283,6 +283,7 @@ static int ip6_forward_proxy_check(struct sk_buff *skb)
u8 nexthdr = hdr->nexthdr;
__be16 frag_off;
int offset;
+   int feat = dst_metric_raw(skb_dst(skb), RTAX_FEATURES);
 
if (ipv6_ext_hdr(nexthdr)) {
offset = ipv6_skip_exthdr(skb, sizeof(*hdr), , 
_off);
@@ -320,8 +321,11 @@ static int ip6_forward_proxy_check(struct sk_buff *skb)
 * The proxying router can't forward traffic sent to a link-local
 * address, so signal the sender and discard the packet. This
 * behavior is clarified by the MIPv6 specification.
+*
+* It's useful to allow an override for transparent traffic relay.
 */
-   if (ipv6_addr_type(>daddr) & IPV6_ADDR_LINKLOCAL) {
+   if ((ipv6_addr_type(>daddr) & IPV6_ADDR_LINKLOCAL) &&
+   !(feat & RTAX_FEATURE_XFACE)) {
dst_link_failure(skb);
return -1;
}
@@ -485,12 +489,15 @@ int ip6_forward(struct sk_buff *skb)
inet_putpeer(peer);
} else {
int addrtype = ipv6_addr_type(>saddr);
+   int feat = dst_metric_raw(dst, RTAX_FEATURES);
 
/* This check is security critical. */
if (addrtype == IPV6_ADDR_ANY ||
addrtype & (IPV6_ADDR_MULTICAST | IPV6_ADDR_LOOPBACK))
goto error;
-   if (addrtype & IPV6_ADDR_LINKLOCAL) {
+
+   if ((addrtype & IPV6_ADDR_LINKLOCAL) &&
+   !(feat & RTAX_FEATURE_XFACE)) {
icmpv6_send(skb, ICMPV6_DEST_UNREACH,
ICMPV6_NOT_NEIGHBOUR, 0);
goto error;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ed446639219c..560c99853907 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -629,8 +629,12 @@ static inline enum rt6_nud_state rt6_check_neigh(struct 
rt6_info *rt)
 static int rt6_score_route(struct rt6_info *rt, int oif,
   int strict)
 {
+   int feat = dst_metric_raw(>dst, RTAX_FEATURES);
int m;
 
+   if (feat & RTAX_FEATURE_XFACE)
+   strict &= ~RT6_LOOKUP_F_IFACE;
+
m = rt6_check_dev(rt, oif);
if (!m && (strict & RT6_LOOKUP_F_IFACE))
return RT6_NUD_FAIL_HARD;
-- 
2.1.4

[RFC] ipv6: allow bypassing cross-intf routing limits

[Michal Kazior]

There are some use-cases to allow link-local
routing for bridging purposes.

One of these is allowing transparent 802.11
bridging. Due to 802.11 framing limitations many
Access Points make it impossible to create bridges
on Client endpoints because they can't maintain
Destination/Source/Transmitter/Receiver address
distinction with only 3 addresses in frame header.

The default behavior, i.e. link-local traffic
being non-routable, remains. The user has to
explicitly enable the bypass when defining a given
route.

Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
---
For more background see:

  http://www.spinics.net/lists/netdev/msg371022.html



 include/uapi/linux/rtnetlink.h |  8 ++--
 net/ipv6/ip6_output.c  | 11 +--
 net/ipv6/route.c   |  4 
 3 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/rtnetlink.h b/include/uapi/linux/rtnetlink.h
index ca764b5da86d..a577eec0e56e 100644
--- a/include/uapi/linux/rtnetlink.h
+++ b/include/uapi/linux/rtnetlink.h
@@ -424,9 +424,13 @@ enum {
 #define RTAX_FEATURE_SACK  (1 << 1)
 #define RTAX_FEATURE_TIMESTAMP (1 << 2)
 #define RTAX_FEATURE_ALLFRAG   (1 << 3)
+#define RTAX_FEATURE_XFACE (1 << 4)
 
-#define RTAX_FEATURE_MASK  (RTAX_FEATURE_ECN | RTAX_FEATURE_SACK | \
-RTAX_FEATURE_TIMESTAMP | RTAX_FEATURE_ALLFRAG)
+#define RTAX_FEATURE_MASK  (RTAX_FEATURE_ECN | \
+RTAX_FEATURE_SACK | \
+RTAX_FEATURE_TIMESTAMP | \
+RTAX_FEATURE_ALLFRAG | \
+RTAX_FEATURE_XFACE)
 
 struct rta_session {
__u8proto;
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 9428345d3a07..9abb42acb6ad 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -283,6 +283,7 @@ static int ip6_forward_proxy_check(struct sk_buff *skb)
u8 nexthdr = hdr->nexthdr;
__be16 frag_off;
int offset;
+   int feat = dst_metric_raw(skb_dst(skb), RTAX_FEATURES);
 
if (ipv6_ext_hdr(nexthdr)) {
offset = ipv6_skip_exthdr(skb, sizeof(*hdr), , 
_off);
@@ -320,8 +321,11 @@ static int ip6_forward_proxy_check(struct sk_buff *skb)
 * The proxying router can't forward traffic sent to a link-local
 * address, so signal the sender and discard the packet. This
 * behavior is clarified by the MIPv6 specification.
+*
+* It's useful to allow an override for transparent traffic relay.
 */
-   if (ipv6_addr_type(>daddr) & IPV6_ADDR_LINKLOCAL) {
+   if ((ipv6_addr_type(>daddr) & IPV6_ADDR_LINKLOCAL) &&
+   !(feat & RTAX_FEATURE_XFACE)) {
dst_link_failure(skb);
return -1;
}
@@ -485,12 +489,15 @@ int ip6_forward(struct sk_buff *skb)
inet_putpeer(peer);
} else {
int addrtype = ipv6_addr_type(>saddr);
+   int feat = dst_metric_raw(dst, RTAX_FEATURES);
 
/* This check is security critical. */
if (addrtype == IPV6_ADDR_ANY ||
addrtype & (IPV6_ADDR_MULTICAST | IPV6_ADDR_LOOPBACK))
goto error;
-   if (addrtype & IPV6_ADDR_LINKLOCAL) {
+
+   if ((addrtype & IPV6_ADDR_LINKLOCAL) &&
+   !(feat & RTAX_FEATURE_XFACE)) {
icmpv6_send(skb, ICMPV6_DEST_UNREACH,
ICMPV6_NOT_NEIGHBOUR, 0);
goto error;
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ed446639219c..560c99853907 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -629,8 +629,12 @@ static inline enum rt6_nud_state rt6_check_neigh(struct 
rt6_info *rt)
 static int rt6_score_route(struct rt6_info *rt, int oif,
   int strict)
 {
+   int feat = dst_metric_raw(>dst, RTAX_FEATURES);
int m;
 
+   if (feat & RTAX_FEATURE_XFACE)
+   strict &= ~RT6_LOOKUP_F_IFACE;
+
m = rt6_check_dev(rt, oif);
if (!m && (strict & RT6_LOOKUP_F_IFACE))
return RT6_NUD_FAIL_HARD;
-- 
2.1.4

IPv6: routing/forwarding/masking link-local addresses

[Michal Kazior]

Hi,

The most commonly used framing/addressing in 802.11 is 3addr. This is
how most Access Points work and Clients follow suit.

However it is impossible to put a 3addr Client interface into a bridge
because it's not possible to properly distinguish RA/TA/DA/SA
addresses.

There is 4addr framing in 802.11 (often referred to as WDS or
Repeater) but it's a loose standard and various vendors implement it
differently. For this kind of framing to work both AP and Client must
support the same flavor of the framing. Many APs don't even allow the
user to enable WDS. This makes it troublesome to extend wireless
networks without replacing AP equipment (upgrading AP software isn't
always an option).

There is already a userspace tool which solves this problem for IPv4
called relayd [1]. It forwards ARP/DHCP packets (and replaces MAC
addresses accordingly) between non-bridged interfaces and adds
additional route entries to the firewall to, effectively, act as a
bridge transparently. via routing. Compared to ARP Proxy it doesn't
require IP address configuration on local interfaces.

I'm trying to add IPv6 support for relayd. The problem is I can't get
link-local addresses to route properly.

I can easily get things like 2000::/32 to route perfectly fine but
fe80:: just won't work. Upon inspection I've noticed:

; ip link add veth0 type veth peer name veth1
; ip link set veth0 up
; ip link set veth1 up
; ip route add fe80::1 dev veth0
; ip route add 2000::1 dev veth0
; ip route get iif veth1 fe80::1
fe80::1 from :: dev veth1  metric 0
cache  iif veth1
; ip route get iif veth1 2000::1
2000::1 from :: dev veth0  metric 0
cache  iif veth1
; ip link del veth0

If I remove default fe80::/64 routes I get:

; ip link del veth0
; ip link add veth0 type veth peer name veth1
; ip link set veth0 up
; ip link set veth1 up
; ip route del fe80::/64 dev veth0
; ip route del fe80::/64 dev veth1
; ip route add fe80::1 dev veth0
; ip route add 2000::1 dev veth0
; ip route get iif veth1 fe80::1
unreachable fe80::1 from :: dev lo  table unspec  proto kernel  metric
4294967295  error -101 iif veth1
; ip route get iif veth1 2000::1
2000::1 from :: dev veth0  metric 0
cache  iif veth1

The fe80:: route gets ignored completely in both cases.

My question is: can I make it work (a kernel knob I'm not aware of) or
does it require patching the kernel? Thoughts/ideas/hints?


Michał

[1]: http://git.openwrt.org/?p=project/relayd.git;a=summary

Re: [RFCv2 2/3] ath10k: report per-station tx/rate rates to mac80211

[Michal Kazior]

On 24 March 2016 at 13:23, Mohammed Shafi Shajakhan
<moham...@codeaurora.org> wrote:
> On Thu, Mar 24, 2016 at 08:49:12AM +0100, Michal Kazior wrote:
>> On 24 March 2016 at 08:19, Mohammed Shafi Shajakhan
>> <moham...@codeaurora.org> wrote:
>> > Hi Michal,
>> >
>> > On Wed, Mar 16, 2016 at 11:17:57AM +0100, Michal Kazior wrote:
>> >> The rate control is offloaded by firmware so it's
>> >> challanging to provide expected throughput value
>> >> for given station.
>> >>
>> >> This approach is naive as it reports last tx rate
>> >> used for given station as provided by firmware
>> >> stat event.
>> >>
>> >> This should be sufficient for airtime estimation
>> >> used for fq-codel-in-mac80211 tx scheduling
>> >> purposes now.
>> >>
>> >> This patch uses a very hacky way to get the stats.
>> >> This is sufficient for proof-of-concept but must
>> >> be cleaned up properly eventually.
>> >>
>> >> Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
>> >> ---
>> >>  drivers/net/wireless/ath/ath10k/core.h  |  5 +++
>> >>  drivers/net/wireless/ath/ath10k/debug.c | 61 
>> >> +
>> >>  drivers/net/wireless/ath/ath10k/mac.c   | 26 --
>> >>  drivers/net/wireless/ath/ath10k/wmi.h   |  2 +-
>> >>  4 files changed, 76 insertions(+), 18 deletions(-)
>> >>
>> >> diff --git a/drivers/net/wireless/ath/ath10k/core.h 
>> >> b/drivers/net/wireless/ath/ath10k/core.h
>> >> index 23ba03fb7a5f..3f76669d44cf 100644
>> >> --- a/drivers/net/wireless/ath/ath10k/core.h
>> >> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> >> @@ -331,6 +331,9 @@ struct ath10k_sta {
>> >>   /* protected by conf_mutex */
>> >>   bool aggr_mode;
>> >>   u64 rx_duration;
>> >> +
>> >> + u32 tx_rate_kbps;
>> >> + u32 rx_rate_kbps;
>> >>  #endif
>> >>  };
>> >>
>> >> @@ -372,6 +375,8 @@ struct ath10k_vif {
>> >>   s8 def_wep_key_idx;
>> >>
>> >>   u16 tx_seq_no;
>> >> + u32 tx_rate_kbps;
>> >> + u32 rx_rate_kbps;
>> >>
>> >>   union {
>> >>   struct {
>> >> diff --git a/drivers/net/wireless/ath/ath10k/debug.c 
>> >> b/drivers/net/wireless/ath/ath10k/debug.c
>> >> index 076d29b53ddf..cc7ebf04ae00 100644
>> >> --- a/drivers/net/wireless/ath/ath10k/debug.c
>> >> +++ b/drivers/net/wireless/ath/ath10k/debug.c
>> >> @@ -316,6 +316,58 @@ static void ath10k_debug_fw_stats_reset(struct 
>> >> ath10k *ar)
>> >>   spin_unlock_bh(>data_lock);
>> >>  }
>> >>
>> >> +static void ath10k_mac_update_txrx_rate_iter(void *data,
>> >> +  u8 *mac,
>> >> +  struct ieee80211_vif *vif)
>> >> +{
>> >> + struct ath10k_fw_stats_peer *peer = data;
>> >> + struct ath10k_vif *arvif;
>> >> +
>> >> + if (memcmp(vif->addr, peer->peer_macaddr, ETH_ALEN))
>> >> + return;
>> >> +
>> >> + arvif = (void *)vif->drv_priv;
>> >> + arvif->tx_rate_kbps = peer->peer_tx_rate;
>> >> + arvif->rx_rate_kbps = peer->peer_rx_rate;
>> >> +}
>> >> +
>> >> +static void ath10k_mac_update_txrx_rate(struct ath10k *ar,
>> >> + struct ath10k_fw_stats *stats)
>> >> +{
>> >> + struct ieee80211_hw *hw = ar->hw;
>> >> + struct ath10k_fw_stats_peer *peer;
>> >> + struct ath10k_sta *arsta;
>> >> + struct ieee80211_sta *sta;
>> >> + const u8 *localaddr = NULL;
>> >> +
>> >> + rcu_read_lock();
>> >> +
>> >> + list_for_each_entry(peer, >peers, list) {
>> >> + /* This doesn't account for multiple STA connected on 
>> >> different
>> >> +  * vifs. Unfortunately there's no way to derive that from 
>> >> the available
>> >> +  * information.
>> >> +  */
>> >> + sta = ieee80211_find_sta_by_ifaddr(hw,
>> >>

Re: [RFCv2 2/3] ath10k: report per-station tx/rate rates to mac80211

[Michal Kazior]

On 24 March 2016 at 08:19, Mohammed Shafi Shajakhan
<moham...@codeaurora.org> wrote:
> Hi Michal,
>
> On Wed, Mar 16, 2016 at 11:17:57AM +0100, Michal Kazior wrote:
>> The rate control is offloaded by firmware so it's
>> challanging to provide expected throughput value
>> for given station.
>>
>> This approach is naive as it reports last tx rate
>> used for given station as provided by firmware
>> stat event.
>>
>> This should be sufficient for airtime estimation
>> used for fq-codel-in-mac80211 tx scheduling
>> purposes now.
>>
>> This patch uses a very hacky way to get the stats.
>> This is sufficient for proof-of-concept but must
>> be cleaned up properly eventually.
>>
>> Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
>> ---
>>  drivers/net/wireless/ath/ath10k/core.h  |  5 +++
>>  drivers/net/wireless/ath/ath10k/debug.c | 61 
>> +
>>  drivers/net/wireless/ath/ath10k/mac.c   | 26 --
>>  drivers/net/wireless/ath/ath10k/wmi.h   |  2 +-
>>  4 files changed, 76 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/core.h 
>> b/drivers/net/wireless/ath/ath10k/core.h
>> index 23ba03fb7a5f..3f76669d44cf 100644
>> --- a/drivers/net/wireless/ath/ath10k/core.h
>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> @@ -331,6 +331,9 @@ struct ath10k_sta {
>>   /* protected by conf_mutex */
>>   bool aggr_mode;
>>   u64 rx_duration;
>> +
>> + u32 tx_rate_kbps;
>> + u32 rx_rate_kbps;
>>  #endif
>>  };
>>
>> @@ -372,6 +375,8 @@ struct ath10k_vif {
>>   s8 def_wep_key_idx;
>>
>>   u16 tx_seq_no;
>> + u32 tx_rate_kbps;
>> + u32 rx_rate_kbps;
>>
>>   union {
>>   struct {
>> diff --git a/drivers/net/wireless/ath/ath10k/debug.c 
>> b/drivers/net/wireless/ath/ath10k/debug.c
>> index 076d29b53ddf..cc7ebf04ae00 100644
>> --- a/drivers/net/wireless/ath/ath10k/debug.c
>> +++ b/drivers/net/wireless/ath/ath10k/debug.c
>> @@ -316,6 +316,58 @@ static void ath10k_debug_fw_stats_reset(struct ath10k 
>> *ar)
>>   spin_unlock_bh(>data_lock);
>>  }
>>
>> +static void ath10k_mac_update_txrx_rate_iter(void *data,
>> +  u8 *mac,
>> +  struct ieee80211_vif *vif)
>> +{
>> + struct ath10k_fw_stats_peer *peer = data;
>> + struct ath10k_vif *arvif;
>> +
>> + if (memcmp(vif->addr, peer->peer_macaddr, ETH_ALEN))
>> + return;
>> +
>> + arvif = (void *)vif->drv_priv;
>> + arvif->tx_rate_kbps = peer->peer_tx_rate;
>> + arvif->rx_rate_kbps = peer->peer_rx_rate;
>> +}
>> +
>> +static void ath10k_mac_update_txrx_rate(struct ath10k *ar,
>> + struct ath10k_fw_stats *stats)
>> +{
>> + struct ieee80211_hw *hw = ar->hw;
>> + struct ath10k_fw_stats_peer *peer;
>> + struct ath10k_sta *arsta;
>> + struct ieee80211_sta *sta;
>> + const u8 *localaddr = NULL;
>> +
>> + rcu_read_lock();
>> +
>> + list_for_each_entry(peer, >peers, list) {
>> + /* This doesn't account for multiple STA connected on different
>> +  * vifs. Unfortunately there's no way to derive that from the 
>> available
>> +  * information.
>> +  */
>> + sta = ieee80211_find_sta_by_ifaddr(hw,
>> +peer->peer_macaddr,
>> +localaddr);
>> + if (!sta) {
>> + /* This tries to update multicast rates */
>> + ieee80211_iterate_active_interfaces_atomic(
>> + hw,
>> + IEEE80211_IFACE_ITER_NORMAL,
>> + ath10k_mac_update_txrx_rate_iter,
>> + peer);
>> + continue;
>> + }
>> +
>> + arsta = (void *)sta->drv_priv;
>> + arsta->tx_rate_kbps = peer->peer_tx_rate;
>> + arsta->rx_rate_kbps = peer->peer_rx_rate;
>> + }
>> +
>> + rcu_read_unlock();
>> +}
>> +
>>  void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
>>  {
>>  

Re: [RFCv2 0/3] mac80211: implement fq codel

[Michal Kazior]

On 21 March 2016 at 18:10, Dave Taht  wrote:
> thx.
>
> a lot to digest.
>
> A) quick notes on "flent-gui bursts_11e-2016-03-21T09*.gz"
>
> 1) the new bursts_11e test *should* have stuck stuff in the VI and VO
> queues, and there *should* have been some sort of difference shown on
> the plots with it. There wasn't.

traffic-gen generates only BE traffic. Everything else runs UDP_RR
which doesn't generate a lot of traffic.


> For diffserv markings I used BE=CS0, BK=CS1, VI=CS5, and VO=EF.
> CS6/CS7 should also land in VO (at least with the soft mac handler
> last I looked). Is there a way to check if you are indeed exercising
> all four 802.11e hardware queues in this test? in ath9k it is the
> "xmit" sysfs var

Hmm.. there are no txq stats. I guess it makes sense to have them?

There is /sys/kernel/debug/ieee80211/phy*/fq which dumps state of all
queues which will be mostly empty with UDP_RR. You can run netperf UDP
stream with diffserv marking to see onto which tid they are mapped.
You can see tid-AC mappings here:
https://wireless.wiki.kernel.org/en/developers/documentation/mac80211/queues

I just checked and EF ends up as tid5 which is VI. It's actually the
same as CS5. You can use CS7 to run on tid7 which is VO.


> 2) In all the old cases the BE UDP_RR flow died on the first burst
> (why?), and the fullpatch preserved it.

I think it's related to my setup which involves veth pairs. I use them
to simulate bridging/AP behavior but maybe it's not doing the job
right, hmm..


> (I would have kind of hoped to
> have seen the BK flow die, actually, in the fullpatch)

There's no extra weight priority to BK. The difference between BE and
BK in 802.11 is contention window access time so BK gets less txops
statistically. Both share the same txop, which is 5.484ms in most
cases.


> 3) I am also confused on 802.11ac - can VO aggregate? ( can't in in 802.11n).

Yes, it should be albeit VI and VO have shorter txop compared to
BE/BK: 3.008ms and 1.504ms respectively.

UDP_RR doesn't really create a lot of opportunities for aggregation.
If you want to see how different queues behave when loaded you'll need
to modify traffic-gen and add bursts across different ACs in the
bursts_11e test.


Michał

Re: [Make-wifi-fast] [RFCv2 1/3] mac80211: implement fq_codel for software queuing

[Michal Kazior]

On 22 March 2016 at 02:35, David Lang <da...@lang.hm> wrote:
> On Wed, 16 Mar 2016, Michal Kazior wrote:
>
>> Since 11n aggregation become important to get the
>> best out of txops. However aggregation inherently
>> requires buffering and queuing. Once variable
>> medium conditions to different associated stations
>> is considered it became apparent that bufferbloat
>> can't be simply fought with qdiscs for wireless
>> drivers.
>
> If the network is quiet enough, don't do any buffering, but in almost all
> situations you are going to need to buffer starting no later than the second
> packet you try to send.
>
> Don't try to make queueing occur, just deal with the queues that form
> naturally because you can't transmit data any faster (and work to keep them
> under some semblence of control)
[...]

This is what already happens. Queues typically start to build up when
hardware tx fifos/queues become busy so by the time they become
available you might have a bunch of frames you can aggregate.

The patch is more about getting rid of qdiscs because it's inherently
hard to teach them how 802.11 aggregation works (per-station per-tid)
and the ever-changing nature of per-station tx conditions.

I'll update the commit log to better reflect what is being done.


Michał

Re: [RFCv2 0/3] mac80211: implement fq codel

[Michal Kazior]

I've re-tested selected cases with wmm_enabled=0 set on the DUT AP.
I'm attaching results.

Naming:
 * "old-" is without mac/ath10k changes (referred to as kvalo-reverts
previously) and fq_codel on qdiscs,
 * "patched-" is all patches applied (both mac and ath),
 * "-be-bursts" is stock "bursts" flent test,
 * "-all-bursts" is modified "bursts" flent test to burst on all 3
tids simultaneously: tid0(BE), tid1(BK), tid5(VI).


Michał

On 16 March 2016 at 19:36, Dave Taht <dave.t...@gmail.com> wrote:
> That is the sanest 802.11e queue behavior I have ever seen!  (at both
> 6 and 300mbit! in the ath10k patched mac test)
>
> It would be good to add a flow to this test that exercises the VI
> queue (CS5 diffserv marking?), and to repeat this test with wmm
> disabled for comparison.
>
>
> Dave Täht
> Let's go make home routers and wifi faster! With better software!
> https://www.gofundme.com/savewifi
>
>
> On Wed, Mar 16, 2016 at 8:37 AM, Dave Taht <dave.t...@gmail.com> wrote:
>> it is helpful to name the test files coherently in the flent tests, in
>> addition to using a directory structure and timestamp. It makes doing
>> comparison plots in data->add-other-open-data-files simpler. "-t
>> patched-mac-300mbps", for example.
>>
>> Also netperf from svn (maybe 2.7, don't remember) will restart udp_rr
>> after a packet loss in 250ms. Seeing a loss on UDP_RR and it stop for
>> a while is "ok".
>> Dave Täht
>> Let's go make home routers and wifi faster! With better software!
>> https://www.gofundme.com/savewifi
>>
>>
>> On Wed, Mar 16, 2016 at 3:26 AM, Michal Kazior <michal.kaz...@tieto.com> 
>> wrote:
>>> On 16 March 2016 at 11:17, Michal Kazior <michal.kaz...@tieto.com> wrote:
>>>> Hi,
>>>>
>>>> Most notable changes:
>>> [...]
>>>>  * ath10k proof-of-concept that uses the new tx
>>>>scheduling (will post results in separate
>>>>email)
>>>
>>> I'm attaching a bunch of tests I've done using flent. They are all
>>> "burst" tests with burst-ports=1 and burst-length=2. The testing
>>> topology is:
>>>
>>>AP > STA
>>>AP )) (( STA
>>>  [veth]--[br]--[wlan] )) (( [wlan]
>>>
>>> You can notice that in some tests plot data gets cut-off. There are 2
>>> problems I've identified:
>>>  - excess drops (not a problem with the patchset and can be seen when
>>> there's no codel-in-mac or scheduling isn't used)
>>>  - UDP_RR hangs (apparently QCA99X0 I have hangs for a few hundred ms
>>> sometimes at times and doesn't Rx frames causing UDP_RR to stop
>>> mid-way; confirmed with logs and sniffer; I haven't figured out *why*
>>> exactly, could be some hw/fw quirk)
>>>
>>> Let me know if you have questions or comments regarding my testing/results.
>>>
>>>
>>> Michał


bursts-2016-03-17T093033.443115.patched_all_bursts.flent.gz
Description: GNU Zip compressed data


bursts-2016-03-17T092946.721003.patched_be_bursts.flent.gz
Description: GNU Zip compressed data


bursts-2016-03-17T092445.132728.old_be_bursts.flent.gz
Description: GNU Zip compressed data


bursts-2016-03-17T091952.053950.old_all_bursts.flent.gz
Description: GNU Zip compressed data

Re: [RFCv2 0/3] mac80211: implement fq codel

[Michal Kazior]

On 16 March 2016 at 16:37, Dave Taht <dave.t...@gmail.com> wrote:
> it is helpful to name the test files coherently in the flent tests, in
> addition to using a directory structure and timestamp. It makes doing
> comparison plots in data->add-other-open-data-files simpler. "-t
> patched-mac-300mbps", for example.

Sorry. I'm still trying to figure out what variables are worth
considering for comparison purposes.


> Also netperf from svn (maybe 2.7, don't remember) will restart udp_rr
> after a packet loss in 250ms. Seeing a loss on UDP_RR and it stop for
> a while is "ok".

I'm using 2.6 straight out of debian repos so yeah. I guess I'll try
using more recent netperf if I can't figure out the hiccups.


Michał


> Dave Täht
> Let's go make home routers and wifi faster! With better software!
> https://www.gofundme.com/savewifi
>
>
> On Wed, Mar 16, 2016 at 3:26 AM, Michal Kazior <michal.kaz...@tieto.com> 
> wrote:
>> On 16 March 2016 at 11:17, Michal Kazior <michal.kaz...@tieto.com> wrote:
>>> Hi,
>>>
>>> Most notable changes:
>> [...]
>>>  * ath10k proof-of-concept that uses the new tx
>>>scheduling (will post results in separate
>>>email)
>>
>> I'm attaching a bunch of tests I've done using flent. They are all
>> "burst" tests with burst-ports=1 and burst-length=2. The testing
>> topology is:
>>
>>AP > STA
>>AP )) (( STA
>>  [veth]--[br]--[wlan] )) (( [wlan]
>>
>> You can notice that in some tests plot data gets cut-off. There are 2
>> problems I've identified:
>>  - excess drops (not a problem with the patchset and can be seen when
>> there's no codel-in-mac or scheduling isn't used)
>>  - UDP_RR hangs (apparently QCA99X0 I have hangs for a few hundred ms
>> sometimes at times and doesn't Rx frames causing UDP_RR to stop
>> mid-way; confirmed with logs and sniffer; I haven't figured out *why*
>> exactly, could be some hw/fw quirk)
>>
>> Let me know if you have questions or comments regarding my testing/results.
>>
>>
>> Michał

Re: [RFCv2 0/3] mac80211: implement fq codel

[Michal Kazior]

TxOP 0 has a special meaning in the standard. For HT/VHT it means the
it is actually limited to 5484us (mixed-mode) or 1us (greenfield).

I suspect the BK/BE latency difference has to do with the fact that
there's bulk traffic going on BE queues (this isn't reflected
explicitly in the plots). The `bursts` flent test includes short
bursts of traffic on tid0 (BE) which is shared with ICMP and BE UDP_RR
(seen as green and blue lines on the plot). Due to (intended) limited
outflow (6mbps) BE queues build up and don't drain for the duration of
the entire test creating more opportunities for aggregating BE traffic
while other queues are near-empty and very short (time wise as well).
If you consider Wi-Fi is half-duplex and latency in the entire stack
(for processing ICMP and UDP_RR) is greater than 11e contention window
timings you can get your BE flow responses with extra delay (since
other queues might have responses ready quicker).

I've modified traffic-gen and re-run tests with bursts on all tested
tids/ACs (tid0, tid1, tid5). I'm attaching the results.

With bursts on all tids you can clearly see BK has much higher latency than BE.

(Note, I've changed my AP to QCA988X with oldie firmware 10.1.467 for
this test; it doesn't have the weird hiccups I was seeing on QCA99X0
and newer QCA988X firmware reports bogus expected throughput which is
most likely a result of my sloppy proof-of-concept change in ath10k).


Michał

On 16 March 2016 at 20:48, Jasmine Strong  wrote:
> BK usually has 0 txop, so it doesn't do aggregation.
>
> On Wed, Mar 16, 2016 at 11:55 AM, Bob Copeland  wrote:
>>
>> On Wed, Mar 16, 2016 at 11:36:31AM -0700, Dave Taht wrote:
>> > That is the sanest 802.11e queue behavior I have ever seen!  (at both
>> > 6 and 300mbit! in the ath10k patched mac test)
>>
>> Out of curiosity, why does BE have larger latency than BK in that chart?
>> I'd have expected the opposite.
>>
>> --
>> Bob Copeland %% http://bobcopeland.com/
>>
>> ___
>> ath10k mailing list
>> ath...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/ath10k
>
>


bursts-2016-03-17T083932.549858.qca988x_10_1_467_fqmac_ath10k_with_tx_sched_6mbps_.flent.gz
Description: GNU Zip compressed data


bursts-2016-03-17T083803.348752.qca988x_10_1_467_fqmac_ath10k_with_tx_sched_6mbps_.flent.gz
Description: GNU Zip compressed data

Re: [RFCv2 0/3] mac80211: implement fq codel

[Michal Kazior]

On 16 March 2016 at 11:17, Michal Kazior <michal.kaz...@tieto.com> wrote:
> Hi,
>
> Most notable changes:
[...]
>  * ath10k proof-of-concept that uses the new tx
>scheduling (will post results in separate
>email)

I'm attaching a bunch of tests I've done using flent. They are all
"burst" tests with burst-ports=1 and burst-length=2. The testing
topology is:

   AP > STA
   AP )) (( STA
 [veth]--[br]--[wlan] )) (( [wlan]

You can notice that in some tests plot data gets cut-off. There are 2
problems I've identified:
 - excess drops (not a problem with the patchset and can be seen when
there's no codel-in-mac or scheduling isn't used)
 - UDP_RR hangs (apparently QCA99X0 I have hangs for a few hundred ms
sometimes at times and doesn't Rx frames causing UDP_RR to stop
mid-way; confirmed with logs and sniffer; I haven't figured out *why*
exactly, could be some hw/fw quirk)

Let me know if you have questions or comments regarding my testing/results.


Michał


fq.tar.gz
Description: GNU Zip compressed data

[RFCv2 1/3] mac80211: implement fq_codel for software queuing

[Michal Kazior]

Since 11n aggregation become important to get the
best out of txops. However aggregation inherently
requires buffering and queuing. Once variable
medium conditions to different associated stations
is considered it became apparent that bufferbloat
can't be simply fought with qdiscs for wireless
drivers.

This bases on codel5 and sch_fq_codel.c. It may
not be the Right Thing yet but it should at least
provide a framework for more improvements.

Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
---
 include/net/mac80211.h |  96 ++-
 net/mac80211/agg-tx.c  |   8 +-
 net/mac80211/cfg.c |   2 +-
 net/mac80211/codel.h   | 264 ++
 net/mac80211/codel_i.h |  89 ++
 net/mac80211/debugfs.c | 267 ++
 net/mac80211/ieee80211_i.h |  45 +++-
 net/mac80211/iface.c   |  25 +-
 net/mac80211/main.c|   9 +-
 net/mac80211/rx.c  |   2 +-
 net/mac80211/sta_info.c|  10 +-
 net/mac80211/sta_info.h|  27 ++
 net/mac80211/status.c  |  64 +
 net/mac80211/tx.c  | 658 ++---
 net/mac80211/util.c|  21 +-
 15 files changed, 1503 insertions(+), 84 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 a5cb1528..947d827f254b 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -565,6 +565,16 @@ struct ieee80211_bss_conf {
struct ieee80211_p2p_noa_attr p2p_noa_attr;
 };
 
+/*
+ * struct codel_params - contains codel parameters
+ * @interval:  initial drop rate
+ * @target: maximum persistent sojourn time
+ */
+struct codel_params {
+   u64 interval;
+   u64 target;
+};
+
 /**
  * enum mac80211_tx_info_flags - flags to describe transmission 
information/status
  *
@@ -853,6 +863,8 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate 
*rate)
  * @band: the band to transmit on (use for checking for races)
  * @hw_queue: HW queue to put the frame on, skb_get_queue_mapping() gives the 
AC
  * @ack_frame_id: internal frame ID for TX status, used internally
+ * @expected_duration: number of microseconds the stack expects this frame to
+ * take to tx. Used for fair queuing.
  * @control: union for control data
  * @status: union for status data
  * @driver_data: array of driver_data pointers
@@ -865,11 +877,10 @@ ieee80211_rate_get_vht_nss(const struct ieee80211_tx_rate 
*rate)
 struct ieee80211_tx_info {
/* common information */
u32 flags;
-   u8 band;
-
-   u8 hw_queue;
-
-   u16 ack_frame_id;
+   u32 band:2,
+   hw_queue:5,
+   ack_frame_id:15,
+   expected_duration:10;
 
union {
struct {
@@ -888,8 +899,18 @@ struct ieee80211_tx_info {
/* only needed before rate control */
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 */
@@ -2114,8 +2135,8 @@ enum ieee80211_hw_flags {
  * @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.
+ * @txq_cparams: codel parameters to control tx queueing dropping behavior
+ * @txq_limit: maximum number of frames queuesd
  */
 struct ieee80211_hw {
struct ieee80211_conf conf;
@@ -2145,7 +2166,8 @@ struct ieee80211_hw {
u8 uapsd_max_sp_len;
u8 n_cipher_schemes;
const struct ieee80211_cipher_scheme *cipher_schemes;
-   int txq_ac_max_pending;
+   struct codel_params txq_cparams;
+   u32 txq_limit;
 };
 
 static inline bool _ieee80211_hw_check(struct ieee80211_hw *hw,
@@ -5633,6 +5655,9 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw 
*hw,
  * txq state can change half-way of this function and the caller may end up
  * with "new" frame_cnt and "old" byte_cnt or vice-versa.
  *
+ * Moreover returned values are best-case, i.e. a

[RFCv2 2/3] ath10k: report per-station tx/rate rates to mac80211

[Michal Kazior]

The rate control is offloaded by firmware so it's
challanging to provide expected throughput value
for given station.

This approach is naive as it reports last tx rate
used for given station as provided by firmware
stat event.

This should be sufficient for airtime estimation
used for fq-codel-in-mac80211 tx scheduling
purposes now.

This patch uses a very hacky way to get the stats.
This is sufficient for proof-of-concept but must
be cleaned up properly eventually.

Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h  |  5 +++
 drivers/net/wireless/ath/ath10k/debug.c | 61 +
 drivers/net/wireless/ath/ath10k/mac.c   | 26 --
 drivers/net/wireless/ath/ath10k/wmi.h   |  2 +-
 4 files changed, 76 insertions(+), 18 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h 
b/drivers/net/wireless/ath/ath10k/core.h
index 23ba03fb7a5f..3f76669d44cf 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -331,6 +331,9 @@ struct ath10k_sta {
/* protected by conf_mutex */
bool aggr_mode;
u64 rx_duration;
+
+   u32 tx_rate_kbps;
+   u32 rx_rate_kbps;
 #endif
 };
 
@@ -372,6 +375,8 @@ struct ath10k_vif {
s8 def_wep_key_idx;
 
u16 tx_seq_no;
+   u32 tx_rate_kbps;
+   u32 rx_rate_kbps;
 
union {
struct {
diff --git a/drivers/net/wireless/ath/ath10k/debug.c 
b/drivers/net/wireless/ath/ath10k/debug.c
index 076d29b53ddf..cc7ebf04ae00 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -316,6 +316,58 @@ static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
spin_unlock_bh(>data_lock);
 }
 
+static void ath10k_mac_update_txrx_rate_iter(void *data,
+u8 *mac,
+struct ieee80211_vif *vif)
+{
+   struct ath10k_fw_stats_peer *peer = data;
+   struct ath10k_vif *arvif;
+
+   if (memcmp(vif->addr, peer->peer_macaddr, ETH_ALEN))
+   return;
+
+   arvif = (void *)vif->drv_priv;
+   arvif->tx_rate_kbps = peer->peer_tx_rate;
+   arvif->rx_rate_kbps = peer->peer_rx_rate;
+}
+
+static void ath10k_mac_update_txrx_rate(struct ath10k *ar,
+   struct ath10k_fw_stats *stats)
+{
+   struct ieee80211_hw *hw = ar->hw;
+   struct ath10k_fw_stats_peer *peer;
+   struct ath10k_sta *arsta;
+   struct ieee80211_sta *sta;
+   const u8 *localaddr = NULL;
+
+   rcu_read_lock();
+
+   list_for_each_entry(peer, >peers, list) {
+   /* This doesn't account for multiple STA connected on different
+* vifs. Unfortunately there's no way to derive that from the 
available
+* information.
+*/
+   sta = ieee80211_find_sta_by_ifaddr(hw,
+  peer->peer_macaddr,
+  localaddr);
+   if (!sta) {
+   /* This tries to update multicast rates */
+   ieee80211_iterate_active_interfaces_atomic(
+   hw,
+   IEEE80211_IFACE_ITER_NORMAL,
+   ath10k_mac_update_txrx_rate_iter,
+   peer);
+   continue;
+   }
+
+   arsta = (void *)sta->drv_priv;
+   arsta->tx_rate_kbps = peer->peer_tx_rate;
+   arsta->rx_rate_kbps = peer->peer_rx_rate;
+   }
+
+   rcu_read_unlock();
+}
+
 void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
 {
struct ath10k_fw_stats stats = {};
@@ -335,6 +387,8 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, 
struct sk_buff *skb)
goto free;
}
 
+   ath10k_mac_update_txrx_rate(ar, );
+
/* Stat data may exceed htc-wmi buffer limit. In such case firmware
 * splits the stats data and delivers it in a ping-pong fashion of
 * request cmd-update event.
@@ -351,13 +405,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, 
struct sk_buff *skb)
if (peer_stats_svc)
ath10k_sta_update_rx_duration(ar, );
 
-   if (ar->debug.fw_stats_done) {
-   if (!peer_stats_svc)
-   ath10k_warn(ar, "received unsolicited stats update 
event\n");
-
-   goto free;
-   }
-
num_peers = ath10k_wmi_fw_stats_num_peers(>debug.fw_stats.peers);
num_vdevs = ath10k_wmi_fw_stats_num_vdevs(>debug.fw_stats.vdevs);
is_start = (list_empty(>debug.fw_stats.pdevs) &&
diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath

[RFCv2 3/3] ath10k: use ieee80211_tx_schedule()

[Michal Kazior]

The wake_tx_queue() scheduling for non-pull-push
(or threshold based pull-push) wasn't really smart
nor fair.

Instead use the new mac80211 Tx scheduling helper
which is part of the fq-codel-in-mac80211 proof of
concept.

Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.c |   2 -
 drivers/net/wireless/ath/ath10k/core.h |   3 -
 drivers/net/wireless/ath/ath10k/mac.c  | 100 -
 3 files changed, 50 insertions(+), 55 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index 2389c0713c13..1848e0b6206a 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -2049,9 +2049,7 @@ struct ath10k *ath10k_core_create(size_t priv_size, 
struct device *dev,
 
mutex_init(>conf_mutex);
spin_lock_init(>data_lock);
-   spin_lock_init(>txqs_lock);
 
-   INIT_LIST_HEAD(>txqs);
INIT_LIST_HEAD(>peers);
init_waitqueue_head(>peer_mapping_wq);
init_waitqueue_head(>htt.empty_tx_wq);
diff --git a/drivers/net/wireless/ath/ath10k/core.h 
b/drivers/net/wireless/ath/ath10k/core.h
index 3f76669d44cf..1f94af046f79 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -801,10 +801,7 @@ struct ath10k {
 
/* protects shared structure data */
spinlock_t data_lock;
-   /* protects: ar->txqs, artxq->list */
-   spinlock_t txqs_lock;
 
-   struct list_head txqs;
struct list_head arvifs;
struct list_head peers;
struct ath10k_peer *peer_map[ATH10K_MAX_NUM_PEER_IDS];
diff --git a/drivers/net/wireless/ath/ath10k/mac.c 
b/drivers/net/wireless/ath/ath10k/mac.c
index addef9179dbe..5b8becb30b62 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3624,17 +3624,14 @@ void ath10k_mgmt_over_wmi_tx_work(struct work_struct 
*work)
 
 static void ath10k_mac_txq_init(struct ieee80211_txq *txq)
 {
-   struct ath10k_txq *artxq = (void *)txq->drv_priv;
-
if (!txq)
return;
 
-   INIT_LIST_HEAD(>list);
+   /* It's useful to keep this even though it doesn't do anything now */
 }
 
 static void ath10k_mac_txq_unref(struct ath10k *ar, struct ieee80211_txq *txq)
 {
-   struct ath10k_txq *artxq = (void *)txq->drv_priv;
struct ath10k_skb_cb *cb;
struct sk_buff *msdu;
int msdu_id;
@@ -3642,11 +3639,6 @@ static void ath10k_mac_txq_unref(struct ath10k *ar, 
struct ieee80211_txq *txq)
if (!txq)
return;
 
-   spin_lock_bh(>txqs_lock);
-   if (!list_empty(>list))
-   list_del_init(>list);
-   spin_unlock_bh(>txqs_lock);
-
spin_lock_bh(>htt.tx_lock);
idr_for_each_entry(>htt.pending_tx, msdu, msdu_id) {
cb = ATH10K_SKB_CB(msdu);
@@ -3725,7 +3717,7 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,
ath10k_htt_tx_dec_pending(htt, is_mgmt);
spin_unlock_bh(>htt.tx_lock);
 
-   return -ENOENT;
+   return 0;
}
 
ath10k_mac_tx_h_fill_cb(ar, vif, txq, skb);
@@ -3752,44 +3744,59 @@ int ath10k_mac_tx_push_txq(struct ieee80211_hw *hw,
return skb_len;
 }
 
+#define MIN_TX_SLOTS 42
+
+static int ath10k_mac_tx_wake(struct ieee80211_hw *hw,
+ struct ieee80211_txq *txq,
+ int budget)
+{
+   struct ath10k *ar = hw->priv;
+   int sent = 0;
+   int ret;
+   int slots;
+
+   if (!ath10k_mac_tx_can_push(hw, txq))
+   return 0;
+
+   /* This gives more opportunity to form longer bursts when there's a lot
+* of stations active
+*/
+   slots = ar->htt.max_num_pending_tx - ar->htt.num_pending_tx;
+   if (slots < MIN_TX_SLOTS)
+   return -1;
+
+   while (budget > 0) {
+   ret = ath10k_mac_tx_push_txq(hw, txq);
+   if (ret <= 0)
+   break;
+
+   sent++;
+   budget -= ret;
+   }
+
+   if (ret >= 0)
+   return sent;
+   else
+   return -1;
+}
+
 void ath10k_mac_tx_push_pending(struct ath10k *ar)
 {
struct ieee80211_hw *hw = ar->hw;
-   struct ieee80211_txq *txq;
-   struct ath10k_txq *artxq;
-   struct ath10k_txq *last;
int ret;
-   int max;
 
-   spin_lock_bh(>txqs_lock);
-   rcu_read_lock();
-
-   last = list_last_entry(>txqs, struct ath10k_txq, list);
-   while (!list_empty(>txqs)) {
-   artxq = list_first_entry(>txqs, struct ath10k_txq, list);
-   txq = container_of((void *)artxq, struct ieee80211_txq,
-  drv_priv);
-
-   /* Prevent aggressive sta/tid taking ove

[RFCv2 0/3] mac80211: implement fq codel

[Michal Kazior]

Hi,

Most notable changes:
 * fixes (duh); fairness should work better now,
 * EWMA codel target based on estimated service
   time,
 * new tx scheduling helper with in-flight
   duration limiting (same idea Emmanuel
   had for iwlwifi),
 * added a few debugfs hooks.
 * ath10k proof-of-concept that uses the new tx
   scheduling (will post results in separate
   email)

The patch grew pretty big and I plan on splitting
it before next submission. Any suggestions?

The tx scheduling probably needs more work and
testing. I didn't evaluate how CPU intensive it is
nor how it influences things like peak throughput
(lab conditions et al) yet.

I've uploaded a branch for convenience:

  https://github.com/kazikcz/linux/tree/fqmac-rfc-v2

This is based on Kalle's ath tree.


Michal Kazior (3):
  mac80211: implement fq_codel for software queuing
  ath10k: report per-station tx/rate rates to mac80211
  ath10k: use ieee80211_tx_schedule()

 drivers/net/wireless/ath/ath10k/core.c  |   2 -
 drivers/net/wireless/ath/ath10k/core.h  |   8 +-
 drivers/net/wireless/ath/ath10k/debug.c |  61 ++-
 drivers/net/wireless/ath/ath10k/mac.c   | 126 +++---
 drivers/net/wireless/ath/ath10k/wmi.h   |   2 +-
 include/net/mac80211.h  |  96 -
 net/mac80211/agg-tx.c   |   8 +-
 net/mac80211/cfg.c  |   2 +-
 net/mac80211/codel.h| 264 +
 net/mac80211/codel_i.h  |  89 +
 net/mac80211/debugfs.c  | 267 +
 net/mac80211/ieee80211_i.h  |  45 ++-
 net/mac80211/iface.c|  25 +-
 net/mac80211/main.c |   9 +-
 net/mac80211/rx.c   |   2 +-
 net/mac80211/sta_info.c |  10 +-
 net/mac80211/sta_info.h |  27 ++
 net/mac80211/status.c   |  64 
 net/mac80211/tx.c   | 658 ++--
 net/mac80211/util.c |  21 +-
 20 files changed, 1629 insertions(+), 157 deletions(-)
 create mode 100644 net/mac80211/codel.h
 create mode 100644 net/mac80211/codel_i.h

-- 
2.1.4

Re: [RFC/RFT] mac80211: implement fq_codel for software queuing

[Michal Kazior]

On 10 March 2016 at 19:57, Dave Taht  wrote:
>>> regular fq_codel uses 1024 and there has not been much reason to
>>> change it. In the case of an AP which has more limited memory, 256 or
>>> 1024 would be a good setting, per station. I'd stick to 1024 for now.
>>
>> Do note that the 4096 is shared _across_ station-tid queues. It is not
>> per-station. If you have 10 stations you still have 4096 flows
>> (actually 4096 + 16*10, because each tid - and there are 16 - has it's
>> own fallback flow in case of hash collision on the global flowmap to
>> maintain per-sta-tid queuing).
>
> I have to admit I didn't parse this well - still haven't, I think I
> need to draw. (got a picture?)

No picture, sorry. Let me rehash instead:

 - there's a pool of 4096 tubes (flows),
 - tubes in the pool are empty,
 - each tube has an id sticker (0,1,2,3,...),
 - each wi-fi station has 16 boxes (tids) for tubes as well,
 - each station box has 1 extra immovable tube (flow),
 - packets are hashed into tube id stickers,
 - matching tube can be:
   - in the pool;
 - move the tube into station-box the packet is associated with
 - put the packet into tube
   - in the station-box the packet is associated with
 - just put the packet into tube
   - in a station-box different from the one the packet is associated with
 - (this is the collision case for fallback flow)
 - put the packet into the immovable tube of station box the
packet is associated with
 - whenever a tube becomes empty it is moved back to the pool


> Where is this part happening in the code (or firmware?)

It's in the ieee80211_txq_enqueue(). My patch originally contains a
mistake (pointed out by Felix) and the function should actually look
like this:

> static void ieee80211_txq_enqueue(struct ieee80211_local *local,
>   struct txq_info *txqi,
>   struct sk_buff *skb)
> {
> struct ieee80211_fq *fq = >fq;
> struct ieee80211_hw *hw = >hw;
> struct txq_flow *flow;
> struct txq_flow *i;
> size_t idx = fq_hash(fq, skb);
>
> flow = >flows[idx];
>
> if (flow->txqi && flow->txqi != txqi)
> flow = >flow;
[...]


> I assembled a few of the patches to date (your fq_codel patch, avery's
> and tims ath9k stuff) and tested them, to no measurable effect,
> against linus's tree a day or two back. I also acquired an ath10k card
> - would one of these suit?
>
> http://www.amazon.com/gp/product/B011SIMFR8?psc=1=true_=oh_aui_detailpage_o08_s00

I haven't used this particular card. It's an older chip (no MU-MIMO)
but otherwise should work fine.


Michał

Re: [RFC/RFT] mac80211: implement fq_codel for software queuing

[Michal Kazior]

On 8 March 2016 at 14:14, Bob Copeland <m...@bobcopeland.com> wrote:
> On Tue, Mar 08, 2016 at 08:12:21AM +0100, Michal Kazior wrote:
>> However other drivers (e.g. ath10k) have offloaded rate control on
>> device. There's currently no way of doing this calculation. I was
>> thinking of drivers exporting tx_rate to mac80211 in some way - either
>> via a simple sta->tx_rate scalar that the driver is responsible for
>> updating, or a EWMA that driver updates (hopefully) periodically and
>> often enough. This should in theory at least allow an estimate how
>> much data on average you can fit into given time frame (e.g. txop, or
>> hardcoded 1ms).
>
> What about implementing ops->get_expected_throughput?  This would be
> useful for mesh (both 11s and batman) as well since they need to
> estimate link quality to pick a path.

Oh, good point! I totally forgot about this because - in the back of
my head - I was convinced this is available only for minstrel. I guess
this could work as well.


Michał

Re: [RFC/RFT] mac80211: implement fq_codel for software queuing

[Michal Kazior]

On 8 March 2016 at 00:06, Dave Taht  wrote:
> Dear Michal:
>
> Going through this patchset... (while watching it compile)
>
>
> +   if (!local->hw.txq_cparams.target)
> +   local->hw.txq_cparams.target = MS2TIME(5);
>
> MS2TIME(20) for now and/or add something saner to this than !*backlog
>
> target will not be a constant in the long run.
>
> +   if (now - custom_codel_get_enqueue_time(skb) < p->target ||
> +   !*backlog) {
> +   /* went below - stay below for at least interval */
> +   vars->first_above_time = 0;
> +   return false;
> +   }
>
>
> *backlog < some_sane_value_for_an_aggregate_for_this_station
>
> Unlike regular codel *backlog should be a ptr to the queuesize for
> this station, not the total queue.
>
> regular codel, by using the shared backlog for all queues, is trying
> to get to a 1 packet depth for all queues, (see commit:
> 865ec5523dadbedefbc5710a68969f686a28d928 ), and store the flow in the
> network, not the queue...
>
> BUT in wifi's case you want to provide good service to all stations,
> which is filling up an aggregate
> for each... (and varying the "sane_value_for_the_aggregate" to suit
> per sta service time requirements in a given round of all stations).

Hmm.. This actually makes me think that:

skb = codel_dequeue(flow, >backlog, >cvars,
>txq_cparams, codel_get_time(), false);

is kind of wrong.

If you want to maintain per-sta aggregate-long queue this probably
needs a sta->backlog instead of flow->backlog (because you can have
multiple flows per station) in the first place.

Not quite sure about cvars though. Thoughts?


Michał

Re: [RFC/RFT] mac80211: implement fq_codel for software queuing

[Michal Kazior]

On 7 March 2016 at 19:28, Dave Taht  wrote:
> On Mon, Mar 7, 2016 at 9:14 AM, Avery Pennarun  wrote:
>> On Mon, Mar 7, 2016 at 11:54 AM, Dave Taht  wrote:
[...]
>>> the underlying code needs to be striving successfully for per-station
>>> airtime fairness for this to work at all, and the driver/card
>>> interface nearly as tight as BQL is for the fq portion to behave
>>> sanely. I'd configure codel at a higher target and try to observe what
>>> is going on at the fq level til that got saner.
>>
>> That seems like two good goals.  So Emmanuel's BQL-like thing seems
>> like we'll need it soon.
>>
>> As for per-station airtime fairness, what's a good approximation of
>> that?  Perhaps round-robin between stations, one aggregate per turn,
>> where each aggregate has a maximum allowed latency?
>
> Strict round robin is a start, and simplest, yes. Sure.
>
> "Oldest station queues first" on a round (probably) has higher
> potential for maximizing txops, but requires more overhead. (shortest
> queue first would be bad). There's another algo based on last received
> packets from a station possibly worth fiddling with in the long run...
>
> as "maximum allowed latency" - well, to me that is eventually also a
> variable, based on the number of stations that have to be scheduled on
> that round. Trying to get away from 10 stations eating 5.7ms each +
> return traffic on a round would be nicer. If you want a constant, for
> now, aim for 2048us or 1TU.

The "one aggregate per turn" is a tricky.

I guess you can guarantee this sort of thing on ath9k/mt76.

This isn't the case for other drivers, e.g. ath10k has a flat tx
queue. You don't really know if the 40 frames you submitted will be
sent with 1, 2 or 3 aggregates. They might not be aggregated at all.

Best thing you can do is to estimate how much bytes can you fit into a
txop on target sta-tid/ac assuming you can get last/avg tx rate to
given station (should be doable on ath10k at least).

Moreover, for MU-MIMO you typically want to burst a few aggregates in
txop to make the sounding to pay off. And this is again tricky on flat
tx queue where you don't really know if target stations can do an
efficient MU transmission and worst case you'll end up with stacking
up 3 txops worth of data in queues.


Oh, and the unfortunate thing is ath10k does offloaded powersaving
which means some frames can clog up tx queues unnecessarily until next
TBTT. This is something that will need to be addressed as well in tx
scheduling. Not sure how yet.

A quick idea - perhaps we could unify ps_tx_buf with txqs and make use
of txqs internally regardless of wake_tx_queue implementation?


[...]
> [1] I've published a lot of stuff showing how damaging 802.11e's edca
> scheduling can be - I lean towards, at most, 2-3 aggregates being in
> the hardware, essentially disabling the VO queue on 802.11n (not sure
> on ac), in favor of VI, promoting or demoting an assembled aggregate
> from BE to BK or VI as needed at the last second before submitting it
> to the hardware, trying harder to only have one aggregate outstanding
> to one station at a time, etc.

Makes sense, but (again) tricky for drivers such as ath10k which have
a flat tx queue. Perhaps I could maintain a simulation of aggregates
or some sort of barriers and hope it's "good enough".


Michał

Re: [RFC/RFT] mac80211: implement fq_codel for software queuing

[Michal Kazior]

On 8 March 2016 at 00:06, Dave Taht <dave.t...@gmail.com> wrote:
> Dear Michal:
>
> Going through this patchset... (while watching it compile)
>
>
> +   if (!local->hw.txq_cparams.target)
> +   local->hw.txq_cparams.target = MS2TIME(5);
>
> MS2TIME(20) for now and/or add something saner to this than !*backlog

Yep, makes sense.


> target will not be a constant in the long run.
>
> +   if (now - custom_codel_get_enqueue_time(skb) < p->target ||
> +   !*backlog) {
> +   /* went below - stay below for at least interval */
> +   vars->first_above_time = 0;
> +   return false;
> +   }
>
> *backlog < some_sane_value_for_an_aggregate_for_this_station
>
> Unlike regular codel *backlog should be a ptr to the queuesize for
> this station, not the total queue.
>
> regular codel, by using the shared backlog for all queues, is trying
> to get to a 1 packet depth for all queues, (see commit:
> 865ec5523dadbedefbc5710a68969f686a28d928 ), and store the flow in the
> network, not the queue...
>
> BUT in wifi's case you want to provide good service to all stations,
> which is filling up an aggregate
> for each... (and varying the "sane_value_for_the_aggregate" to suit
> per sta service time requirements in a given round of all stations).

This is tricky. Drivers that use minstrel can probably do the estimate
rather easily.

However other drivers (e.g. ath10k) have offloaded rate control on
device. There's currently no way of doing this calculation. I was
thinking of drivers exporting tx_rate to mac80211 in some way - either
via a simple sta->tx_rate scalar that the driver is responsible for
updating, or a EWMA that driver updates (hopefully) periodically and
often enough. This should in theory at least allow an estimate how
much data on average you can fit into given time frame (e.g. txop, or
hardcoded 1ms).

I'll try looking more into this. Any hints/suggestion welcome.


>
> ...
>
> +   fq->flows_cnt = 4096;
>
> regular fq_codel uses 1024 and there has not been much reason to
> change it. In the case of an AP which has more limited memory, 256 or
> 1024 would be a good setting, per station. I'd stick to 1024 for now.

Do note that the 4096 is shared _across_ station-tid queues. It is not
per-station. If you have 10 stations you still have 4096 flows
(actually 4096 + 16*10, because each tid - and there are 16 - has it's
own fallback flow in case of hash collision on the global flowmap to
maintain per-sta-tid queuing).

With that in mind do you still think 1024 is enough?


> With large values for flows_cnt, fq, dominates, for small values, aqm
> does. We did quite a lot of testing at 16 and 32 queues in the early
> days, with pretty good results, except when we didn't. Cake went whole
> hog with an 8 way set associative hash leading to "near perfect" fq,
> which, at the cost of more cpu overhead, could cut the number of
> queues down by a lot, also. Eric did "perfect" fq with sch_fq...

Out of curiosity - do you have any numbers to compare against
fq_codel? Like hash collision probability vs number of active flows?


> (btw: another way to test how codel is working is to set flows_cnt to
> 1. I will probably end up doing that at some point)
>
> +   fq->perturbation = prandom_u32();
> +   fq->quantum = 300;
>
> quantum 300 is a good compromise to maximize delivery of small packets
> from different flows. Probably the right thing on a station.
>
> It also has cpu overhead. Quantum=1514 is more of the right thing on an AP.
>
> (but to wax philosophical, per packet fairness rather than byte
> fairness probably spreads errors across more flows in a wifi aggregate
> than byte fairness, thus 300 remains a decent compromise if you can
> spare the cpu)
>
> ...
>
> where would be a suitable place to make (count, ecn_marks, drops)
> visible in this subsystem?

I was thinking of using debugfs for starters and then, once things
settle down, move to nl80211. Maybe we could re-use some enums like
TCA_FQ_CODEL_TARGET? Not sure if that's the best idea though. We might
need extra knobs to control and/or have in mind we might want to
replace the queuing algorithm at some point in the future as well
(which will need a different set of knobs).


>
> ...
>
> Is this "per station" or per station, per 802.11e queue?

What do you have in mind by "this"?



Michał

>
> Dave Täht
> Let's go make home routers and wifi faster! With better software!
> https://www.gofundme.com/savewifi
>
>
> On Fri, Feb 26, 2016 at 5:09 AM, Michal Kazior <michal.kaz...@tieto.com> 
> wrote:
>> Since 11n aggregation become important to get the
>> best out of

Re: [RFC/RFT] mac80211: implement fq_codel for software queuing

[Michal Kazior]

On 4 March 2016 at 03:48, Tim Shepard  wrote:
[...]
> (I am interested in knowing what other mac80211 drivers have been
>  modified to use the mac80211 intermediate software queues.   I know
>  Michal mentioned he has patches for ath10k that are not yet released,
>  and I know Felix is finishing up the mt76 driver which uses them.)

Patches for ath10k are under review since quite some time now (but are
not merged yet). The latest re-spin is:

  http://lists.infradead.org/pipermail/ath10k/2016-March/006923.html


Michał

Re: [RFC/RFT] mac80211: implement fq_codel for software queuing

[Michal Kazior]

On 1 March 2016 at 15:02, Johannes Berg <johan...@sipsolutions.net> wrote:
> On Fri, 2016-02-26 at 14:09 +0100, Michal Kazior wrote:
>>
>> +typedef u64 codel_time_t;
>
> Do we really need this? And if yes, does it have to be in the public
> header file? Why a typedef anyway?

Hmm.. I don't think it's strictly necessary. I just wanted to keep as
much from the original codel implementation as possible. I'm fine with
using just u64.


>> - * @txq_ac_max_pending: maximum number of frames per AC pending in
>> all txq
>> - *   entries for a vif.
>> + * @txq_cparams: codel parameters to control tx queueing dropping
>> behavior
>> + * @txq_limit: maximum number of frames queuesd
>
> typo - queued
>
>> @@ -2133,7 +2155,8 @@ struct ieee80211_hw {
>>   u8 uapsd_max_sp_len;
>>   u8 n_cipher_schemes;
>>   const struct ieee80211_cipher_scheme *cipher_schemes;
>> - int txq_ac_max_pending;
>> + struct codel_params txq_cparams;
>
> Should this really be a parameter the driver determines?

I would think so, or at least it should be able to influence it in
*some* way. You can have varying degree of induced latency depending
on fw/hw tx queue depth, air conditions and possible tx rates implying
different/varying RTT. Cake[1] even has a few RTT presets like: lan,
internet, satellite.

I don't really have a plan how exactly a driver could make use of it
for benefit though. It might end up not being necessary after all if
generic tx scheduling materializes in mac80211.


[1]: http://www.bufferbloat.net/projects/codel/wiki/Cake


>> +static void ieee80211_if_setup_no_queue(struct net_device *dev)
>> +{
>> + ieee80211_if_setup(dev);
>> + dev->priv_flags |= IFF_NO_QUEUE;
>> + /* Note for backporters: use dev->tx_queue_len = 0 instead
>> of IFF_ */
>
> Heh. Remove that comment; we have an spatch in backports already :)

I've put it in the RFC specifically in case anyone wants to port this
bypassing backports, e.g. to openwrt's quilt (so when compilation
fails, you can quickly fix it up). I'll remove it before proper
submission obviously :)


>> --- a/net/mac80211/sta_info.h
>> +++ b/net/mac80211/sta_info.h
>> @@ -19,6 +19,7 @@
>>  #include 
>>  #include 
>>  #include "key.h"
>> +#include "codel_i.h"
>>
>>  /**
>>   * enum ieee80211_sta_info_flags - Stations flags
>> @@ -327,6 +328,32 @@ struct mesh_sta {
>>
>>  DECLARE_EWMA(signal, 1024, 8)
>>
>> +struct txq_info;
>> +
>> +/**
>> + * struct txq_flow - per traffic flow queue
>> + *
>> + * This structure is used to distinguish and queue different traffic
>> flows
>> + * separately for fair queueing/AQM purposes.
>> + *
>> + * @txqi: txq_info structure it is associated at given time
>> + * @flowchain: can be linked to other flows for RR purposes
>> + * @backlogchain: can be linked to other flows for backlog sorting
>> purposes
>> + * @queue: sk_buff queue
>> + * @cvars: codel state vars
>> + * @backlog: number of bytes pending in the queue
>> + * @deficit: used for fair queueing balancing
>> + */
>> +struct txq_flow {
>> + struct txq_info *txqi;
>> + struct list_head flowchain;
>> + struct list_head backlogchain;
>> + struct sk_buff_head queue;
>> + struct codel_vars cvars;
>> + u32 backlog;
>> + u32 deficit;
>> +};
>> +
>>  /**
>>   * struct sta_info - STA information
>>   *
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index af584f7cdd63..f42f898cb8b5 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> @@ -34,6 +34,7 @@
>>  #include "wpa.h"
>>  #include "wme.h"
>>  #include "rate.h"
>> +#include "codel.h"
>
>> +static inline codel_time_t
>> +custom_codel_get_enqueue_time(struct sk_buff *skb)
>
> There are a lot of inlines here - first of all, do they all need to be
> inline?

I just followed the style of code found in codel implementation. I
figured there's a reason why is uses `inline` although I didn't do any
measurements myself.


> And secondly, perhaps it would make some sense to move this into
> another file?

I did consider it briefly but decided it'll be beneficial for the
compiler to have tx hotpath in a single file.


Michał

Re: [RFC/RFT] mac80211: implement fq_codel for software queuing

[Michal Kazior]

On 26 February 2016 at 17:48, Felix Fietkau  wrote:
[...]
>> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
>> index af584f7cdd63..f42f898cb8b5 100644
>> --- a/net/mac80211/tx.c
>> +++ b/net/mac80211/tx.c
>> + [...]
>> +static void ieee80211_txq_enqueue(struct ieee80211_local *local,
>> +   struct txq_info *txqi,
>> +   struct sk_buff *skb)
>> +{
>> + struct ieee80211_fq *fq = >fq;
>> + struct ieee80211_hw *hw = >hw;
>> + struct txq_flow *flow;
>> + struct txq_flow *i;
>> + size_t idx = fq_hash(fq, skb);
>> +
>> + flow = >flows[idx];
>> +
>> + if (flow->txqi)
>> + flow = >flow;
> I'm not sure I understand this part correctly, but shouldn't that be:
> if (flow->txqi && flow->txqi != txqi)

You're correct. Good catch, thanks!


Michał

[RFC/RFT] mac80211: implement fq_codel for software queuing

[Michal Kazior]

Since 11n aggregation become important to get the
best out of txops. However aggregation inherently
requires buffering and queuing. Once variable
medium conditions to different associated stations
is considered it became apparent that bufferbloat
can't be simply fought with qdiscs for wireless
drivers. 11ac with MU-MIMO makes the problem
worse because the bandwidth-delay product becomes
even greater.

This bases on codel5 and sch_fq_codel.c. It may
not be the Right Thing yet but it should at least
provide a framework for more improvements.

I guess dropping rate could factor in per-station
rate control info but I don't know how this should
exactly be done. HW rate control drivers would
need extra work to take advantage of this.

This obviously works only with drivers that use
wake_tx_queue op.

Note: This uses IFF_NO_QUEUE to get rid of qdiscs
for wireless drivers that use mac80211 and
implement wake_tx_queue op.

Moreover the current txq_limit and latency setting
might need tweaking. Either from userspace or be
dynamically scaled with regard to, e.g. number of
associated stations.

FWIW This already works nicely with ath10k's (not
yey merged) pull-push congestion control for
MU-MIMO as far as throughput is concerned.

Evaluating latency improvements is a little tricky
at this point if a driver is using more queue
layering and/or its firmware controls tx
scheduling - hence I don't have any solid data on
this. I'm open for suggestions though.

It might also be a good idea to do the following
in the future:

 - make generic tx scheduling which does some RR
   over per-sta-tid queues and dequeues bursts of
   packets to form a PPDU to fit into designated
   txop timeframe and bytelimit

   This could in theory be shared and used by
   ath9k and (future) mt76.

   Moreover tx scheduling could factor in rate
   control info and keep per-station number of
   queued packets at a sufficient low threshold to
   avoid queue buildup for slow stations. Emmanuel
   already did similar experiment for iwlwifi's
   station mode and got promising results.

 - make software queueing default internally in
   mac80211. This could help other drivers to get
   at least some benefit from mac80211 smarter
   queueing.

Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
---
 include/net/mac80211.h |  36 -
 net/mac80211/agg-tx.c  |   8 +-
 net/mac80211/codel.h   | 260 +++
 net/mac80211/codel_i.h |  89 +++
 net/mac80211/ieee80211_i.h |  27 +++-
 net/mac80211/iface.c   |  25 ++-
 net/mac80211/main.c|   9 +-
 net/mac80211/rx.c  |   2 +-
 net/mac80211/sta_info.c|  10 +-
 net/mac80211/sta_info.h|  27 
 net/mac80211/tx.c  | 370 -
 net/mac80211/util.c|  20 ++-
 12 files changed, 805 insertions(+), 78 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 6617516a276f..4667d2bad356 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -565,6 +565,18 @@ struct ieee80211_bss_conf {
struct ieee80211_p2p_noa_attr p2p_noa_attr;
 };
 
+typedef u64 codel_time_t;
+
+/*
+ * struct codel_params - contains codel parameters
+ * @interval:  initial drop rate
+ * @target: maximum persistent sojourn time
+ */
+struct codel_params {
+   codel_time_tinterval;
+   codel_time_ttarget;
+};
+
 /**
  * enum mac80211_tx_info_flags - flags to describe transmission 
information/status
  *
@@ -886,8 +898,18 @@ struct ieee80211_tx_info {
/* only needed before rate control */
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.
+*/
+   codel_time_t enqueue_time;
+   };
struct ieee80211_key_conf *hw_key;
u32 flags;
/* 4 bytes free */
@@ -2102,8 +2124,8 @@ enum ieee80211_hw_flags {
  * @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.
+ * @txq_cparams:

Re: [RFC v2] iwlwifi: pcie: transmit queue auto-sizing

[Michal Kazior]

On 5 February 2016 at 17:47, Dave Taht  wrote:
>> A bursted txop can be as big as 5-10ms. If you consider you want to
>> queue 5-10ms worth of data for *each* station at any given time you
>> obviously introduce a lot of lag. If you have 10 stations you might
>> end up with service period at 10*10ms = 100ms. This gets even worse if
>> you consider MU-MIMO because you need to do an expensive sounding
>> procedure before transmitting. So while SU aggregation can probably
>> still work reasonably well with shorter bursts (1-2ms) MU needs at
>> least 3ms to get *any* gain when compared to SU (which obviously means
>> you want more to actually make MU pay off).
>
> I am not sure where you get these numbers. Got a spreadsheet?

Here's a nice summary on some of it:

  http://chimera.labs.oreilly.com/books/123401739/ch03.html#figure-mac-ampdu

Even if your single A-MPDU can be shorter than a txop you can burst a
few of them if my understanding is correct.

The overhead associated with MU sounding is something I've been told
only. Apparently for MU to pay off you need fairly big bursts. This
implies that the more stations you have to service the less it makes
sense to attempt MU if you consider latency.


> Gradually reducing the maximum sized txop as a function of the number
> of stations makes sense. If you have 10 stations pending delivery and
> reduced the max txop to 1ms, you hurt bandwidth at that instant, but
> by offering more service to more stations, in less time, they will
> converge on a reasonable share of the bandwidth for each, faster[1].
> And I'm sure that the person videoconferencing on a link like that
> would appreciate getting some service inside of a 10ms interval,
> rather than a 100ms.
>
> yes, there's overhead, and that's not the right number, which would
> vary as to g,n,ac and successors.
>
> You will also get more opportunities to use mu-mimo with shorter
> bursts extant and more stations being regularly serviced.
>
> [1] https://www.youtube.com/watch?v=Rb-UnHDw02o at about 13:50

This is my thinking as well, at least for most common use cases.

If you try to optimize for throughput by introducing extra induced
latency you might end up not being able to use aggregation in practice
anyway because you won't be able to start up connections and ask for
enough data - or at least that's what my intuition tells me.

But, like I've mentioned, there's interest in making it possible to
maximize for throughput (regardless of latency). This surely makes
sense for synthetic UDP benchmarks. But does it make sense for any
real-world application? No idea.


>> The rule of thumb is the
>> longer you wait the bigger capacity you can get.
>
> This is not strictly true as the "fountain" of packets is regulated by
> acks on the other side of the link, and ramp up or down as a function
> of service time and loss.

Yes, if you consider real world cases, i.e. TCP, web traffic, etc.
then you're correct.


>> Apparently there's interest in maximizing throughput but it stands in
>> direct opposition of keeping the latency down so I've been thinking
>> how to satisfy both.
>>
>> The current approach ath10k is taking (patches in review [1][2]) is to
>> use mac80211 software queues for per-station queuing, exposing queue
>> state to firmware (it decides where frames should be dequeued from)
>> and making it possible to stop/wake per-station tx subqueue with fake
>> netdev queues. I'm starting to think this is not the right way though
>> because it's inherently hard to control latency and there's a huge
>> memory overhead associated with the fake netdev queues.
>
> What is this overhead?

E.g. if you want to be able to maximize throughput for 50 MU clients
you need to be able to queue, in theory, 50*200 (roughly) frames. This
translates to both huge memory usage and latency *and* renders
(fq_)codel qdisc rather.. moot.


> Applying things  like codel tend to dramatically shorten the amount of
> skbs extant...

> modern 802.11ac capable hardware has tons more
> memory...

I don't think it does. QCA988x is able to handle "only" 1424 tx
descriptors (IOW 1500-byte long MSDUs) in the driver-to-firmware tx
queue (it's a flat queue). QCA99x0 is able to handle 2500 if asked
politely.

This is still not enough to satisfy the insane "maximize the
capacity/throughput" expectations though.

You could actually argue it's too much from the bufferbloat problem
point of view anyway and Emmanuel's patch proves it is beneficial to
buffer less in driver depending on the sojourn packet time.


>> Also fq_codel
>> is a less effective with this kind of setup.
>
> fq_codel's principal problems with working with wifi are long and
> documented in the talk above.
>
>> My current thinking is that the entire problem should be solved via
>> (per-AC) qdiscs, e.g. fq_codel. I guess one could use
>> limit/target/interval/quantum knobs to tune it for higher latency of
>> aggregation-oriented Wi-Fi links where long service 

Re: [RFC v2] iwlwifi: pcie: transmit queue auto-sizing

[Michal Kazior]

On 4 February 2016 at 22:14, Ben Greear  wrote:
> On 02/04/2016 12:56 PM, Grumbach, Emmanuel wrote:
>> On 02/04/2016 10:46 PM, Ben Greear wrote:
>>> On 02/04/2016 12:16 PM, Emmanuel Grumbach wrote:

 As many (all?) WiFi devices, Intel WiFi devices have
 transmit queues which have 256 transmit descriptors
 each and each descriptor corresponds to an MPDU.
 This means that when it is full, the queue contains
 256 * ~1500 bytes to be transmitted (if we don't have
 A-MSDUs). The purpose of those queues is to have enough
 packets to be ready for transmission so that when the device
 gets an opportunity to transmit (TxOP), it can take as many
 packets as the spec allows and aggregate them into one
 A-MPDU or even several A-MPDUs if we are using bursts.
>>>
>>> I guess this is only really usable if you have exactly one
>>> peer connected (ie, in station mode)?
>>>
>>> Otherwise, you could have one slow peer and one fast one,
>>> and then I suspect this would not work so well?
>>
>>
>> Yes. I guess this one (big) limitation. I guess that what would happen
>> in this case is that the the latency would constantly jitter. But I also

Hmm.. You'd probably need to track per-station packet sojourn time as
well and make it possible to stop/wake queues per station.


>> noticed that I could reduce the transmit queue to 130 descriptors
>> (instead of 256) and still reach maximal throughput because we can
>> refill the queues quickly enough.
>> In iwlwifi, we have plans to have one queue for each peer.
>> This is under development. Not sure when it'll be ready. It also requires
>> firmware change obviously.
>
> Per-peer queues will probably be nice, especially if we can keep the
> buffer bloat manageable.

Per-station queues sound tricky if you consider bufferbloat.

To maximize use of airtime (i.e. txop) you need to send big
aggregates. Since aggregates are per station-tid to maximize
multi-station performance (in AP mode) you'll need to queue a lot of
frames, per each station, depending on the chosen tx rate.

A bursted txop can be as big as 5-10ms. If you consider you want to
queue 5-10ms worth of data for *each* station at any given time you
obviously introduce a lot of lag. If you have 10 stations you might
end up with service period at 10*10ms = 100ms. This gets even worse if
you consider MU-MIMO because you need to do an expensive sounding
procedure before transmitting. So while SU aggregation can probably
still work reasonably well with shorter bursts (1-2ms) MU needs at
least 3ms to get *any* gain when compared to SU (which obviously means
you want more to actually make MU pay off). The rule of thumb is the
longer you wait the bigger capacity you can get.

Apparently there's interest in maximizing throughput but it stands in
direct opposition of keeping the latency down so I've been thinking
how to satisfy both.

The current approach ath10k is taking (patches in review [1][2]) is to
use mac80211 software queues for per-station queuing, exposing queue
state to firmware (it decides where frames should be dequeued from)
and making it possible to stop/wake per-station tx subqueue with fake
netdev queues. I'm starting to think this is not the right way though
because it's inherently hard to control latency and there's a huge
memory overhead associated with the fake netdev queues. Also fq_codel
is a less effective with this kind of setup.

My current thinking is that the entire problem should be solved via
(per-AC) qdiscs, e.g. fq_codel. I guess one could use
limit/target/interval/quantum knobs to tune it for higher latency of
aggregation-oriented Wi-Fi links where long service time (think
100-200ms) is acceptable. However fq_codel is oblivious to how Wi-Fi
works in the first place, i.e. Wi-Fi gets better throughput if you
deliver bursts of packets destined to the same station. Moreover this
gets even more complicated with MU-MIMO where you may want to consider
spatial location (which influences signal quality when grouped) of
each station when you decide which set of stations you're going to
aggregate to in parallel. Since drivers have a finite tx ring this it
is important to deliver bursts that can actually be aggregated
efficiently. This means driver would need to be able to tell qdisc
about per-flow conditions to influence the RR scheme in some way
(assuming a qdiscs even understands flows; do we need a unified way of
talking about flows between qdiscs and drivers?).


[1]: https://www.spinics.net/lists/linux-wireless/msg146187.html
[2]: https://www.spinics.net/lists/linux-wireless/msg146512.html


>>> For reference, ath10k has around 1400 tx descriptors, though
>>> in practice not all are usable, and in stock firmware, I'm guessing
>>> the NIC will never be able to actually fill up it's tx descriptors
>>> and stop traffic.  Instead, it just allows the stack to try to
>>> TX, then drops the frame...
>>
>>
>> 1400 descriptors, ok... but they are not 

[RFC] mac80211: introduce netdev queue control for txqs

[Michal Kazior]

Until now all stations (notably in AP mode) would
share 4 AC queues exposed via netdev. This meant
that whenever a single station's AC became full
all other stations' AC were considered full and
stopped as well.

This was suboptimal and could lead to per station
traffic starvation and/or suboptimal Tx
aggregation performance.

The introduced queue control is performed by
mac80211 alone. Additional netdev queues are
created only if driver supports wake_tx_queue()
op.

First 4 netdev queues are used for unclassified
traffic (e.g. when there's no sta_info for given
RA) which is mostly multicast.

All other netdev queues (>=4) are used per
station. Each station gets 4 queues for each AC.
Whenever station's AC-related ieee80211_txqs get
full only that single station's netdev queue is
stopped allowing all other stations (either using
identical AC number or not) to continue submitting
traffic without interruption.

Current implementation supports up to 63
non-overlapping stations. Overlapping stations
will share their per AC netdev queues so whenever
any of overlapping stations' AC queue is stopped
others are stopped as well.

This was designed with MU-MIMO in mind but should
benefit regular Tx aggregation as well because
drivers will now have the ability to avoid
clogging up their internal (be it firmware or
hardware) queues with traffic to slow or
unresponsive stations needlessly.

Note: This can significantly increase memory usage
with, e.g. fq_codel qdisc even up to 20MB per
virtual interface.

Signed-off-by: Michal Kazior <michal.kaz...@tieto.com>
---
Note: It is recommended to have the following
patch applied in order to avoid conflicts:

  https://patchwork.kernel.org/patch/8134481/

I'm re-sending this with netdev in Cc as per
Johannes' suggestion.


 net/mac80211/cfg.c |  5 +++
 net/mac80211/ieee80211_i.h | 16 +
 net/mac80211/iface.c   | 21 ++-
 net/mac80211/sta_info.c| 86 --
 net/mac80211/sta_info.h| 11 ++
 net/mac80211/tx.c  | 74 ++-
 net/mac80211/util.c| 35 ---
 net/mac80211/wme.c |  5 +++
 8 files changed, 238 insertions(+), 15 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 66d22de93c8d..0428c5f68e8c 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1370,6 +1370,11 @@ static int ieee80211_change_station(struct wiphy *wiphy,
prev_4addr = true;
}
 
+   if (local->ops->wake_tx_queue) {
+   sta_info_ndev_free(sta->sdata, sta);
+   sta_info_ndev_init(vlansdata, sta);
+   }
+
sta->sdata = vlansdata;
ieee80211_check_fast_xmit(sta);
 
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index a96f8c0461f6..416bd12f14d2 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -47,6 +47,17 @@ struct ieee80211_local;
  * frame can be up to about 2 kB long. */
 #define TOTAL_MAX_TX_BUFFER 512
 
+/* The number of stations exposed via netdev queues. First 4 netdev queues are
+ * mapped to vif's ACs. Subsequent ones, in groups of 4, are ACs for stations.
+ * NUM_NDEV_STA + 1 station is wrapped around to 1st station. f.e. netdev queue
+ * 5 corresponds to AC1 of STA0, STA63, STA126, ... and queue 8 corresponds
+ * to AC0 of STA1, STA64, STA127, ...
+ *
+ * This is used only when driver implements wake_tx_queues() op.
+ */
+#define IEEE80211_NUM_NDEV_STA 63
+#define IEEE80211_NUM_NDEV_STA_Q (IEEE80211_NUM_NDEV_STA * IEEE80211_NUM_ACS)
+
 /* Required encryption head and tailroom */
 #define IEEE80211_ENCRYPT_HEADROOM 8
 #define IEEE80211_ENCRYPT_TAILROOM 18
@@ -852,7 +863,12 @@ struct ieee80211_sub_if_data {
bool control_port_no_encrypt;
int encrypt_headroom;
 
+   spinlock_t ndev_lock; /* protects access to ndev_sta_idr */
+   DECLARE_BITMAP(ndev_sta_q_stopped, IEEE80211_NUM_NDEV_STA_Q);
+   atomic_t ndev_sta_q_refs[IEEE80211_NUM_NDEV_STA_Q];
+   struct idr ndev_sta_idr;
atomic_t txqs_len[IEEE80211_NUM_ACS];
+
struct ieee80211_tx_queue_params tx_conf[IEEE80211_NUM_ACS];
struct mac80211_qos_map __rcu *qos_map;
 
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 453b4e741780..d3dae1ae5652 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1094,6 +1094,8 @@ static void ieee80211_teardown_sdata(struct 
ieee80211_sub_if_data *sdata)
 
if (ieee80211_vif_is_mesh(>vif))
mesh_rmc_free(sdata);
+
+   idr_destroy(>ndev_sta_idr);
 }
 
 static void ieee80211_uninit(struct net_device *dev)
@@ -1373,6 +1375,7 @@ static void ieee80211_setup_sdata(struct 
ieee80211_sub_if_data *sdata,
 {
static const u8 bssid_wildcard[ETH_ALEN] = {0xff, 0xff, 0xff,
0xff, 0xff, 0xff};
+   int

Re: pull-request: wireless-drivers-next 2015-05-21

[Michal Kazior]

On 21 May 2015 at 15:39, Kalle Valo kv...@codeaurora.org wrote:
 Hi Dave,

 here's a wireless-drivers pull request for 4.2. This time please pay
 extra attention to this pull as there are two problems:

 First of all as you can see the diffstat from git-pull-request in the
 end is just weird. I was long and hard trying to check everything and to
 my understanding all the merges look ok and I cannot explain the reason
 for the diffstat, but of course I might be missing something. Maybe
 git-request-pull is just buggy? At least with gitk everything looks to
 be ok and the patch list below also looks valid.

 Secondly there's a non-trivial conflict in
 drivers/net/wireless/ath/ath10k/mac.c which is due to removal of
 FIF_PROMISC_IN_BSS in commit df1404650c. You need to remove more code
 than just the obvious conflicts shown by git. In the end of this mail I
 added a git diff output after I fixed the conflict, hopefully that helps
 you to fix it. The main points are that you remove
 ath10k_mac_should_disable_promisc() and the last ath10k_monitor_recalc()
 call from ath10k_vdev_start_restart() along with the obvious conflict
 fixes git points out.

 There's also a patch from Michal which will also help to fix the
 resolution. Michal, please double check the resolution proposal below so
 that I didn't miss anything.

 https://patchwork.kernel.org/patch/6387631/

[...]

 diff --cc drivers/net/wireless/ath/ath10k/mac.c
 index fcd08b2f8d26,eaa0182e001d..
 --- a/drivers/net/wireless/ath/ath10k/mac.c
 +++ b/drivers/net/wireless/ath/ath10k/mac.c
 @@@ -766,9 -1031,68 +1031,48 @@@ static int ath10k_monitor_stop(struct a
 return 0;
   }

  -static bool ath10k_mac_should_disable_promisc(struct ath10k *ar)
  -{
  -  struct ath10k_vif *arvif;
  -
  -  if (!(ar-filter_flags  FIF_PROMISC_IN_BSS))
  -  return true;
  -
  -  if (!ar-num_started_vdevs)
  -  return false;
  -
  -  list_for_each_entry(arvif, ar-arvifs, list)
  -  if (arvif-vdev_type != WMI_VDEV_TYPE_AP)
  -  return false;
  -
  -  ath10k_dbg(ar, ATH10K_DBG_MAC,
  - mac disabling promiscuous mode because vdev is 
 started\n);
  -  return true;
  -}
  -
 + static bool ath10k_mac_monitor_vdev_is_needed(struct ath10k *ar)
 + {
 +   int num_ctx;
 +
 +   /* At least one chanctx is required to derive a channel to start
 +* monitor vdev on.
 +*/
 +   num_ctx = ath10k_mac_num_chanctxs(ar);
 +   if (num_ctx == 0)
 +   return false;
 +
 +   /* If there's already an existing special monitor interface then don't
 +* bother creating another monitor vdev.
 +*/
 +   if (ar-monitor_arvif)
 +   return false;
 +
 +   return ar-monitor ||
  - !ath10k_mac_should_disable_promisc(ar) ||
 +  test_bit(ATH10K_CAC_RUNNING, ar-dev_flags);
 + }
 +
 + static bool ath10k_mac_monitor_vdev_is_allowed(struct ath10k *ar)
 + {
 +   int num_ctx;
 +
 +   num_ctx = ath10k_mac_num_chanctxs(ar);
 +
 +   /* FIXME: Current interface combinations and cfg80211/mac80211 code
 +* shouldn't allow this but make sure to prevent handling the 
 following
 +* case anyway since multi-channel DFS hasn't been tested at all.
 +*/
 +   if (test_bit(ATH10K_CAC_RUNNING, ar-dev_flags)  num_ctx  1)
 +   return false;
 +
 +   return true;
 + }
 +
   static int ath10k_monitor_recalc(struct ath10k *ar)
   {
 -   bool should_start;
 +   bool needed;
 +   bool allowed;
 +   int ret;

 lockdep_assert_held(ar-conf_mutex);


Looks good to me.

There's still some code left which is unnecessary (see 2 last hunks on
patchwork) because due to FIF_PROMISC_IN_BSS removal entire commit
548462133d98 becomes obsolete. Since these 2 hunk leftovers don't
break anything functionally I guess this can be cleaned up in a follow
up patch after the merge. Just my 2cc.


 @@@ -871,12 -1231,46 +1211,46 @@@ static void ath10k_recalc_radar_detecti
 }
   }

 - static int ath10k_vdev_start_restart(struct ath10k_vif *arvif, bool restart)
 + static int ath10k_vdev_stop(struct ath10k_vif *arvif)
 + {
 +   struct ath10k *ar = arvif-ar;
 +   int ret;
 +
 +   lockdep_assert_held(ar-conf_mutex);
 +
 +   reinit_completion(ar-vdev_setup_done);
 +
 +   ret = ath10k_wmi_vdev_stop(ar, arvif-vdev_id);
 +   if (ret) {
 +   ath10k_warn(ar, failed to stop WMI vdev %i: %d\n,
 +   arvif-vdev_id, ret);
 +   return ret;
 +   }
 +
 +   ret = ath10k_vdev_setup_sync(ar);
 +   if (ret) {
 +   ath10k_warn(ar, failed to syncronise setup for vdev %i: 
 %d\n,
 +   arvif-vdev_id, ret);
 +   return ret;
 +   }
 +
 +   WARN_ON(ar-num_started_vdevs == 0);
 +
 +   if (ar-num_started_vdevs != 0) {
 +   ar-num_started_vdevs--;
 + 

Re: pull-request: wireless-drivers-next 2015-05-21

[Michal Kazior]

On 22 May 2015 at 14:27, Kalle Valo kv...@codeaurora.org wrote:
 Michal Kazior michal.kaz...@tieto.com writes:
 On 21 May 2015 at 15:39, Kalle Valo kv...@codeaurora.org wrote:
[...]
 + static int ath10k_vdev_start_restart(struct ath10k_vif *arvif,
 +const struct cfg80211_chan_def 
 *chandef,
 +bool restart)
   {
 struct ath10k *ar = arvif-ar;
 -   struct cfg80211_chan_def *chandef = ar-chandef;
 struct wmi_vdev_start_request_arg arg = {};
  -  int ret = 0, ret2;
  +  int ret = 0;

 lockdep_assert_held(ar-conf_mutex);


 Kalle, I'm not seeing this when I merge your pull tag on top of
 net-next/master. Am I missing something?

 You lost me, you are not seeing what?

Your merge resolution diff seems to include stuff regarding
ath10k_vdev_stop(). I didn't see it when I tried the merge and had
only 2 conflicts: the promisc code and at feature flags (XMIT_FAST).


 I think we have a misunderstanding

That's possible.


 here, let me explain how I created that diff:

 git clone --reference ~/linux-2.6/ 
 git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git
 cd net-next/
 git pull 
 git://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git 
 tags/wireless-drivers-next-for-davem-2015-05-21
 emacs drivers/net/wireless/ath/ath10k/mac.c
 git diff  resolution.txt

For what it's worth I did:
 git checkout net-next/master
 git merge wireless-drivers-next-for-davem-2015-05-21
 # fix conflicts
 git commit -a
 git show

Merge should essentially yield the same result as your pull.


Michał
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html