| Message ID | 20230522121532.86610-30-arinc.unal@arinc9.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: dsa: mt7530: improve, trap BPDU & LLDP, and prefer CPU port | expand |
On Mon, May 22, 2023 at 03:15:31PM +0300, arinc9.unal@gmail.com wrote: > From: Vladimir Oltean <olteanv@gmail.com> > > When multiple CPU ports are being used, the numerically smallest CPU port > becomes the port all user ports become affine to. This may not be the best > choice for all switches as there may be a numerically greater CPU port with > more bandwidth than the numerically smallest one. > > Such switches are MT7530 and MT7531BE, which the MT7530 DSA subdriver > controls. Port 5 of these switches has got RGMII whilst port 6 has got > either TRGMII or SGMII. > > Therefore, introduce the preferred_default_local_cpu_port operation to the > DSA subsystem and use it on the MT7530 DSA subdriver to prefer port 6 as > the default CPU port. > > To prove the benefit of this operation, I (Arınç) have done a bidirectional > speed test between two DSA user ports on the MT7531BE switch using iperf3. > The user ports are 1 Gbps full duplex and on different networks so the SoC > MAC would have to do 2 Gbps TX and 2 Gbps RX to deliver full speed. I think the real argument would sound like this: Since the introduction of the OF bindings, DSA has always had a policy that in case multiple CPU ports are present in the device tree, the numerically first one is always chosen. The MT7530 switch family has 2 CPU ports, 5 and 6, where port 6 is preferable because it has higher bandwidth. The MT7530 driver developers had 3 options: - to modify DSA when the driver was introduced, such as to prefer the better port - to declare both CPU ports in device trees as CPU ports, and live with the sub-optimal performance resulting from not preferring the better port - to declare just port 6 in the device tree as a CPU port Of course they chose the path of least resistance (3rd option), kicking the can down the road. The hardware description in the device tree is supposed to be stable - developers are not supposed to adopt the strategy of piecemeal hardware description, where the device tree is updated in lockstep with the features that the kernel currently supports. Now, as a result of the fact that they did that, any attempts to modify the device tree and describe both CPU ports as CPU ports would make DSA change its default selection from port 6 to 5, effectively resulting in a performance degradation visible to users as can be seen below vvvvv > > Without preferring port 6: > > [ ID][Role] Interval Transfer Bitrate Retr > [ 5][TX-C] 0.00-20.00 sec 374 MBytes 157 Mbits/sec 734 sender > [ 5][TX-C] 0.00-20.00 sec 373 MBytes 156 Mbits/sec receiver > [ 7][RX-C] 0.00-20.00 sec 1.81 GBytes 778 Mbits/sec 0 sender > [ 7][RX-C] 0.00-20.00 sec 1.81 GBytes 777 Mbits/sec receiver > > With preferring port 6: > > [ ID][Role] Interval Transfer Bitrate Retr > [ 5][TX-C] 0.00-20.00 sec 1.99 GBytes 856 Mbits/sec 273 sender > [ 5][TX-C] 0.00-20.00 sec 1.99 GBytes 855 Mbits/sec receiver > [ 7][RX-C] 0.00-20.00 sec 1.72 GBytes 737 Mbits/sec 15 sender > [ 7][RX-C] 0.00-20.00 sec 1.71 GBytes 736 Mbits/sec receiver > > Using one port for WAN and the other ports for LAN is a very popular use > case which is what this test emulates. As such, this change proposes that we retroactively modify stable kernels to keep the mt7530 driver preferring port 6 even with device trees where the hardware is more fully described. Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch") > > This doesn't affect the remaining switches, MT7531AE and the switch on the > MT7988 SoC. Both CPU ports of the MT7531AE switch have got SGMII and there > is only one CPU port on the switch on the MT7988 SoC. > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> > --- See the difference in intent?
On 26.05.2023 20:17, Vladimir Oltean wrote: > On Mon, May 22, 2023 at 03:15:31PM +0300, arinc9.unal@gmail.com wrote: >> From: Vladimir Oltean <olteanv@gmail.com> >> >> When multiple CPU ports are being used, the numerically smallest CPU port >> becomes the port all user ports become affine to. This may not be the best >> choice for all switches as there may be a numerically greater CPU port with >> more bandwidth than the numerically smallest one. >> >> Such switches are MT7530 and MT7531BE, which the MT7530 DSA subdriver >> controls. Port 5 of these switches has got RGMII whilst port 6 has got >> either TRGMII or SGMII. >> >> Therefore, introduce the preferred_default_local_cpu_port operation to the >> DSA subsystem and use it on the MT7530 DSA subdriver to prefer port 6 as >> the default CPU port. >> >> To prove the benefit of this operation, I (Arınç) have done a bidirectional >> speed test between two DSA user ports on the MT7531BE switch using iperf3. >> The user ports are 1 Gbps full duplex and on different networks so the SoC >> MAC would have to do 2 Gbps TX and 2 Gbps RX to deliver full speed. > > I think the real argument would sound like this: > > Since the introduction of the OF bindings, DSA has always had a policy > that in case multiple CPU ports are present in the device tree, the > numerically first one is always chosen. > > The MT7530 switch family has 2 CPU ports, 5 and 6, where port 6 is > preferable because it has higher bandwidth. > > The MT7530 driver developers had 3 options: > - to modify DSA when the driver was introduced, such as to prefer the > better port > - to declare both CPU ports in device trees as CPU ports, and live with > the sub-optimal performance resulting from not preferring the better > port > - to declare just port 6 in the device tree as a CPU port > > Of course they chose the path of least resistance (3rd option), kicking > the can down the road. The hardware description in the device tree is > supposed to be stable - developers are not supposed to adopt the > strategy of piecemeal hardware description, where the device tree is > updated in lockstep with the features that the kernel currently supports. > > Now, as a result of the fact that they did that, any attempts to modify > the device tree and describe both CPU ports as CPU ports would make DSA > change its default selection from port 6 to 5, effectively resulting in > a performance degradation visible to users as can be seen below vvvvv > >> >> Without preferring port 6: >> >> [ ID][Role] Interval Transfer Bitrate Retr >> [ 5][TX-C] 0.00-20.00 sec 374 MBytes 157 Mbits/sec 734 sender >> [ 5][TX-C] 0.00-20.00 sec 373 MBytes 156 Mbits/sec receiver >> [ 7][RX-C] 0.00-20.00 sec 1.81 GBytes 778 Mbits/sec 0 sender >> [ 7][RX-C] 0.00-20.00 sec 1.81 GBytes 777 Mbits/sec receiver >> >> With preferring port 6: >> >> [ ID][Role] Interval Transfer Bitrate Retr >> [ 5][TX-C] 0.00-20.00 sec 1.99 GBytes 856 Mbits/sec 273 sender >> [ 5][TX-C] 0.00-20.00 sec 1.99 GBytes 855 Mbits/sec receiver >> [ 7][RX-C] 0.00-20.00 sec 1.72 GBytes 737 Mbits/sec 15 sender >> [ 7][RX-C] 0.00-20.00 sec 1.71 GBytes 736 Mbits/sec receiver >> >> Using one port for WAN and the other ports for LAN is a very popular use >> case which is what this test emulates. > > As such, this change proposes that we retroactively modify stable > kernels to keep the mt7530 driver preferring port 6 even with device > trees where the hardware is more fully described. > > Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch") > >> >> This doesn't affect the remaining switches, MT7531AE and the switch on the >> MT7988 SoC. Both CPU ports of the MT7531AE switch have got SGMII and there >> is only one CPU port on the switch on the MT7988 SoC. >> >> Signed-off-by: Vladimir Oltean <olteanv@gmail.com> >> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> >> --- > > See the difference in intent? Yeah, nicely put. Arınç
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 8f5a8803cb33..8fd23da76169 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -399,6 +399,20 @@ static void mt7530_pll_setup(struct mt7530_priv *priv) core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_GSWCK_EN); } +/* If port 6 is available as a CPU port, always prefer that as the default, + * otherwise don't care. + */ +static struct dsa_port * +mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds) +{ + struct dsa_port *cpu_dp = dsa_to_port(ds, 6); + + if (dsa_port_is_cpu(cpu_dp)) + return cpu_dp; + + return NULL; +} + /* Setup port 6 interface mode and TRGMII TX circuit */ static void mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface) @@ -3000,6 +3014,7 @@ static int mt7988_setup(struct dsa_switch *ds) const struct dsa_switch_ops mt7530_switch_ops = { .get_tag_protocol = mtk_get_tag_protocol, .setup = mt753x_setup, + .preferred_default_local_cpu_port = mt753x_preferred_default_local_cpu_port, .get_strings = mt7530_get_strings, .get_ethtool_stats = mt7530_get_ethtool_stats, .get_sset_count = mt7530_get_sset_count, diff --git a/include/net/dsa.h b/include/net/dsa.h index 8903053fa5aa..ab0f0a5b0860 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -958,6 +958,14 @@ struct dsa_switch_ops { struct phy_device *phy); void (*port_disable)(struct dsa_switch *ds, int port); + /* + * Compatibility between device trees defining multiple CPU ports and + * drivers which are not OK to use by default the numerically smallest + * CPU port of a switch for its local ports. This can return NULL, + * meaning "don't know/don't care". + */ + struct dsa_port *(*preferred_default_local_cpu_port)(struct dsa_switch *ds); + /* * Port's MAC EEE settings */ diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index ab1afe67fd18..1afed89e03c0 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -403,6 +403,24 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst) return 0; } +static struct dsa_port * +dsa_switch_preferred_default_local_cpu_port(struct dsa_switch *ds) +{ + struct dsa_port *cpu_dp; + + if (!ds->ops->preferred_default_local_cpu_port) + return NULL; + + cpu_dp = ds->ops->preferred_default_local_cpu_port(ds); + if (!cpu_dp) + return NULL; + + if (WARN_ON(!dsa_port_is_cpu(cpu_dp) || cpu_dp->ds != ds)) + return NULL; + + return cpu_dp; +} + /* Perform initial assignment of CPU ports to user ports and DSA links in the * fabric, giving preference to CPU ports local to each switch. Default to * using the first CPU port in the switch tree if the port does not have a CPU @@ -410,12 +428,16 @@ static int dsa_tree_setup_default_cpu(struct dsa_switch_tree *dst) */ static int dsa_tree_setup_cpu_ports(struct dsa_switch_tree *dst) { - struct dsa_port *cpu_dp, *dp; + struct dsa_port *preferred_cpu_dp, *cpu_dp, *dp; list_for_each_entry(cpu_dp, &dst->ports, list) { if (!dsa_port_is_cpu(cpu_dp)) continue; + preferred_cpu_dp = dsa_switch_preferred_default_local_cpu_port(cpu_dp->ds); + if (preferred_cpu_dp && preferred_cpu_dp != cpu_dp) + continue; + /* Prefer a local CPU port */ dsa_switch_for_each_port(dp, cpu_dp->ds) { /* Prefer the first local CPU port found */