Messages in this thread | | | Date | Wed, 28 Jul 2021 21:37:05 +0300 | From | Vladimir Oltean <> | Subject | Re: [RFC net-next 1/2] net: dsa: tag_mtk: skip address learning on transmit to standalone ports |
| |
On Thu, Jul 29, 2021 at 01:53:25AM +0800, DENG Qingfang wrote: > Consider the following bridge configuration, where bond0 is not > offloaded: > > +-- br0 --+ > / / | \ > / / | \ > / | | bond0 > / | | / \ > swp0 swp1 swp2 swp3 swp4 > . . . > . . . > A B C > > Address learning is enabled on offloaded ports (swp0~2) and the CPU > port, so when client A sends a packet to C, the following will happen: > > 1. The switch learns that client A can be reached at swp0. > 2. The switch probably already knows that client C can be reached at the > CPU port, so it forwards the packet to the CPU. > 3. The bridge core knows client C can be reached at bond0, so it > forwards the packet back to the switch. > 4. The switch learns that client A can be reached at the CPU port. > 5. The switch forwards the packet to either swp3 or swp4, according to > the packet's tag. > > That makes client A's MAC address flap between swp0 and the CPU port. If > client B sends a packet to A, it is possible that the packet is > forwarded to the CPU. With offload_fwd_mark = 1, the bridge core won't > forward it back to the switch, resulting in packet loss. > > To avoid that, skip address learning on the CPU port when the destination > port is standalone, which can be done by setting the SA_DIS bit of the > MTK tag, if bridge_dev of the destination port is not set. > > Signed-off-by: DENG Qingfang <dqfext@gmail.com> > --- > net/dsa/tag_mtk.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/net/dsa/tag_mtk.c b/net/dsa/tag_mtk.c > index cc3ba864ad5b..8c361812e21b 100644 > --- a/net/dsa/tag_mtk.c > +++ b/net/dsa/tag_mtk.c > @@ -15,8 +15,7 @@ > #define MTK_HDR_XMIT_TAGGED_TPID_8100 1 > #define MTK_HDR_XMIT_TAGGED_TPID_88A8 2 > #define MTK_HDR_RECV_SOURCE_PORT_MASK GENMASK(2, 0) > -#define MTK_HDR_XMIT_DP_BIT_MASK GENMASK(5, 0) > -#define MTK_HDR_XMIT_SA_DIS BIT(6) > +#define MTK_HDR_XMIT_SA_DIS_SHIFT 6 > > static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb, > struct net_device *dev) > @@ -50,7 +49,8 @@ static struct sk_buff *mtk_tag_xmit(struct sk_buff *skb, > * whether that's a combined special tag with 802.1Q header. > */ > mtk_tag[0] = xmit_tpid; > - mtk_tag[1] = (1 << dp->index) & MTK_HDR_XMIT_DP_BIT_MASK;
Why stop AND-ing with MTK_HDR_XMIT_DP_BIT_MASK if you were doing that before? If it's not needed (probably isn't), it would be nice to split that up.
> + mtk_tag[1] = BIT(dp->index) | > + (!dp->bridge_dev << MTK_HDR_XMIT_SA_DIS_SHIFT); > > /* Tag control information is kept for 802.1Q */ > if (xmit_tpid == MTK_HDR_XMIT_UNTAGGED) { > -- > 2.25.1 >
Otherwise this is as correct as can be without implementing TX forwarding offload for the bridge (which you've explained why it doesn't map 1:1 with what your hw can do). But just because a port is under a bridge doesn't mean that the only packets it sends belong to that bridge. Think AF_PACKET sockets, PTP etc. The bridge also has a no_linklocal_learn option that maybe should be taken into consideration for drivers that can do something meaningful about it. Anyway, food for thought.
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
| |