Re: [PATCH net-next 29/30] net: dsa: introduce preferred_default_local_cpu_port and use on MT7530

Vladimir Oltean
2023-05-26T17:18:33Z

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?