[PATCH 0/3] Add memory limits to fq.h and mac80211 TXQ

[PATCH 0/3] Add memory limits to fq.h and mac80211 TXQ

[Toke Høiland-Jørgensen]

This is a series of small patches to avoid OOM conditions on small
wireless devices with the mac80211 intermediate TXQ structure. The
current default limit in fq.h translates to up to 16 Mbytes of memory
usage, which can be fatal to a device with 32 MBytes of total RAM.

Rather than just change the fq_limit, this ports the memory limit
mechanism from the fq_codel qdisc. The second patch in the series just
adds the new fields to the mac80211 'aqm' debugfs file.

The third patch changes mac80211 to set a lower memory limit for non-VHT
devices. The assumption is that (a) for 802.11n and lower 4 Mbytes of
total queue (2048 packets, 64 max-size aggregates) is plenty, and so it
is safe to simply limit the queue size. And (b) that VHT-capable devices
are usually installed in systems equipped with more system memory.

Toke Høiland-Jørgensen (3):
  fq.h: Port memory limit mechanism from fq_codel
  mac80211: Export fq memory limit information in debugfs
  mac80211: Set lower memory limit for non-VHT devices

 include/net/fq.h       |  3 +++
 include/net/fq_impl.h  |  7 ++++++-
 net/mac80211/debugfs.c |  8 ++++++++
 net/mac80211/tx.c      | 18 ++++++++++++++++++
 4 files changed, 35 insertions(+), 1 deletion(-)

-- 
2.9.3

base-commit: fb2a3d5c7c85cb6e8bc88192be919b4ef8d6e630

[PATCH 1/3] fq.h: Port memory limit mechanism from fq_codel

[Toke Høiland-Jørgensen]

The reusable fairness queueing implementation (fq.h) lacks the memory
usage limit that the fq_codel qdisc has. This means that small
devices (e.g. WiFi routers) can run out of memory when flooded with a
large number of packets. This ports the memory limit feature from
fq_codel to fq.h.

Signed-off-by: Toke Høiland-Jørgensen <toke-LJ9M9ZcSy1A@public.gmane.org>
---
 include/net/fq.h      | 3 +++
 include/net/fq_impl.h | 7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/net/fq.h b/include/net/fq.h
index 268b490..6d8521a 100644
--- a/include/net/fq.h
+++ b/include/net/fq.h
@@ -72,9 +72,12 @@ struct fq {
 	u32 flows_cnt;
 	u32 perturbation;
 	u32 limit;
+	u32 memory_limit;
+	u32 memory_usage;
 	u32 quantum;
 	u32 backlog;
 	u32 overlimit;
+	u32 overmemory;
 	u32 collisions;
 };
 
diff --git a/include/net/fq_impl.h b/include/net/fq_impl.h
index 163f3ed..4e6131c 100644
--- a/include/net/fq_impl.h
+++ b/include/net/fq_impl.h
@@ -29,6 +29,7 @@ static struct sk_buff *fq_flow_dequeue(struct fq *fq,
 	tin->backlog_packets--;
 	flow->backlog -= skb->len;
 	fq->backlog--;
+	fq->memory_usage -= skb->truesize;
 
 	if (flow->backlog == 0) {
 		list_del_init(&flow->backlogchain);
@@ -154,6 +155,7 @@ static void fq_tin_enqueue(struct fq *fq,
 	flow->backlog += skb->len;
 	tin->backlog_bytes += skb->len;
 	tin->backlog_packets++;
+	fq->memory_usage += skb->truesize;
 	fq->backlog++;
 
 	fq_recalc_backlog(fq, tin, flow);
@@ -166,7 +168,7 @@ static void fq_tin_enqueue(struct fq *fq,
 
 	__skb_queue_tail(&flow->queue, skb);
 
-	if (fq->backlog > fq->limit) {
+	if (fq->backlog > fq->limit || fq->memory_usage > fq->memory_limit) {
 		flow = list_first_entry_or_null(&fq->backlogs,
 						struct fq_flow,
 						backlogchain);
@@ -181,6 +183,8 @@ static void fq_tin_enqueue(struct fq *fq,
 
 		flow->tin->overlimit++;
 		fq->overlimit++;
+		if (fq->memory_usage > fq->memory_limit)
+			fq->overmemory++;
 	}
 }
 
@@ -251,6 +255,7 @@ static int fq_init(struct fq *fq, int flows_cnt)
 	fq->perturbation = prandom_u32();
 	fq->quantum = 300;
 	fq->limit = 8192;
+	fq->memory_limit = 16 << 20; /* 16 MBytes */
 
 	fq->flows = kcalloc(fq->flows_cnt, sizeof(fq->flows[0]), GFP_KERNEL);
 	if (!fq->flows)
-- 
2.9.3

[PATCH 2/3] mac80211: Export fq memory limit information in debugfs

[Toke Høiland-Jørgensen]

Add memory limit, usage and overlimit counter to per-PHY 'aqm' debugfs
file.

Signed-off-by: Toke Høiland-Jørgensen <toke-LJ9M9ZcSy1A@public.gmane.org>
---
 net/mac80211/debugfs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 8ca62b6..f56e2f4 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -89,13 +89,19 @@ static ssize_t aqm_read(struct file *file,
 			"R fq_flows_cnt %u\n"
 			"R fq_backlog %u\n"
 			"R fq_overlimit %u\n"
+			"R fq_overmemory %u\n"
 			"R fq_collisions %u\n"
+			"R fq_memory_usage %u\n"
+			"RW fq_memory_limit %u\n"
 			"RW fq_limit %u\n"
 			"RW fq_quantum %u\n",
 			fq->flows_cnt,
 			fq->backlog,
+			fq->overmemory,
 			fq->overlimit,
 			fq->collisions,
+			fq->memory_usage,
+			fq->memory_limit,
 			fq->limit,
 			fq->quantum);
 
@@ -128,6 +134,8 @@ static ssize_t aqm_write(struct file *file,
 
 	if (sscanf(buf, "fq_limit %u", &local->fq.limit) == 1)
 		return count;
+	else if (sscanf(buf, "fq_memory_limit %u", &local->fq.memory_limit) == 1)
+		return count;
 	else if (sscanf(buf, "fq_quantum %u", &local->fq.quantum) == 1)
 		return count;
 
-- 
2.9.3

[PATCH 3/3] mac80211: Set lower memory limit for non-VHT devices

[Toke Høiland-Jørgensen]

Small devices can run out of memory from queueing too many packets. If
VHT is not supported by the PHY, having more than 4 MBytes of total
queue in the TXQ intermediate queues is not needed, and so we can safely
limit the memory usage in these cases and avoid OOM.

Signed-off-by: Toke Høiland-Jørgensen <toke-LJ9M9ZcSy1A@public.gmane.org>
---
 net/mac80211/tx.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1ff08be..82f41fc 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1434,6 +1434,8 @@ int ieee80211_txq_setup_flows(struct ieee80211_local *local)
 	struct fq *fq = &local->fq;
 	int ret;
 	int i;
+	bool supp_vht = false;
+	enum nl80211_band band;
 
 	if (!local->ops->wake_tx_queue)
 		return 0;
@@ -1442,6 +1444,22 @@ int ieee80211_txq_setup_flows(struct ieee80211_local *local)
 	if (ret)
 		return ret;
 
+	/*
+	 * If the hardware doesn't support VHT, it is safe to limit the maximum
+	 * queue size. 4 Mbytes is 64 max-size aggregates in 802.11n.
+	 */
+	for (band = 0; band < NUM_NL80211_BANDS; band++) {
+		struct ieee80211_supported_band *sband;
+
+		sband = local->hw.wiphy->bands[band];
+		if (!sband)
+			continue;
+
+		supp_vht = supp_vht || sband->vht_cap.vht_supported;
+	}
+	if (!supp_vht)
+		fq->memory_limit = 4 << 20; /* 4 Mbytes */
+
 	codel_params_init(&local->cparams);
 	local->cparams.interval = MS2TIME(100);
 	local->cparams.target = MS2TIME(20);
-- 
2.9.3

Re: [PATCH 3/3] mac80211: Set lower memory limit for non-VHT devices

[Johannes Berg]

On Fri, 2016-09-23 at 21:59 +0200, Toke Høiland-Jørgensen wrote:
> Small devices can run out of memory from queueing too many packets.
> If VHT is not supported by the PHY, having more than 4 MBytes of
> total queue in the TXQ intermediate queues is not needed, and so we
> can safely limit the memory usage in these cases and avoid OOM.

I kinda see the logic here - we really don't need to queue as much if
we can't possibly transmit it out quickly - but it seems to me we
should also throw in some kind of limit that's relative to the amount
of memory you have on the system?

I've applied these anyway though. I just don't like your assumption (b)
much as the rationale for it.

johannes

Re: [PATCH 3/3] mac80211: Set lower memory limit for non-VHT devices

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

> On Fri, 2016-09-23 at 21:59 +0200, Toke Høiland-Jørgensen wrote:
>> Small devices can run out of memory from queueing too many packets.
>> If VHT is not supported by the PHY, having more than 4 MBytes of
>> total queue in the TXQ intermediate queues is not needed, and so we
>> can safely limit the memory usage in these cases and avoid OOM.
>
> I kinda see the logic here - we really don't need to queue as much if
> we can't possibly transmit it out quickly - but it seems to me we
> should also throw in some kind of limit that's relative to the amount
> of memory you have on the system?

Yes, ideally. That goes for FQ-CoDel as well, BTW. LEDE currently
carries a patch for that which just changes the hard-coded default to
another hard-coded default. Not sure how to get a good value to use,
though; and deciding on how large a fraction of memory to use for
packets starts smelling an awful lot like setting policy in the kernel,
doesn't it?

> I've applied these anyway though. I just don't like your assumption (b)
> much as the rationale for it.

Right, thanks. I'll come up with a better rationale next time ;)

-Toke

Re: [PATCH 3/3] mac80211: Set lower memory limit for non-VHT devices

[Johannes Berg]

> > I kinda see the logic here - we really don't need to queue as much
> > if we can't possibly transmit it out quickly - but it seems to me
> > we should also throw in some kind of limit that's relative to the
> > amount of memory you have on the system?
> 
> Yes, ideally. That goes for FQ-CoDel as well, BTW. LEDE currently
> carries a patch for that which just changes the hard-coded default to
> another hard-coded default. Not sure how to get a good value to use,
> though; and deciding on how large a fraction of memory to use for
> packets starts smelling an awful lot like setting policy in the
> kernel, doesn't it?

Yeah, I agree it does seem awkward.

Perhaps we should instead pick a low limit and let users change it more
easily (i.e. not debugfs)? I don't know a good answer to this either.

johannes

Re: [PATCH 3/3] mac80211: Set lower memory limit for non-VHT devices

[Toke Høiland-Jørgensen]

Johannes Berg <johannes@sipsolutions.net> writes:

>> > I kinda see the logic here - we really don't need to queue as much
>> > if we can't possibly transmit it out quickly - but it seems to me
>> > we should also throw in some kind of limit that's relative to the
>> > amount of memory you have on the system?
>> 
>> Yes, ideally. That goes for FQ-CoDel as well, BTW. LEDE currently
>> carries a patch for that which just changes the hard-coded default to
>> another hard-coded default. Not sure how to get a good value to use,
>> though; and deciding on how large a fraction of memory to use for
>> packets starts smelling an awful lot like setting policy in the
>> kernel, doesn't it?
>
> Yeah, I agree it does seem awkward.
>
> Perhaps we should instead pick a low limit and let users change it
> more easily (i.e. not debugfs)? I don't know a good answer to this
> either.

Hmm, I'll talk it over with some of the LEDE people who are more used to
dealing with these sorts of memory-constrained devices than I am. Will
send a patch if we come up with a good solution :)

-Toke