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

[Vladimir Oltean]

On Mon, May 22, 2023 at 03:15:31PM +0300, arinc9.unal@xxxxxxxxx wrote:
> From: Vladimir Oltean <olteanv@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Signed-off-by: Arınç ÜNAL <arinc.unal@xxxxxxxxxx>
> ---

See the difference in intent?