Remove unnecessary sysctls, add comments to the remaining ones. #139

fbl · 2021-04-19T19:07:29+02:00

fbl commented

2021-04-19 19:07:29 +02:00

Many of the set sysctls are either unnecessary, are already default in the kernel or in OpenWrts defaults, or the reason for them being explicitly set is unknown.

Remove all those sysctls from fff-network, as unfounded deviations from default values will cause hard-to-debug problems in the future.

The original motivation for this patch is the netdev_max_backlog sysctl, which was set to a very low value without any reason or comment. This hurt forwarding performance on mt7621 with DSA significantly and took quite a while to discover.

Many of the set sysctls are either unnecessary, are already default in the kernel or in OpenWrts defaults, or the reason for them being explicitly set is unknown. Remove all those sysctls from fff-network, as unfounded deviations from default values will cause hard-to-debug problems in the future. The original motivation for this patch is the netdev_max_backlog sysctl, which was set to a very low value without any reason or comment. This hurt forwarding performance on mt7621 with DSA significantly and took quite a while to discover.

fbl added the

RFC

label 2021-04-19 19:07:29 +02:00

fbl added 2 commits 2021-04-19 19:07:30 +02:00

9fc8413aec fff-network: Remove obsolete and unnecessary sysctls

Many of the set sysctls are either unnecessary, are already default in
the kernel or in OpenWrts defaults, or the reason for them being
explicitly set is unknown.

Remove all those sysctls from fff-network, as unfounded deviations from
default values will cause hard-to-debug problems in the future.

The original motivation for this patch is the netdev_max_backlog sysctl,
which was set to a very low value without any reason or comment.
This hurt forwarding performance on mt7621 with DSA significantly and
took quite a while to discover.

Signed-off-by: Fabian Bläse <fabian@blaese.de>

285c2cc22d fff-network: Add comments to set sysctls

To make it clear why the set sysctls are necessary, add appropriate
comments to them. Also reorder them for improved readability.

Signed-off-by: Fabian Bläse <fabian@blaese.de>

fbl removed the

RFC

label 2021-06-20 16:36:17 +02:00

fbl commented

2021-07-09 10:05:06 +02:00

Tested-by: Fabian Bläse <fabian@blaese.de>

`Tested-by: Fabian Bläse <fabian@blaese.de>`

fbl commented

2021-07-09 10:25:13 +02:00

applied.

fbl closed this pull request

2021-07-09 10:25:13 +02:00

adschm commented

2021-07-09 17:21:30 +02:00

Seit wann applien wir denn Patches ganz ohne Review?

fbl commented

2021-07-09 17:27:18 +02:00

Der Patch lag knapp 3 Monate rum. Da er für für einige Geräte ein echtes Performance-Problem löst und aktuell Review-Technisch fast gar nichts passiert, habe ich mir die Freiheit genommen, den Patch zu applien.

Ich glaube wir hatten das früher auch mal in Diskussion, ob man die (ungeschriebenen?) Regeln dahingehend etwas aufweichen möchte, um die Entwicklung nicht unnötig zu bremsen. Ich wäre hier auch weiterhin dafür, das zu tun.

Der Patch lag knapp 3 Monate rum. Da er für für einige Geräte ein echtes Performance-Problem löst und aktuell Review-Technisch fast gar nichts passiert, habe ich mir die Freiheit genommen, den Patch zu applien. Ich glaube wir hatten das früher auch mal in Diskussion, ob man die (ungeschriebenen?) Regeln dahingehend etwas aufweichen möchte, um die Entwicklung nicht unnötig zu bremsen. Ich wäre hier auch weiterhin dafür, das zu tun.

adschm commented

2021-07-09 17:36:27 +02:00

Ja, wir hatten das in Diskussion und alle außer mir waren dagegen.

Und wenn ich alle Patches applie, die lange rumliegen, dann erkennt man die Firmware nicht mehr wieder ...

Ja, wir hatten das in Diskussion und alle außer mir waren dagegen. Und wenn ich alle Patches applie, die lange rumliegen, dann erkennt man die Firmware nicht mehr wieder ...

fbl commented

2021-07-09 17:42:12 +02:00

Ja, wir hatten das in Diskussion und alle außer mir waren dagegen.

Also ich dachte eigentlich, dass ich dafür war. Möglicherweise hats das damals aber aus Zeitmangel nicht bis in eine Mail geschafft.

Und wenn ich alle Patches applie, die lange rumliegen, dann erkennt man die Firmware nicht mehr wieder ...

Alternativvorschläge für Patches, die keine Reviewer finden (ohne veta!), aber aus Sicht eines Commiters/Entwicklers wichtig sind?
Ich hatte jetzt allgemein eigentlich auch nicht vor, bei jeder Änderung 3 Monate zu warten, um es dann ohne Review applien zu können.

> Ja, wir hatten das in Diskussion und alle außer mir waren dagegen. Also ich dachte eigentlich, dass ich dafür war. Möglicherweise hats das damals aber aus Zeitmangel nicht bis in eine Mail geschafft. > Und wenn ich alle Patches applie, die lange rumliegen, dann erkennt man die Firmware nicht mehr wieder ... Alternativvorschläge für Patches, die keine Reviewer finden (ohne veta!), aber aus Sicht eines Commiters/Entwicklers wichtig sind? Ich hatte jetzt allgemein eigentlich auch nicht vor, bei jeder Änderung 3 Monate zu warten, um es dann ohne Review applien zu können.