| Message ID | 20210126040949.3130937-1-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] bridge: Propagate NETDEV_NOTIFY_PEERS notifier | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 7 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Tue, Jan 26, 2021 at 04:55:22PM +0200, Nikolay Aleksandrov wrote: > >> Thanks for the reply. There are a few reasons I think the bridge should > >> handle NETDEV_NOTIFY_PEERS: > >> > >> 1. Only a few devices will call NETDEV_NOTIFY_PEERS notifier: bond, team, > >> virtio, xen, 6lowpan. There should have no much notification message. > > > > You can't send a broadcast to all ports because 1 bond's link status has changed. > > That makes no sense, the GARP needs to be sent only on that bond. The bond devices > > are heavily used with bridge setups, and in general the bridge is considered a switch > > device, it shouldn't be broadcasting GARPs to all ports when one changes link state. > > > > Scratch the last sentence, I guess you're talking about when the bond's mac causes > the bridge to change mac address by br_stp_recalculate_bridge_id(). I was wondering Yes, that's what I mean. Sorry I didn't make it clear in commit description. > at first why would you need to send garp, but in fact, as Ido mentioned privately, > it is already handled correctly, but you need to have set arp_notify sysctl. > Then if the bridge's mac changes because of the bond flapping a NETDEV_NOTIFY_PEERS will be > generated. Check: > devinet.c inetdev_event() -> case NETDEV_CHANGEADDR Yes, this is a generic work around. It will handle all mac changing instead of failover. For IGMP, although you said they are different. In my understanding, when bridge mac changed, we need to re-join multicast group, while a gratuitous ARP is also needed. I couldn't find a reason why IGMP message is OK but GARP is not. > > Alternatively you can always set the bridge mac address manually and then it won't be > changed by such events. Thanks for this tips. I'm not sure if administers like this way. This remind me another issue. Should we resend IGMP when got port NETDEV_RESEND_IGMP notify, Even the bridge mac address may not changed? Shouldn't we only resend IGMP, GARP when bridge mac address changed, e.g. diff --git a/net/bridge/br.c b/net/bridge/br.c index 1b169f8e7491..74571f24bb18 100644 --- a/net/bridge/br.c +++ b/net/bridge/br.c @@ -80,8 +80,11 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v changed_addr = br_stp_recalculate_bridge_id(br); spin_unlock_bh(&br->lock); - if (changed_addr) + if (changed_addr) { call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev); + call_netdevice_notifiers(NETDEV_RESEND_IGMP, br->dev); + call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, br->dev); + } break; @@ -124,11 +127,6 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v case NETDEV_PRE_TYPE_CHANGE: /* Forbid underlaying device to change its type. */ return NOTIFY_BAD; - - case NETDEV_RESEND_IGMP: - /* Propagate to master device */ - call_netdevice_notifiers(event, br->dev); - break; } if (event != NETDEV_UNREGISTER) Thanks Hangbin
On 27/01/2021 06:15, Hangbin Liu wrote: > On Tue, Jan 26, 2021 at 04:55:22PM +0200, Nikolay Aleksandrov wrote: >>>> Thanks for the reply. There are a few reasons I think the bridge should >>>> handle NETDEV_NOTIFY_PEERS: >>>> >>>> 1. Only a few devices will call NETDEV_NOTIFY_PEERS notifier: bond, team, >>>> virtio, xen, 6lowpan. There should have no much notification message. >>> >>> You can't send a broadcast to all ports because 1 bond's link status has changed. >>> That makes no sense, the GARP needs to be sent only on that bond. The bond devices >>> are heavily used with bridge setups, and in general the bridge is considered a switch >>> device, it shouldn't be broadcasting GARPs to all ports when one changes link state. >>> >> >> Scratch the last sentence, I guess you're talking about when the bond's mac causes >> the bridge to change mac address by br_stp_recalculate_bridge_id(). I was wondering > > Yes, that's what I mean. Sorry I didn't make it clear in commit description. > >> at first why would you need to send garp, but in fact, as Ido mentioned privately, >> it is already handled correctly, but you need to have set arp_notify sysctl. >> Then if the bridge's mac changes because of the bond flapping a NETDEV_NOTIFY_PEERS will be >> generated. Check: >> devinet.c inetdev_event() -> case NETDEV_CHANGEADDR > > Yes, this is a generic work around. It will handle all mac changing instead of > failover. > > For IGMP, although you said they are different. In my understanding, when > bridge mac changed, we need to re-join multicast group, while a gratuitous > ARP is also needed. I couldn't find a reason why IGMP message is OK but GARP > is not. > I think that's needed more because of port changing rather than mac changing. Switches need to be updated if the port has changed, all of that is already handled correctly by the bond. And I also meant that mcast is handled very differently in the bridge, usually you'd have snooping enabled. The patch below isn't correct and will actually break some cases when bonding flaps ports and propagates NETDEV_RESEND_IGMP with a bridge on top. >> >> Alternatively you can always set the bridge mac address manually and then it won't be >> changed by such events. > > Thanks for this tips. I'm not sure if administers like this way. > > This remind me another issue. Should we resend IGMP when got port > NETDEV_RESEND_IGMP notify, Even the bridge mac address may not changed? > Shouldn't we only resend IGMP, GARP when bridge mac address changed, e.g. > > diff --git a/net/bridge/br.c b/net/bridge/br.c > index 1b169f8e7491..74571f24bb18 100644 > --- a/net/bridge/br.c > +++ b/net/bridge/br.c > @@ -80,8 +80,11 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v > changed_addr = br_stp_recalculate_bridge_id(br); > spin_unlock_bh(&br->lock); > > - if (changed_addr) > + if (changed_addr) { > call_netdevice_notifiers(NETDEV_CHANGEADDR, br->dev); > + call_netdevice_notifiers(NETDEV_RESEND_IGMP, br->dev); > + call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, br->dev); > + } > > break; > > @@ -124,11 +127,6 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v > case NETDEV_PRE_TYPE_CHANGE: > /* Forbid underlaying device to change its type. */ > return NOTIFY_BAD; > - > - case NETDEV_RESEND_IGMP: > - /* Propagate to master device */ > - call_netdevice_notifiers(event, br->dev); > - break; > } > > if (event != NETDEV_UNREGISTER) > > > Thanks > Hangbin >
On Wed, Jan 27, 2021 at 11:43:30AM +0200, Nikolay Aleksandrov wrote: > > For IGMP, although you said they are different. In my understanding, when > > bridge mac changed, we need to re-join multicast group, while a gratuitous > > ARP is also needed. I couldn't find a reason why IGMP message is OK but GARP > > is not. > > > > I think that's needed more because of port changing rather than mac changing. > Switches need to be updated if the port has changed, all of that is already handled > correctly by the bond. And I also meant that mcast is handled very differently in > the bridge, usually you'd have snooping enabled. > > The patch below isn't correct and will actually break some cases when bonding > flaps ports and propagates NETDEV_RESEND_IGMP with a bridge on top. Hi Nikolay, I'm little curious. bond/team device will resend IGMP as their MAC address changed. - bond_resend_igmp_join_requests_delayed() - call_netdevice_notifiers(NETDEV_RESEND_IGMP, bond->dev); - team_mcast_rejoin_work() - call_netdevice_notifiers(NETDEV_RESEND_IGMP, team->dev); What's the purpose that bridge resend IGMP if it's mac address not changed? I mean, when there is a bridge on top of bond/team, when bond/team flap ports, bond/team will re-send IGMP and bridge just need to forward it. bridge doesn't need to re-send the IGMP itself if it's MAC address not changed. Thanks Hangbin
diff --git a/net/bridge/br.c b/net/bridge/br.c index ef743f94254d..b6a0921bb498 100644 --- a/net/bridge/br.c +++ b/net/bridge/br.c @@ -125,6 +125,7 @@ static int br_device_event(struct notifier_block *unused, unsigned long event, v /* Forbid underlying device to change its type. */ return NOTIFY_BAD; + case NETDEV_NOTIFY_PEERS: case NETDEV_RESEND_IGMP: /* Propagate to master device */ call_netdevice_notifiers(event, br->dev);
After adding bridge as upper layer of bond/team, we usually clean up the IP address on bond/team and set it on bridge. When there is a failover, bond/team will not send gratuitous ARP since it has no IP address. Then the down layer(e.g. VM tap dev) of bridge will not able to receive this notification. Make bridge to be able to handle NETDEV_NOTIFY_PEERS notifier. Signed-off-by: Hangbin Liu <liuhangbin@gmail.com> --- net/bridge/br.c | 1 + 1 file changed, 1 insertion(+)