[OpenWrt-Devel,v3] kernel: ath10k-ct: provide a build variant for small RAM devices

According to many bugreports [0][1][2] the default ath10k-ct kernel
module is unusable on devices with just 64 MiB RAM or with 128 MiB and
dual ath10k cards. The target boards boot but eventually oom-killer
starts to interfere with normal operation, so the current state is
effectively broken.

Since the two patches in question have a performance impact (and
possibly some other unexpected side-effects) a dedicated build variant
is added so that users of the low RAM devices can still benefit from all
the ath10k-ct advantages.

According to testing [3] results, the issue can be experienced even with
"a 256MB device with three radios". Measured performance impact of
implementing small buffers was lowering "the maximum 5 GHz throughput on
an IPQ40xx device without RPS/XPS optimizations from 494/432 Mbit/s for
TCP transfers (download/upload) to 438/343 Mbit/s"

The patches were apparently inspired by QSDK tweaks used by ODMs for the
affected devices.

[0] http://lists.infradead.org/pipermail/openwrt-devel/2019-December/020573.html
[1] https://github.com/openwrt/openwrt/pull/1077
[2] https://bugs.openwrt.org/index.php?do=details&task_id=2664
[3] https://github.com/freifunk-gluon/gluon/pull/1440#issue-195607701

Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
This is compile-tested only!

Changes for v3:

    - Apply all patches to both build variants, select by a preprocessor
      symbol

Changes for v2:

    - Added performance impact details to the commit message
    - Fixed QUILT=1 operation
    - Refreshed the new patches

 package/kernel/ath10k-ct/Makefile             |  18 +++-
 ...0-0010-ath10k-limit-htt-rx-ring-size.patch |  28 +++++
 ...60-0011-ath10k-limit-pci-buffer-size.patch | 100 ++++++++++++++++++
 3 files changed, 145 insertions(+), 1 deletion(-)
 create mode 100644 package/kernel/ath10k-ct/patches/960-0010-ath10k-limit-htt-rx-ring-size.patch
 create mode 100644 package/kernel/ath10k-ct/patches/960-0011-ath10k-limit-pci-buffer-size.patch

Paul Fertser <fercerpav@gmail.com> writes:

> --- /dev/null
> +++ b/package/kernel/ath10k-ct/patches/960-0011-ath10k-limit-pci-buffer-size.patch
> @@ -0,0 +1,100 @@
> +--- a/ath10k-4.19/pci.c
> ++++ b/ath10k-4.19/pci.c
> +@@ -142,7 +142,11 @@ static struct ce_attr host_ce_config_wla
> + 		.flags = CE_ATTR_FLAGS,
> + 		.src_nentries = 0,
> + 		.src_sz_max = 2048,
> ++#ifndef CONFIG_ATH10K_SMALLBUFFERS
> + 		.dest_nentries = 512,
> ++#else
> ++		.dest_nentries = 128,
> ++#endif
> + 		.recv_cb = ath10k_pci_htt_htc_rx_cb,
> + 	},
> + 

Why not replace the magic numbers with a macro?  Then you could get away
with *one* "if configx then this else that"?  And preferably put it in a
header file.

Or maybe these things even could be made runtime configurable?  Buffers
of this size really should be IMHO, as there is no way to make one size
fit all.  As demonstrated...


Bjørn

On 12/23/19 10:17 AM, Bjørn Mork wrote:
> Paul Fertser <fercerpav@gmail.com> writes:
> 
>> --- /dev/null
>> +++ b/package/kernel/ath10k-ct/patches/960-0011-ath10k-limit-pci-buffer-size.patch
>> @@ -0,0 +1,100 @@
>> +--- a/ath10k-4.19/pci.c
>> ++++ b/ath10k-4.19/pci.c
>> +@@ -142,7 +142,11 @@ static struct ce_attr host_ce_config_wla
>> + 		.flags = CE_ATTR_FLAGS,
>> + 		.src_nentries = 0,
>> + 		.src_sz_max = 2048,
>> ++#ifndef CONFIG_ATH10K_SMALLBUFFERS
>> + 		.dest_nentries = 512,
>> ++#else
>> ++		.dest_nentries = 128,
>> ++#endif
>> + 		.recv_cb = ath10k_pci_htt_htc_rx_cb,
>> + 	},
>> + 
> 
> Why not replace the magic numbers with a macro?  Then you could get away
> with *one* "if configx then this else that"?  And preferably put it in a
> header file.
> 
> Or maybe these things even could be made runtime configurable?  Buffers
> of this size really should be IMHO, as there is no way to make one size
> fit all.  As demonstrated...

Hi,

Runtime configuration would be nice, but as far as I know there is no
standard interface available.

Huake

Hi Bjørn,

On Mon, Dec 23, 2019 at 10:17:11AM +0100, Bjørn Mork wrote:
> Paul Fertser <fercerpav@gmail.com> writes:
> > --- /dev/null
> > +++ b/package/kernel/ath10k-ct/patches/960-0011-ath10k-limit-pci-buffer-size.patch
> > @@ -0,0 +1,100 @@
> > +--- a/ath10k-4.19/pci.c
> > ++++ b/ath10k-4.19/pci.c
> > +@@ -142,7 +142,11 @@ static struct ce_attr host_ce_config_wla
> > + 		.flags = CE_ATTR_FLAGS,
> > + 		.src_nentries = 0,
> > + 		.src_sz_max = 2048,
> > ++#ifndef CONFIG_ATH10K_SMALLBUFFERS
> > + 		.dest_nentries = 512,
> > ++#else
> > ++		.dest_nentries = 128,
> > ++#endif
> > + 		.recv_cb = ath10k_pci_htt_htc_rx_cb,
> > + 	},
> > + 
> 
> Why not replace the magic numbers with a macro?  Then you could get away
> with *one* "if configx then this else that"?  And preferably put it in a
> header file.

There're different values for different buffers so there can't be a
single number to fit them all. And I do not see how adding 4 different
defines just for the sake of it would make sense there.

> Or maybe these things even could be made runtime configurable?  Buffers
> of this size really should be IMHO, as there is no way to make one size
> fit all.  As demonstrated...

This was already discussed, please see [0]. I think adding a kernel
module parameter would make sense for this but it's also much more
complicated than just adding two patches that were already used in
OpenWrt for many years. So far nobody volunteered to do that, and I
wanted to provide at least some semi-sane solution for the upcoming
release.

[0] https://patchwork.ozlabs.org/comment/2322701/

On 12/23/19 8:57 PM, Paul Fertser wrote:
> Hi Bjørn,
> 
> On Mon, Dec 23, 2019 at 10:17:11AM +0100, Bjørn Mork wrote:
>> Paul Fertser <fercerpav@gmail.com> writes:
>>> --- /dev/null
>>> +++ b/package/kernel/ath10k-ct/patches/960-0011-ath10k-limit-pci-buffer-size.patch
>>> @@ -0,0 +1,100 @@
>>> +--- a/ath10k-4.19/pci.c
>>> ++++ b/ath10k-4.19/pci.c
>>> +@@ -142,7 +142,11 @@ static struct ce_attr host_ce_config_wla
>>> + 		.flags = CE_ATTR_FLAGS,
>>> + 		.src_nentries = 0,
>>> + 		.src_sz_max = 2048,
>>> ++#ifndef CONFIG_ATH10K_SMALLBUFFERS
>>> + 		.dest_nentries = 512,
>>> ++#else
>>> ++		.dest_nentries = 128,
>>> ++#endif
>>> + 		.recv_cb = ath10k_pci_htt_htc_rx_cb,
>>> + 	},
>>> + 
>>
>> Why not replace the magic numbers with a macro?  Then you could get away
>> with *one* "if configx then this else that"?  And preferably put it in a
>> header file.
> 
> There're different values for different buffers so there can't be a
> single number to fit them all. And I do not see how adding 4 different
> defines just for the sake of it would make sense there.
> 
>> Or maybe these things even could be made runtime configurable?  Buffers
>> of this size really should be IMHO, as there is no way to make one size
>> fit all.  As demonstrated...
> 
> This was already discussed, please see [0]. I think adding a kernel
> module parameter would make sense for this but it's also much more
> complicated than just adding two patches that were already used in
> OpenWrt for many years. So far nobody volunteered to do that, and I
> wanted to provide at least some semi-sane solution for the upcoming
> release.
> 
> [0] https://patchwork.ozlabs.org/comment/2322701/
> 
Hi Paul,

Thank you for your patches, I applied both patches to master with some
small fixes and backported the first patch for 19.07 branch. The patch
adding the dependencies to the ath79 target is not applying on 19.07, it
would be nice if you could provide a version on top of 19.07 too.

Hauke