diff mbox series

[net-next] r8169: enable GRO software interrupt coalescing per default

Message ID 9d94f2d8-d297-7550-2932-793a34e5efb9@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net-next] r8169: enable GRO software interrupt coalescing per default | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 12 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Heiner Kallweit Nov. 26, 2022, 2:07 p.m. UTC
There are reports about r8169 not reaching full line speed on certain
systems (e.g. SBC's) with a 2.5Gbps link.
There was a time when hardware interrupt coalescing was enabled per
default, but this was changed due to ASPM-related issues on few systems.

Meanwhile we have sysfs attributes for controlling kind of
"software interrupt coalescing" on the GRO level. However most distros
and users don't know about it. So lets set a conservative default for
both involved parameters. Users can still override the defaults via
sysfs. Don't enable these settings on the fast ethernet chip versions,
they are slow enough.

Even with these conservative setting interrupt load on my 1Gbps test
system reduced significantly.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/ethernet/realtek/r8169_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Jakub Kicinski Nov. 29, 2022, 3:07 a.m. UTC | #1
On Sat, 26 Nov 2022 15:07:07 +0100 Heiner Kallweit wrote:
> There are reports about r8169 not reaching full line speed on certain
> systems (e.g. SBC's) with a 2.5Gbps link.
> There was a time when hardware interrupt coalescing was enabled per
> default, but this was changed due to ASPM-related issues on few systems.
> 
> Meanwhile we have sysfs attributes for controlling kind of
> "software interrupt coalescing" on the GRO level. However most distros
> and users don't know about it. So lets set a conservative default for
> both involved parameters. Users can still override the defaults via
> sysfs. Don't enable these settings on the fast ethernet chip versions,
> they are slow enough.
> 
> Even with these conservative setting interrupt load on my 1Gbps test
> system reduced significantly.

Sure, why not. Could you please wrap the init into a helper?
Should help us ensure the params are not wildly different between
drivers and make any later refactoring easier.

Maybe something like:

/**
 * netdev_sw_irq_coalesce_default_on() - enable SW IRQ coalescing by default
 * @dev: netdev to enable the IRQ coalescing on
 * bla bla bla
 */
int netdev_sw_irq_coalesce_default_on(struct net_device *dev)
{
	WARN_ON(dev->reg_state != NETREG_UNREGISTERED);

	dev->gro_flush_timeout = 20000;
	dev->napi_defer_hard_irqs = 1;
}
EXPORT...
Heiner Kallweit Nov. 30, 2022, 10:33 p.m. UTC | #2
On 29.11.2022 04:07, Jakub Kicinski wrote:
> On Sat, 26 Nov 2022 15:07:07 +0100 Heiner Kallweit wrote:
>> There are reports about r8169 not reaching full line speed on certain
>> systems (e.g. SBC's) with a 2.5Gbps link.
>> There was a time when hardware interrupt coalescing was enabled per
>> default, but this was changed due to ASPM-related issues on few systems.
>>
>> Meanwhile we have sysfs attributes for controlling kind of
>> "software interrupt coalescing" on the GRO level. However most distros
>> and users don't know about it. So lets set a conservative default for
>> both involved parameters. Users can still override the defaults via
>> sysfs. Don't enable these settings on the fast ethernet chip versions,
>> they are slow enough.
>>
>> Even with these conservative setting interrupt load on my 1Gbps test
>> system reduced significantly.
> 
> Sure, why not. Could you please wrap the init into a helper?
> Should help us ensure the params are not wildly different between
> drivers and make any later refactoring easier.
> 
> Maybe something like:
> 
> /**
>  * netdev_sw_irq_coalesce_default_on() - enable SW IRQ coalescing by default
>  * @dev: netdev to enable the IRQ coalescing on
>  * bla bla bla
>  */
> int netdev_sw_irq_coalesce_default_on(struct net_device *dev)
> {
> 	WARN_ON(dev->reg_state != NETREG_UNREGISTERED);
> 
> 	dev->gro_flush_timeout = 20000;
> 	dev->napi_defer_hard_irqs = 1;
> }
> EXPORT...

Great, you did the most difficult work and chose a proper function name.
I followed your suggestion and put this (slightly adjusted) function
into net core. Just submitted the series including it.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 5bc1181f8..1cb51ec71 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -5282,6 +5282,12 @@  static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->hw_features |= NETIF_F_RXALL;
 	dev->hw_features |= NETIF_F_RXFCS;
 
+	if (tp->supports_gmii) {
+		/* enable software interrupt coalescing per default */
+		dev->gro_flush_timeout = 20000;
+		dev->napi_defer_hard_irqs = 1;
+	}
+
 	/* configure chip for default features */
 	rtl8169_set_features(dev, dev->features);