[PATCH] ath9k: Use minstrel rate control by default

[PATCH] ath9k: Use minstrel rate control by default

[Sujith Manoharan]

From: Sujith Manoharan <c_manoha@qca.qualcomm.com>

The ath9k rate control algorithm has various architectural
issues that make it a poor fit in scenarios like congested
environments etc.

An example: https://bugzilla.redhat.com/show_bug.cgi?id=927191

Change the default to minstrel which is more robust in such cases.
The ath9k RC code is left in the driver for now, maybe it can
be removed altogether later on.

Cc: stable@vger.kernel.org
Cc: Jouni Malinen <jouni@qca.qualcomm.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath9k/Kconfig  | 7 ++++---
 drivers/net/wireless/ath/ath9k/Makefile | 2 +-
 drivers/net/wireless/ath/ath9k/init.c   | 2 +-
 drivers/net/wireless/ath/ath9k/rc.h     | 2 +-
 4 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/Kconfig b/drivers/net/wireless/ath/ath9k/Kconfig
index 3b07851..7526580 100644
--- a/drivers/net/wireless/ath/ath9k/Kconfig
+++ b/drivers/net/wireless/ath/ath9k/Kconfig
@@ -84,13 +84,14 @@ config ATH9K_DFS_CERTIFIED
 	  developed. At this point enabling this option won't do anything
 	  except increase code size.
 
-config ATH9K_RATE_CONTROL
+config ATH9K_LEGACY_RATE_CONTROL
 	bool "Atheros ath9k rate control"
 	depends on ATH9K
-	default y
+	default n
 	---help---
 	  Say Y, if you want to use the ath9k specific rate control
-	  module instead of minstrel_ht.
+	  module instead of minstrel_ht. Be warned that there are various
+	  issues with the ath9k RC and minstrel is a more robust algorithm.
 
 config ATH9K_HTC
        tristate "Atheros HTC based wireless cards support"
diff --git a/drivers/net/wireless/ath/ath9k/Makefile b/drivers/net/wireless/ath/ath9k/Makefile
index 2ad8f94..75ee9e7 100644
--- a/drivers/net/wireless/ath/ath9k/Makefile
+++ b/drivers/net/wireless/ath/ath9k/Makefile
@@ -8,7 +8,7 @@ ath9k-y +=	beacon.o \
 		antenna.o
 
 ath9k-$(CONFIG_ATH9K_BTCOEX_SUPPORT) += mci.o
-ath9k-$(CONFIG_ATH9K_RATE_CONTROL) += rc.o
+ath9k-$(CONFIG_ATH9K_LEGACY_RATE_CONTROL) += rc.o
 ath9k-$(CONFIG_ATH9K_PCI) += pci.o
 ath9k-$(CONFIG_ATH9K_AHB) += ahb.o
 ath9k-$(CONFIG_ATH9K_DEBUGFS) += debug.o
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index db81acc..b8a3a0e 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -830,7 +830,7 @@ void ath9k_set_hw_capab(struct ath_softc *sc, struct ieee80211_hw *hw)
 	sc->ant_rx = hw->wiphy->available_antennas_rx;
 	sc->ant_tx = hw->wiphy->available_antennas_tx;
 
-#ifdef CONFIG_ATH9K_RATE_CONTROL
+#ifdef CONFIG_ATH9K_LEGACY_RATE_CONTROL
 	hw->rate_control_algorithm = "ath9k_rate_control";
 #endif
 
diff --git a/drivers/net/wireless/ath/ath9k/rc.h b/drivers/net/wireless/ath/ath9k/rc.h
index 267dbfc..b9a8738 100644
--- a/drivers/net/wireless/ath/ath9k/rc.h
+++ b/drivers/net/wireless/ath/ath9k/rc.h
@@ -231,7 +231,7 @@ static inline void ath_debug_stat_retries(struct ath_rate_priv *rc, int rix,
 }
 #endif
 
-#ifdef CONFIG_ATH9K_RATE_CONTROL
+#ifdef CONFIG_ATH9K_LEGACY_RATE_CONTROL
 int ath_rate_control_register(void);
 void ath_rate_control_unregister(void);
 #else
-- 
1.8.3

Re: [PATCH] ath9k: Use minstrel rate control by default

[Linus Torvalds]

On Thu, Jun 6, 2013 at 12:10 PM, Sujith Manoharan <sujith@msujith.org> wrote:
>
> Change the default to minstrel which is more robust in such cases.

Ack. Background for John: the renaming means that people who had the
old broken default won't just silently continue to see their "default
y" from their old defconfigs.

So now you have to explicitly enable this by hand to get the fragile
legacy rate control.

            Linus

Re: [PATCH] ath9k: Use minstrel rate control by default

[Ben Greear]

On 06/05/2013 08:10 PM, Sujith Manoharan wrote:
> From: Sujith Manoharan <c_manoha@qca.qualcomm.com>
>
> The ath9k rate control algorithm has various architectural
> issues that make it a poor fit in scenarios like congested
> environments etc.
>
> An example: https://bugzilla.redhat.com/show_bug.cgi?id=927191
>
> Change the default to minstrel which is more robust in such cases.
> The ath9k RC code is left in the driver for now, maybe it can
> be removed altogether later on.
>
> Cc: stable@vger.kernel.org
> Cc: Jouni Malinen <jouni@qca.qualcomm.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
> ---
>   drivers/net/wireless/ath/ath9k/Kconfig  | 7 ++++---
>   drivers/net/wireless/ath/ath9k/Makefile | 2 +-
>   drivers/net/wireless/ath/ath9k/init.c   | 2 +-
>   drivers/net/wireless/ath/ath9k/rc.h     | 2 +-
>   4 files changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/Kconfig b/drivers/net/wireless/ath/ath9k/Kconfig
> index 3b07851..7526580 100644
> --- a/drivers/net/wireless/ath/ath9k/Kconfig
> +++ b/drivers/net/wireless/ath/ath9k/Kconfig
> @@ -84,13 +84,14 @@ config ATH9K_DFS_CERTIFIED
>   	  developed. At this point enabling this option won't do anything
>   	  except increase code size.
>
> -config ATH9K_RATE_CONTROL
> +config ATH9K_LEGACY_RATE_CONTROL
>   	bool "Atheros ath9k rate control"
>   	depends on ATH9K
> -	default y
> +	default n
>   	---help---
>   	  Say Y, if you want to use the ath9k specific rate control
> -	  module instead of minstrel_ht.
> +	  module instead of minstrel_ht. Be warned that there are various
> +	  issues with the ath9k RC and minstrel is a more robust algorithm.
>
>   config ATH9K_HTC
>          tristate "Atheros HTC based wireless cards support"
> diff --git a/drivers/net/wireless/ath/ath9k/Makefile b/drivers/net/wireless/ath/ath9k/Makefile
> index 2ad8f94..75ee9e7 100644
> --- a/drivers/net/wireless/ath/ath9k/Makefile
> +++ b/drivers/net/wireless/ath/ath9k/Makefile
> @@ -8,7 +8,7 @@ ath9k-y +=	beacon.o \
>   		antenna.o
>
>   ath9k-$(CONFIG_ATH9K_BTCOEX_SUPPORT) += mci.o
> -ath9k-$(CONFIG_ATH9K_RATE_CONTROL) += rc.o
> +ath9k-$(CONFIG_ATH9K_LEGACY_RATE_CONTROL) += rc.o
>   ath9k-$(CONFIG_ATH9K_PCI) += pci.o
>   ath9k-$(CONFIG_ATH9K_AHB) += ahb.o
>   ath9k-$(CONFIG_ATH9K_DEBUGFS) += debug.o
> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
> index db81acc..b8a3a0e 100644
> --- a/drivers/net/wireless/ath/ath9k/init.c
> +++ b/drivers/net/wireless/ath/ath9k/init.c
> @@ -830,7 +830,7 @@ void ath9k_set_hw_capab(struct ath_softc *sc, struct ieee80211_hw *hw)
>   	sc->ant_rx = hw->wiphy->available_antennas_rx;
>   	sc->ant_tx = hw->wiphy->available_antennas_tx;
>
> -#ifdef CONFIG_ATH9K_RATE_CONTROL
> +#ifdef CONFIG_ATH9K_LEGACY_RATE_CONTROL
>   	hw->rate_control_algorithm = "ath9k_rate_control";
>   #endif

If you will just delete that code above, then you can compile in both ath9k and minstrel rate
controls and select at run-time by twiddling the mac80211 rate control module option.

Last I tested there were still advantages to ath9k rate control in our environment,
(including when running through an attenuator and over-the-air),
so I think it should be left in to keep minstrel honest if nothing else.

Thanks,
Ben

>
> diff --git a/drivers/net/wireless/ath/ath9k/rc.h b/drivers/net/wireless/ath/ath9k/rc.h
> index 267dbfc..b9a8738 100644
> --- a/drivers/net/wireless/ath/ath9k/rc.h
> +++ b/drivers/net/wireless/ath/ath9k/rc.h
> @@ -231,7 +231,7 @@ static inline void ath_debug_stat_retries(struct ath_rate_priv *rc, int rix,
>   }
>   #endif
>
> -#ifdef CONFIG_ATH9K_RATE_CONTROL
> +#ifdef CONFIG_ATH9K_LEGACY_RATE_CONTROL
>   int ath_rate_control_register(void);
>   void ath_rate_control_unregister(void);
>   #else
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] ath9k: Use minstrel rate control by default

[Linus Torvalds]

On Thu, Jun 6, 2013 at 12:49 PM, Ben Greear <greearb@candelatech.com> wrote:
>
> Last I tested there were still advantages to ath9k rate control in our
> environment,
> (including when running through an attenuator and over-the-air),
> so I think it should be left in to keep minstrel honest if nothing else.

I don't care if it is left in, but it had better not be enabled by
default.  Randomly not being able to connect AT ALL to wireless
networks is not a valid "rate control" model.

                 Linus

Re: [PATCH] ath9k: Use minstrel rate control by default

[Sujith Manoharan]

Ben Greear wrote:
> If you will just delete that code above, then you can compile in both ath9k
> and minstrel rate controls and select at run-time by twiddling the mac80211
> rate control module option.

I am not sure if a user is going to change the rate control at run-time by
reloading mac80211 and what would be point in using ath9k RC which has basic
connectivity problems ?

> Last I tested there were still advantages to ath9k rate control in our environment,
> (including when running through an attenuator and over-the-air),
> so I think it should be left in to keep minstrel honest if nothing else.

ath9k RC shows slightly higher throughput numbers in an _ideal_ environment,
but that is hardly the case in real-world usage. I agree that the code can
be left in the driver, it will be useful for comparison with minstrel.

Sujith

Re: [PATCH] ath9k: Use minstrel rate control by default

[Ben Greear]

On 06/05/2013 09:05 PM, Sujith Manoharan wrote:
> Ben Greear wrote:
>> If you will just delete that code above, then you can compile in both ath9k
>> and minstrel rate controls and select at run-time by twiddling the mac80211
>> rate control module option.
>
> I am not sure if a user is going to change the rate control at run-time by
> reloading mac80211 and what would be point in using ath9k RC which has basic
> connectivity problems ?
>
>> Last I tested there were still advantages to ath9k rate control in our environment,
>> (including when running through an attenuator and over-the-air),
>> so I think it should be left in to keep minstrel honest if nothing else.
>
> ath9k RC shows slightly higher throughput numbers in an _ideal_ environment,
> but that is hardly the case in real-world usage. I agree that the code can
> be left in the driver, it will be useful for comparison with minstrel.

Then default to minstrel, but no need to make people re-compile their
kernel to try a different rate control.  For some users/systems this is
effectively impossible, and for the rest, it is an extra bit of pain.

The point of leaving ath9k rate control in is exactly to help make sure
minstrel has something to compare against.  And if a user happens to know
they have a clean environment always and forever, then ath9k may be
the best solution for them.

It is a trivial amount of change to your patch to enable this
behaviour.  In fact, if you just remove the code I mentioned then
they will *automatically* get defaulted to minstrel regardless of
their kernel config.  The only way to use ath9k rate control after
that is to set the module option in mac80211 (and enable ath9k rate
control for compilation).

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] ath9k: Use minstrel rate control by default

[Sujith Manoharan]

Ben Greear wrote:
> Then default to minstrel, but no need to make people re-compile their
> kernel to try a different rate control.  For some users/systems this is
> effectively impossible, and for the rest, it is an extra bit of pain.
> 
> The point of leaving ath9k rate control in is exactly to help make sure
> minstrel has something to compare against.  And if a user happens to know
> they have a clean environment always and forever, then ath9k may be
> the best solution for them.
> 
> It is a trivial amount of change to your patch to enable this
> behaviour.  In fact, if you just remove the code I mentioned then
> they will *automatically* get defaulted to minstrel regardless of
> their kernel config.  The only way to use ath9k rate control after
> that is to set the module option in mac80211 (and enable ath9k rate
> control for compilation).

Yes, I think this can be done instead, since we have always had both
ath9k RC and minstrel compiled in by default. I'll send a v2.

Sujith