Nikolay Aleksandrov
2022-Mar-30 16:16 UTC
[Bridge] [PATCH net-next v2] veth: Support bonding events
On 30/03/2022 18:51, Jakub Kicinski wrote:> On Wed, 30 Mar 2022 13:14:12 +0200 Alexandra Winter wrote: >>>> This patch in no way addresses (2). But then, again, if we put >>>> a macvlan on top of a bridge master it will shotgun its GARPS all >>>> the same. So it's not like veth would be special in that regard. >>>> >>>> Nik, what am I missing? >>> >>> If we're talking about macvlan -> bridge -> bond then the bond flap's >>> notify peers shouldn't reach the macvlan. > > Hm, right. I'm missing a step in my understanding. As you say bridge > does not seem to be re-broadcasting the event to its master. So how > does Alexandra catch this kind of an event? :S > > case NETDEV_NOTIFY_PEERS: > /* propagate to peer of a bridge attached veth */ > if (netif_is_bridge_master(dev)) { > > IIUC bond will notify with dev == bond netdev. Where is the event with > dev == br generated? >Good question. :)>>> Generally broadcast traffic >>> is quite expensive for the bridge, I have patches that improve on the >>> technical side (consider ports only for the same bcast domain), but you also >>> wouldn't want unnecessary bcast packets being sent around. :) >>> There are setups with tens of bond devices and propagating that to all would be >>> very expensive, but most of all unnecessary. It would also hurt setups with >>> a lot of vlan devices on the bridge. There are setups with hundreds of vlans >>> and hundreds of macvlans on top, propagating it up would send it to all of >>> them and that wouldn't scale at all, these mostly have IP addresses too. > > Ack. > >>> Perhaps we can enable propagation on a per-port or per-bridge basis, then we >>> can avoid these walks. That is, make it opt-in. > > Maybe opt-out? But assuming the event is only generated on > active/backup switch over - when would it be okay to ignore > the notification? >Let me just clarify, so I'm sure I've not misunderstood you. Do you mean opt-out as in make it default on? IMO that would be a problem, large scale setups would suddenly start propagating it to upper devices which would cause a lot of unnecessary bcast. I meant enable it only if needed, and only on specific ports (second part is not necessary, could be global, I think it's ok either way). I don't think any setup which has many upper vlans/macvlans would ever enable this.>>>>> It also seems difficult to avoid re-bouncing the notifier. >>>> >>>> syzbot will make short work of this patch, I think the potential >>>> for infinite loops has to be addressed somehow. IIUC this is the >>>> first instance of forwarding those notifiers to a peer rather >>>> than within a upper <> lower device hierarchy which is a DAG. >> >> My concern was about the Hangbin's alternative proposal to notify all >> bridge ports. I hope in my porposal I was able to avoid infinite loops. > > Possibly I'm confused as to where the notification for bridge master > gets sent..IIUC it bypasses the bridge and sends a notify peers for the veth peer so it would generate a grat arp (inetdev_event -> NETDEV_NOTIFY_PEERS).
Jakub Kicinski
2022-Mar-30 17:12 UTC
[Bridge] [PATCH net-next v2] veth: Support bonding events
On Wed, 30 Mar 2022 19:16:42 +0300 Nikolay Aleksandrov wrote:> > Maybe opt-out? But assuming the event is only generated on > > active/backup switch over - when would it be okay to ignore > > the notification? > > Let me just clarify, so I'm sure I've not misunderstood you. Do you mean opt-out as in > make it default on? IMO that would be a problem, large scale setups would suddenly > start propagating it to upper devices which would cause a lot of unnecessary bcast. > I meant enable it only if needed, and only on specific ports (second part is not > necessary, could be global, I think it's ok either way). I don't think any setup > which has many upper vlans/macvlans would ever enable this.That may be. I don't have a good understanding of scenarios in which GARP is required and where it's not :) Goes without saying but the default should follow the more common scenario.> >> My concern was about the Hangbin's alternative proposal to notify all > >> bridge ports. I hope in my porposal I was able to avoid infinite loops. > > > > Possibly I'm confused as to where the notification for bridge master > > gets sent.. > > IIUC it bypasses the bridge and sends a notify peers for the veth peer so it would > generate a grat arp (inetdev_event -> NETDEV_NOTIFY_PEERS).Ack, I was basically repeating the question of where does the notification with dev == br get generated. There is a protection in this patch to make sure the other end of the veth is not plugged into a bridge (i.e. is not a bridge port) but there can be a macvlan on top of that veth that is part of a bridge, so IIUC that check is either insufficient or unnecessary.