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

Arınç ÜNAL
2023-06-04T10:03:32Z

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ç