Skip to contentSkip to conversation

rc80211-pid: export human-readable target_pf value to debugfs

PATCH In review Stefano Brivio 3 participants 13 messages Dec 23, 2007 3 messages not in the archive mac80211

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

#1 lore raw

PATCH 1/7 rc80211-pid: export human-readable target_pf value to debugfs

Export the non-shifted target_pf value to debugfs, so that it's human-readable.

---
 rc80211_pid.h      |    2 +-
 rc80211_pid_algo.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
net/mac80211/rc80211_pid.h +1 -1
Index: wireless-2.6/net/mac80211/rc80211_pid.h
===================================================================
@@ -38,7 +38,7 @@
  * link quality is good, the controller will fail to adjust failed frames
  * percentage to the target. This is intentional.
  */
-#define RC_PID_TARGET_PF (11 << RC_PID_ARITH_SHIFT)
+#define RC_PID_TARGET_PF 11
 
 /* Rate behaviour normalization quantity over time. */
 #define RC_PID_NORM_OFFSET 3
net/mac80211/rc80211_pid_algo.c +1 -1
Index: wireless-2.6/net/mac80211/rc80211_pid_algo.c
===================================================================
@@ -210,7 +210,7 @@ static void rate_control_pid_sample(stru
 	rate_control_pid_normalize(pinfo, mode->num_rates);
 
 	/* Compute the proportional, integral and derivative errors. */
-	err_prop = pinfo->target - pf;
+	err_prop = (pinfo->target << RC_PID_ARITH_SHIFT) - pf;
 
 	err_avg = spinfo->err_avg_sc >> pinfo->smoothing_shift;
 	spinfo->err_avg_sc = spinfo->err_avg_sc - err_avg + err_prop;
Signature · 2 lines
Ciao
Stefano

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): 20071223131135.391cc0bb@morte

#4 lore raw

Hey,

I'm a little late on this and away from my development machine anyway, = but when reading my email, I noticed the following and thought this is = important enough to point out, even though I only have that crappy webm= ailer and the only kernel tree I can look at is wireless-2.6/everything=
on git.kernel.org

In rc80211_pid.h, you have the following (this from the wireless-2.6 tr= ee):

        /* Sampling period for measuring percentage of failed frames. *=
/
        #define RC_PID_INTERVAL (HZ / 8)

But rc80211_pid_algo.c says:

        period =3D (HZ * pinfo->sampling_period + 500) / 1000;
        if (!period)
                period =3D 1;

You see this is completely bogus. My original patch had:

        /* Sampling period for measuring percentage of failed frames in=
 0.001s. */
        #define RC_PID_INTERVAL 1000

You see why what you posted and John commited is bogus?

I'm getting a little fed up with this, you keep screwing up the code wi= th my name on it *sigh*

And again, I still don't think guarding against period =3D=3D 0 is the = right thing to do. As I explained privately in IRC, we should make sure=
the sample_interval is larger than HZ when setting the sample_interval= variable (once nl80211 is done). It doesn't make sense to do this che=
ck every time we execute the code when we can do it once in advance.

Sigh.

Mattias
--=20
Der GMX SmartSurfer hilft bis zu 70% Ihrer Onlinekosten zu sparen!=20 Ideal f=FCr Modem und ISDN: http://www.gmx.net/de/go/smartsurfer
-
To unsubscribe from this list: send the line "unsubscribe linux-wireles= s" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw

Re:

Quoted 19 lines · On Sun, 23 Dec 2007 13:19:51 +0100 "Mattias Nissler" <[email protected]>…

Hey,

I'm a little late on this and away from my development machine anyway, but when reading my email, I noticed the following and thought this is important enough to point out, even though I only have that crappy webmailer and the only kernel tree I can look at is wireless-2.6/everything on git.kernel.org

In rc80211_pid.h, you have the following (this from the wireless-2.6 tree):

        /* Sampling period for measuring percentage of failed frames. */
        #define RC_PID_INTERVAL (HZ / 8)

But rc80211_pid_algo.c says:

        period = (HZ * pinfo->sampling_period + 500) / 1000;
        if (!period)
                period = 1;
Quoted 6 lines

You see this is completely bogus. My original patch had:

        /* Sampling period for measuring percentage of failed frames in 0.001s. */
        #define RC_PID_INTERVAL 1000

You see why what you posted and John commited is bogus?

Indeed. I didn't notice about this because HZ == 1000 on my laptop. But I didn't even sign off that patch, I just added a From: on top of it, and changed this:

+#define RC_PID_INTERVAL (HZ / 1)
[yes, this is present in your original - as sent through IRC - patch]

to:

+#define RC_PID_INTERVAL (HZ / 8)
[and IIRC we discussed about this on IRC]

I'll post a fix for this.

Quoted 2 lines

I'm getting a little fed up with this, you keep screwing up the code with my name on it *sigh*

Please, could you show _exactly_ which patches with your name (as in,
From: you) you think I screwed up?

I don't think I screwed up anything yours. I would be very sorry otherwise.

Quoted 5 lines

And again, I still don't think guarding against period == 0 is the right thing to do. As I explained privately in IRC, we should make sure the sample_interval is larger than HZ when setting the sample_interval variable (once nl80211 is done). It doesn't make sense to do this check every time we execute the code when we can do it once in advance.

Exactly, once nl80211 is done. Now, what if the control interval gets changed after initialization? How often should we check whether it's larger than HZ? If you got another solution which would work together with the current debugfs interface, please post it.

Quoted 1 line

Sigh.

Please stop this. I think that this is:
1) not correct (but I don't care that much);
2) useless;
3) confusing.

Sorry again if I actually screwed up anything.

--
Ciao
Stefano

#6 lore raw

PATCH 2/7 rc80211-pid: add kerneldoc for tunable parameters

In reply to a message not in the archive

Add a kerneldoc description for parameters which are tunable through debugfs.

---
 rc80211_pid.h |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)
net/mac80211/rc80211_pid.h +24 -0
Index: wireless-2.6/net/mac80211/rc80211_pid.h
===================================================================
@@ -1,5 +1,6 @@
 /*
  * Copyright 2007, Mattias Nissler <[email protected]>
+ * Copyright 2007, Stefano Brivio <[email protected]>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -119,6 +120,29 @@ struct rc_pid_events_file_info {
 	unsigned int next_entry;
 };
 
+/**
+ * struct rc_pid_debugfs_entries - tunable parameters
+ *
+ * Algorithm parameters, tunable via debugfs.
+ * @dir: the debugfs directory for a specific phy
+ * @target: target percentage for failed frames
+ * @sampling_period: error sampling interval in milliseconds
+ * @coeff_p: absolute value of the proportional coefficient
+ * @coeff_i: absolute value of the integral coefficient
+ * @coeff_d: absolute value of the derivative coefficient
+ * @smoothing_shift: absolute value of the integral smoothing factor (i.e.
+ *	amount of smoothing introduced by the exponential moving average)
+ * @sharpen_factor: absolute value of the derivative sharpening factor (i.e.
+ *	amount of emphasis given to the derivative term after low activity
+ *	events)
+ * @sharpen_duration: duration of the sharpening effect after the detected low
+ *	activity event, relative to sampling_period
+ * @norm_offset: amount of normalization periodically performed on the learnt
+ *	rate behaviour values (lower means we should trust more what we learnt
+ *	about behaviour of rates, higher means we should trust more the natural
+ *	ordering of rates)
+ * @fast_start: if Y, push high rates right after initialization
+ */
 struct rc_pid_debugfs_entries {
 	struct dentry *dir;
 	struct dentry *target;
Signature · 2 lines
Ciao
Stefano
#7 lore raw

PATCH 3/7 rc80211-pid: simplify and fix shift_adjust

In reply to a message not in the archive

Simplify and fix rate_control_pid_shift_adjust(). A bug prevented correct mapping of sorted rates, and readability was seriously flawed.

---
 rc80211_pid_algo.c |   34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)
net/mac80211/rc80211_pid_algo.c +16 -18
Index: wireless-2.6/net/mac80211/rc80211_pid_algo.c
===================================================================
@@ -74,29 +74,27 @@ static int rate_control_pid_shift_adjust
 {
 	int i, j, k, tmp;
 
-	if (cur + adj < 0)
-		return 0;
-	if (cur + adj >= l)
-		return l - 1;
+	j = r[cur].rev_index;
+	i = j + adj;
 
-	i = r[cur + adj].rev_index;
+	if (i < 0)
+		return r[0].index;
+	if (i >= l - 1)
+		return r[l - 1].index;
 
-	j = r[cur].rev_index;
+	tmp = i;
 
 	if (adj < 0) {
-			tmp = i;
-			for (k = j; k >= i; k--)
-				if (r[k].diff <= r[j].diff)
-					tmp = k;
-			return r[tmp].index;
-	} else if (adj > 0) {
-			tmp = i;
-			for (k = i + 1; k + i < l; k++)
-				if (r[k].diff <= r[i].diff)
-					tmp = k;
-			return r[tmp].index;
+		for (k = j; k >= i; k--)
+			if (r[k].diff <= r[j].diff)
+				tmp = k;
+	} else {
+		for (k = i + 1; k + i < l; k++)
+			if (r[k].diff <= r[i].diff)
+				tmp = k;
 	}
-	return cur + adj;
+
+	return r[tmp].index;
 }
 
 static void rate_control_pid_adjust_rate(struct ieee80211_local *local,
Signature · 2 lines
Ciao
Stefano
#8 lore raw

PATCH 4/7 rc80211-pid: fix sta_info refcounting

In reply to a message not in the archive

Fix a bug which caused uncorrect refcounting of PHYs in mac80211. Thanks to Johannes Berg for spotting this out.

---
 rc80211_pid_algo.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
net/mac80211/rc80211_pid_algo.c +2 -1
Index: wireless-2.6/net/mac80211/rc80211_pid_algo.c
===================================================================
@@ -254,7 +254,7 @@ static void rate_control_pid_tx_status(v
 	/* Ignore all frames that were sent with a different rate than the rate
 	 * we currently advise mac80211 to use. */
 	if (status->control.rate != &local->oper_hw_mode->rates[sta->txrate])
-		return;
+		goto ignore;
 
 	spinfo = sta->rate_ctrl_priv;
 	spinfo->tx_num_xmit++;
@@ -295,6 +295,7 @@ static void rate_control_pid_tx_status(v
 	if (time_after(jiffies, spinfo->last_sample + period))
 		rate_control_pid_sample(pinfo, local, sta);
 
+ignore:
 	sta_info_put(sta);
 }
 
Signature · 2 lines
Ciao
Stefano
maintainer Acked-by on v1 4/7
#9 lore raw

Re: [PATCH 4/7] rc80211-pid: fix sta_info refcounting

Quoted 5 lines · Stefano Brivio

Fix a bug which caused uncorrect refcounting of PHYs in mac80211. Thanks to Johannes Berg for spotting this out.

Yup, this looks like the correct fix.

Quoted 26 lines
---
 rc80211_pid_algo.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: wireless-2.6/net/mac80211/rc80211_pid_algo.c
===================================================================

net/mac80211/rc80211_pid_algo.c +2 -1
@@ -254,7 +254,7 @@ static void rate_control_pid_tx_status(v
 	/* Ignore all frames that were sent with a different rate than the rate
 	 * we currently advise mac80211 to use. */
 	if (status->control.rate != &local->oper_hw_mode->rates[sta->txrate])
-		return;
+		goto ignore;
 
 	spinfo = sta->rate_ctrl_priv;
 	spinfo->tx_num_xmit++;
@@ -295,6 +295,7 @@ static void rate_control_pid_tx_status(v
 	if (time_after(jiffies, spinfo->last_sample + period))
 		rate_control_pid_sample(pinfo, local, sta);
 
+ignore:
 	sta_info_put(sta);
 }
 
#10 lore raw

PATCH 5/7 rc80211-pid: pf_target tuning

In reply to a message not in the archive

Set a better value for percentage target for failed frames. The previous value slowed down too much rate increases in case of permanently low activity. While at it, increase readability.

---
 rc80211_pid.h |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)
net/mac80211/rc80211_pid.h +13 -13
Index: wireless-2.6/net/mac80211/rc80211_pid.h
===================================================================
@@ -11,41 +11,41 @@
 #define RC80211_PID_H
 
 /* Sampling period for measuring percentage of failed frames. */
-#define RC_PID_INTERVAL (HZ / 8)
+#define RC_PID_INTERVAL			(HZ / 8)
 
 /* Exponential averaging smoothness (used for I part of PID controller) */
-#define RC_PID_SMOOTHING_SHIFT 3
-#define RC_PID_SMOOTHING (1 << RC_PID_SMOOTHING_SHIFT)
+#define RC_PID_SMOOTHING_SHIFT		3
+#define RC_PID_SMOOTHING		(1 << RC_PID_SMOOTHING_SHIFT)
 
 /* Sharpening factor (used for D part of PID controller) */
-#define RC_PID_SHARPENING_FACTOR 0
-#define RC_PID_SHARPENING_DURATION 0
+#define RC_PID_SHARPENING_FACTOR	0
+#define RC_PID_SHARPENING_DURATION	0
 
 /* Fixed point arithmetic shifting amount. */
-#define RC_PID_ARITH_SHIFT 8
+#define RC_PID_ARITH_SHIFT		8
 
 /* Fixed point arithmetic factor. */
-#define RC_PID_ARITH_FACTOR (1 << RC_PID_ARITH_SHIFT)
+#define RC_PID_ARITH_FACTOR		(1 << RC_PID_ARITH_SHIFT)
 
 /* Proportional PID component coefficient. */
-#define RC_PID_COEFF_P 15
+#define RC_PID_COEFF_P			15
 /* Integral PID component coefficient. */
-#define RC_PID_COEFF_I 9
+#define RC_PID_COEFF_I			9
 /* Derivative PID component coefficient. */
-#define RC_PID_COEFF_D 15
+#define RC_PID_COEFF_D			15
 
 /* Target failed frames rate for the PID controller. NB: This effectively gives
  * maximum failed frames percentage we're willing to accept. If the wireless
  * link quality is good, the controller will fail to adjust failed frames
  * percentage to the target. This is intentional.
  */
-#define RC_PID_TARGET_PF 11
+#define RC_PID_TARGET_PF		14
 
 /* Rate behaviour normalization quantity over time. */
-#define RC_PID_NORM_OFFSET 3
+#define RC_PID_NORM_OFFSET		3
 
 /* Push high rates right after loading. */
-#define RC_PID_FAST_START 0
+#define RC_PID_FAST_START		0
 
 /* Arithmetic right shift for positive and negative values for ISO C. */
 #define RC_PID_DO_ARITH_RIGHT_SHIFT(x, y) \
Signature · 2 lines
Ciao
Stefano
#11 lore raw

PATCH 6/7 rc80211-pid: add MAINTAINERS entry

In reply to a message not in the archive

Add an entry in MAINTAINERS for rc80211-pid.

---
 MAINTAINERS |   10 ++++++++++
 1 file changed, 10 insertions(+)
MAINTAINERS +10 -0
Index: wireless-2.6/MAINTAINERS
===================================================================
@@ -2495,6 +2495,16 @@ W:	http://linuxwireless.org/
 T:	git kernel.org:/pub/scm/linux/kernel/git/linville/wireless-2.6.git
 S:	Maintained
 
+MAC80211 PID RATE CONTROL
+P:	Stefano Brivio
+M:	[email protected]
+P:	Mattias Nissler
+M:	[email protected]
+L:	[email protected]
+W:	http://linuxwireless.org/en/developers/Documentation/mac80211/RateControl/PID
+T:	git kernel.org:/pub/scm/linux/kernel/git/linville/wireless-2.6.git
+S:	Maintained
+
 MACVLAN DRIVER
 P:	Patrick McHardy
 M:	[email protected]
Signature · 2 lines
Ciao
Stefano
#12 lore raw

RFC PATCH 7/7 mac80211: fix sta_info locking

In reply to a message not in the archive

While tinkering with a sta_info refcounting bug in rc80211-pid algorithm, I discovered that calling sta_info_get() and then sta_info_put() right after would cause a kernel panic on my uniprocessor, preemptible kernel. I couldn't set up netconsole, however, most of the trace is reported below (my camera did its best, as the trace wouldn't fit on the screen and I couldn't scroll, so wasn't able to see the first part with naked eyes :).

EIP at delay_tsc+0x22/0x50
[couldn't read the EBX and such, but I guess you won't care]

panic+0xf9/0x100
die+0x1e0/0x1f0
do_page_fault+0x357/0x640
autoremove_wake_function+0x1b/0x50
__wake_up_common+0x3e/0x70
do_page_fault+0x0/0x640
error_code+0x6a/0x70
sta_info_get+0x3a/0x60 [mac80211]
__ieee80211_rx+0x290/0x1830 [mac80211]
skb_queue_tail+0x3b/0x70
ieee80211_rx_irqsafe+0x30/0x80 [mac80211]
ssb_pci_write32+0x22/0x70 [ssb]
ieee80211_tasklet_handler+0xaf/0xe0 [mac80211]
hrtimer_run_queues+0xf6/0x1a0
process_timeout+0x0/0x10
tasklet_action+0x27/0x60
__do_softirq+0x54/0xb0
do_softirq+0x7b/0xe0
handle_level_irq+0x0/0x110
irq_exit+0x30/0x40
do_IRQ+0x83/0xd0
common_interrupt+0x23/0x20

[...]

So I guessed that locking was lacking somewhere. The following patch fixes the issue for me, but I'm not sure at all that it's the right fix. Thanks.

---

net/mac80211/sta_info.c +3 -0
Index: wireless-2.6/net/mac80211/sta_info.c
===================================================================
@@ -105,6 +105,7 @@ static void sta_info_release(struct kref
 	struct ieee80211_local *local = sta->local;
 	struct sk_buff *skb;
 
+	write_lock_bh(&local->sta_lock);
 	/* free sta structure; it has already been removed from
 	 * hash table etc. external structures. Make sure that all
 	 * buffered frames are release (one might have been added
@@ -118,6 +119,8 @@ static void sta_info_release(struct kref
 	}
 	rate_control_free_sta(sta->rate_ctrl, sta->rate_ctrl_priv);
 	rate_control_put(sta->rate_ctrl);
+	write_unlock_bh(&local->sta_lock);
+
 	kfree(sta);
 }
 
Signature · 2 lines
Ciao
Stefano
maintainer
#13 lore raw

Re: [RFC PATCH 7/7] mac80211: fix sta_info locking

Quoted 3 lines

EIP at delay_tsc+0x22/0x50
[couldn't read the EBX and such, but I guess you won't care]

It'd be good to know actually, along with disassembly.

Quoted 2 lines

So I guessed that locking was lacking somewhere. The following patch fixes the issue for me, but I'm not sure at all that it's the right fix. Thanks.

Definitely not. If you look at the code in question, it has nothing at all that needs locking. It's not accessing any shared structures at all.

johannes

#14 lore raw

Re: [RFC PATCH 7/7] mac80211: fix sta_info locking

Quoted 10 lines · On Sun, 23 Dec 2007 08:38:56 +0100 Johannes Berg <[email protected]>…
Quoted 2 lines

EIP at delay_tsc+0x22/0x50
[couldn't read the EBX and such, but I guess you won't care]

It'd be good to know actually, along with disassembly.

Quoted 2 lines

So I guessed that locking was lacking somewhere. The following patch fixes the issue for me, but I'm not sure at all that it's the right fix. Thanks.

Definitely not. If you look at the code in question, it has nothing at all that needs locking. It's not accessing any shared structures at all.

I now removed the lock, rebuilt my quilt tree, rebuilt mac80211 and I can't reproduce it. I think that could have been my fault, I should have did something wrong such as calling sta_info_put() twice or somesuch. Sorry for the noise. Will poke you again if I happen to reproduce this.

--
Ciao
Stefano

#15 lore raw

Re: [RFC PATCH 7/7] mac80211: fix sta_info locking

Quoted 2 lines · On Sun, 23 Dec 2007 11:18:06 +0100 Stefano Brivio <[email protected]>…

I now removed the lock, rebuilt my quilt tree, rebuilt mac80211 and I can't reproduce it. I think that could have been my fault, I should have did

							  ^^^^ must, I meant.
Signature · 2 lines
Ciao
Stefano