mbox series

[net-next,0/6] Improvement for DSA cross-chip setups

Message ID 20210618183017.3340769-1-olteanv@gmail.com
Headers show
Series Improvement for DSA cross-chip setups | expand

Message

Vladimir Oltean June 18, 2021, 6:30 p.m. UTC
From: Vladimir Oltean <vladimir.oltean@nxp.com>

This series improves some aspects in multi-switch DSA tree topologies:
- better device tree validation
- better handling of MTU changes
- better handling of multicast addresses
- removal of some unused code

Vladimir Oltean (6):
  net: dsa: assert uniqueness of dsa,member properties
  net: dsa: export the dsa_port_is_{user,cpu,dsa} helpers
  net: dsa: execute dsa_switch_mdb_add only for routing port in
    cross-chip topologies
  net: dsa: calculate the largest_mtu across all ports in the tree
  net: dsa: targeted MTU notifiers should only match on one port
  net: dsa: remove cross-chip support from the MRP notifiers

 include/net/dsa.h  | 15 ++++++++
 net/dsa/dsa2.c     | 22 ++++--------
 net/dsa/dsa_priv.h |  4 +--
 net/dsa/port.c     |  4 +--
 net/dsa/slave.c    | 22 ++++++------
 net/dsa/switch.c   | 87 ++++++++--------------------------------------
 6 files changed, 53 insertions(+), 101 deletions(-)

Comments

Florian Fainelli June 19, 2021, 1:59 a.m. UTC | #1
On 6/18/2021 11:30 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> The cross-chip notifiers work by comparing each ds->index against the

> info->sw_index value from the notifier. The ds->index is retrieved from

> the device tree dsa,member property.

> 

> If a single tree cross-chip topology does not declare unique switch IDs,

> this will result in hard-to-debug issues/voodoo effects such as the

> cross-chip notifier for one switch port also matching the port with the

> same number from another switch.

> 

> Check in dsa_switch_parse_member_of() whether the DSA switch tree

> contains a DSA switch with the index we're preparing to add, before

> actually adding it.

> 

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>


Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

-- 
Florian
Florian Fainelli June 20, 2021, 2:24 p.m. UTC | #2
On 6/18/2021 11:30 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> Currently, the notifier for adding a multicast MAC address matches on

> the targeted port and on all DSA links in the system, be they upstream

> or downstream links.

> 

> This leads to a considerable amount of useless traffic.

> 

> Consider this daisy chain topology, and a MDB add notifier emitted on

> sw0p0. It matches on sw0p0, sw0p3, sw1p3 and sw2p4.

> 

>    sw0p0     sw0p1     sw0p2     sw0p3     sw0p4

> [  user ] [  user ] [  user ] [  dsa  ] [  cpu  ]

> [   x   ] [       ] [       ] [   x   ] [       ]

>                                   |

>                                   +---------+

>                                             |

>    sw1p0     sw1p1     sw1p2     sw1p3     sw1p4

> [  user ] [  user ] [  user ] [  dsa  ] [  dsa  ]

> [       ] [       ] [       ] [   x   ] [   x   ]

>                                   |

>                                   +---------+

>                                             |

>    sw2p0     sw2p1     sw2p2     sw2p3     sw2p4

> [  user ] [  user ] [  user ] [  user ] [  dsa  ]

> [       ] [       ] [       ] [       ] [   x   ]

> 

> But switch 0 has no reason to send the multicast traffic for that MAC

> address on sw0p3, which is how it reaches switches 1 and 2. Those

> switches don't expect, according to the user configuration, to receive

> this multicast address from switch 1, and they will drop it anyway,

> because the only valid destination is the port they received it on.

> They only need to configure themselves to deliver that multicast address

> _towards_ switch 1, where the MDB entry is installed.

> 

> Similarly, switch 1 should not send this multicast traffic towards

> sw1p3, because that is how it reaches switch 2.

> 

> With this change, the heat map for this MDB notifier changes as follows:

> 

>    sw0p0     sw0p1     sw0p2     sw0p3     sw0p4

> [  user ] [  user ] [  user ] [  dsa  ] [  cpu  ]

> [   x   ] [       ] [       ] [       ] [       ]

>                                   |

>                                   +---------+

>                                             |

>    sw1p0     sw1p1     sw1p2     sw1p3     sw1p4

> [  user ] [  user ] [  user ] [  dsa  ] [  dsa  ]

> [       ] [       ] [       ] [       ] [   x   ]

>                                   |

>                                   +---------+

>                                             |

>    sw2p0     sw2p1     sw2p2     sw2p3     sw2p4

> [  user ] [  user ] [  user ] [  user ] [  dsa  ]

> [       ] [       ] [       ] [       ] [   x   ]

> 

> Now the mdb notifier behaves the same as the fdb notifier.

> 

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>


Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

-- 
Florian
Florian Fainelli June 20, 2021, 2:25 p.m. UTC | #3
On 6/18/2021 11:30 AM, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> dsa_slave_change_mtu() calls dsa_port_mtu_change() twice:

> - it sends a cross-chip notifier with the MTU of the CPU port which is

>   used to update the DSA links.

> - it sends one targeted MTU notifier which is supposed to only match the

>   user port on which we are changing the MTU. The "propagate_upstream"

>   variable is used here to bypass the cross-chip notifier system from

>   switch.c

> 

> But due to a mistake, the second, targeted notifier matches not only on

> the user port, but also on the DSA link which is a member of the same

> switch, if that exists.

> 

> And because the DSA links of the entire dst were programmed in a

> previous round to the largest_mtu via a "propagate_upstream == true"

> notification, then the dsa_port_mtu_change(propagate_upstream == false)

> call that is immediately upcoming will break the MTU on the one DSA link

> which is chip-wise local to the dp whose MTU is changing right now.

> 

> Example given this daisy chain topology:

> 

>    sw0p0     sw0p1     sw0p2     sw0p3     sw0p4

> [  cpu  ] [  user ] [  user ] [  dsa  ] [  user ]

> [   x   ] [       ] [       ] [   x   ] [       ]

>                                   |

>                                   +---------+

>                                             |

>    sw1p0     sw1p1     sw1p2     sw1p3     sw1p4

> [  user ] [  user ] [  user ] [  dsa  ] [  dsa  ]

> [       ] [       ] [       ] [       ] [   x   ]

> 

> ip link set sw0p1 mtu 9000

> ip link set sw1p1 mtu 9000 # at this stage, sw0p1 and sw1p1 can talk

>                            # to one another using jumbo frames

> ip link set sw0p2 mtu 1500 # this programs the sw0p3 DSA link first to

>                            # the largest_mtu of 9000, then reprograms it to

>                            # 1500 with the "propagate_upstream == false"

>                            # notifier, breaking communication between

>                            # sw0p1 and sw1p1

> 

> To escape from this situation, make the targeted match really match on a

> single port - the user port, and rename the "propagate_upstream"

> variable to "targeted_match" to clarify the intention and avoid future

> issues.

> 

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>


Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

-- 
Florian
Andrew Lunn June 21, 2021, 1:53 p.m. UTC | #4
On Fri, Jun 18, 2021 at 09:30:12PM +0300, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>

> 

> The cross-chip notifiers work by comparing each ds->index against the

> info->sw_index value from the notifier. The ds->index is retrieved from

> the device tree dsa,member property.

> 

> If a single tree cross-chip topology does not declare unique switch IDs,

> this will result in hard-to-debug issues/voodoo effects such as the

> cross-chip notifier for one switch port also matching the port with the

> same number from another switch.

> 

> Check in dsa_switch_parse_member_of() whether the DSA switch tree

> contains a DSA switch with the index we're preparing to add, before

> actually adding it.

> 

> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>


Reviewed-by: Andrew Lunn <andrew@lunn.ch>


    Andrew