[PATCH 0/3] bridge: few enhancements and small fixes

[PATCH 0/3] bridge: few enhancements and small fixes

[Luis R. Rodriguez]

Here's a few fixes I've been carrying around. I've now tested them
on as many systems / environments as I can. They should be ready.

Luis R. Rodriguez (3):
  bridge: preserve random init MAC address
  bridge: trigger a bridge calculation upon port changes
  bridge: fix bridge root block on designated port

 net/bridge/br_device.c  |  1 +
 net/bridge/br_netlink.c | 24 ++++++++++++++++
 net/bridge/br_private.h |  2 ++
 net/bridge/br_stp.c     | 73 +++++++++++++++++++++++++++++++++++++++++++++----
 net/bridge/br_stp_if.c  |  6 +++-
 5 files changed, 99 insertions(+), 7 deletions(-)

-- 
1.8.5.3

[PATCH 1/3] bridge: preserve random init MAC address

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

As it is now if you add create a bridge it gets started
with a random MAC address and if you then add a net_device
as a slave but later kick it out you end up with a zero
MAC address. Instead preserve the original random MAC
address and use it.

If you manually set the bridge address that will always
be respected. This change only takes effect if at the time
of computing the new root port we determine we have found
no candidates.

Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: bridge@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xenproject.org
Cc: kvm@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 net/bridge/br_device.c  | 1 +
 net/bridge/br_private.h | 1 +
 net/bridge/br_stp_if.c  | 3 +++
 3 files changed, 5 insertions(+)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index b063050..5f13eac 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -368,6 +368,7 @@ void br_dev_setup(struct net_device *dev)
 	br->bridge_id.prio[1] = 0x00;
 
 	ether_addr_copy(br->group_addr, eth_reserved_addr_base);
+	ether_addr_copy(br->random_init_addr, dev->dev_addr);
 
 	br->stp_enabled = BR_NO_STP;
 	br->group_fwd_mask = BR_GROUPFWD_DEFAULT;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index e1ca1dc..32a06da 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -240,6 +240,7 @@ struct net_bridge
 	unsigned long			bridge_hello_time;
 	unsigned long			bridge_forward_delay;
 
+	u8				random_init_addr[ETH_ALEN];
 	u8				group_addr[ETH_ALEN];
 	u16				root_port;
 
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 189ba1e..4c9ad45 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -239,6 +239,9 @@ bool br_stp_recalculate_bridge_id(struct net_bridge *br)
 	if (ether_addr_equal(br->bridge_id.addr, addr))
 		return false;	/* no change */
 
+	if (ether_addr_equal(addr, br_mac_zero))
+		addr = br->random_init_addr;
+
 	br_stp_change_bridge_id(br, addr);
 	return true;
 }
-- 
1.8.5.3

[PATCH 2/3] bridge: trigger a bridge calculation upon port changes

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

If netlink is used to tune a port we currently don't trigger a
new recalculation of the bridge id, ensure that happens just as
if we're adding a new net_device onto the bridge.

Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: bridge@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xenproject.org
Cc: kvm@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 net/bridge/br_netlink.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index e74b6d53..6f1b26d 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -364,6 +364,7 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh)
 	struct net_bridge_port *p;
 	struct nlattr *tb[IFLA_BRPORT_MAX + 1];
 	int err = 0;
+	bool changed;
 
 	protinfo = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_PROTINFO);
 	afspec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_AF_SPEC);
@@ -386,7 +387,12 @@ int br_setlink(struct net_device *dev, struct nlmsghdr *nlh)
 
 			spin_lock_bh(&p->br->lock);
 			err = br_setport(p, tb);
+			changed = br_stp_recalculate_bridge_id(p->br);
 			spin_unlock_bh(&p->br->lock);
+			if (changed)
+				call_netdevice_notifiers(NETDEV_CHANGEADDR,
+							 p->br->dev);
+			netdev_update_features(p->br->dev);
 		} else {
 			/* Binary compatibility with old RSTP */
 			if (nla_len(protinfo) < sizeof(u8))
-- 
1.8.5.3

[PATCH 3/3] bridge: fix bridge root block on designated port

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

Root port blocking was designed so that a bridge port can opt
out of becoming the designated root port for a bridge. If a port
however first becomes the designated root port and we then toggle
the root port block on it we currently don't kick that port out of
the designated root port. This fixes that. This is particularly
important for net_devices that would wish to never become a root
port from the start, currently toggling that off will enable root
port flag but it won't really kick the bridge and do what you'd
expect, the MAC address is kept on the bridge of the toggled port
for root_block if it was the designated port.

In order to catch if a port with root port block preference is set
we need to move our check for root block prior to the root selection
so check for root blocked ports upon eveyr br_configuration_update().
We also simply just prevent the root-blocked ports from consideration
as root port candidates on br_should_become_root_port() and
br_stp_recalculate_bridge_id().

The issue that this patch is trying to address and fix can be tested
easily before and after this patch is applied using 2 TAP net_devices
and then toggling at will with the root_block knob.

ip tuntap add dev tap0 mode tap
ip tuntap add dev tap1 mode tap
ip link add dev br0 type bridge
ip link show br0
echo -------------------------------------------
ip link set dev tap0 master br0
ip link
echo -------------------------------------------
ip link set dev tap1 master br0
ip link
echo -------------------------------------------

Upon review at the above results you can toggle root_block
on each port to see if you see the results you expect.

bridge link set dev tap0 root_block on
ip link
bridge link set dev tap1 root_block on
ip link

Toggling off root_block on any port should will bring back the
port to be a candidate for designated root port:

bridge link set dev tap1 root_block off
ip link
bridge link set dev tap0 root_block off
ip link

To nuke:

ip tuntap del tap0 mode tap
ip tuntap del tap0 mode tap

Cc: Stephen Hemminger <stephen@networkplumber.org>
Cc: bridge@lists.linux-foundation.org
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: xen-devel@lists.xenproject.org
Cc: kvm@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 net/bridge/br_netlink.c | 18 ++++++++++++
 net/bridge/br_private.h |  1 +
 net/bridge/br_stp.c     | 73 +++++++++++++++++++++++++++++++++++++++++++++----
 net/bridge/br_stp_if.c  |  3 +-
 4 files changed, 88 insertions(+), 7 deletions(-)

diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 6f1b26d..fbec354 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -324,6 +324,21 @@ static void br_set_port_flag(struct net_bridge_port *p, struct nlattr *tb[],
 	}
 }
 
+static void br_kick_bridge_port(struct net_bridge_port *p)
+{
+	struct net_bridge *br = p->br;
+	bool wasroot;
+
+	wasroot = br_is_root_bridge(br);
+	br_become_designated_port(p);
+
+	br_configuration_update(br);
+	br_port_state_selection(br);
+
+	if (br_is_root_bridge(br) && !wasroot)
+		br_become_root_bridge(br);
+}
+
 /* Process bridge protocol info on port */
 static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
 {
@@ -353,6 +368,9 @@ static int br_setport(struct net_bridge_port *p, struct nlattr *tb[])
 		if (err)
 			return err;
 	}
+
+	br_kick_bridge_port(p);
+
 	return 0;
 }
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 32a06da..45d7917 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -150,6 +150,7 @@ struct net_bridge_port
 	u8				priority;
 	u8				state;
 	u16				port_no;
+	bool				root_block_enabled;
 	unsigned char			topology_change_ack;
 	unsigned char			config_pending;
 	port_id				port_id;
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 3c86f05..f5741f3 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -59,6 +59,7 @@ static int br_should_become_root_port(const struct net_bridge_port *p,
 
 	br = p->br;
 	if (p->state == BR_STATE_DISABLED ||
+	   (p->flags & BR_ROOT_BLOCK) ||
 	    br_is_designated_port(p))
 		return 0;
 
@@ -104,7 +105,7 @@ static void br_root_port_block(const struct net_bridge *br,
 			       struct net_bridge_port *p)
 {
 
-	br_notice(br, "port %u(%s) tried to become root port (blocked)",
+	br_notice(br, "port %u (%s) is now root blocked",
 		  (unsigned int) p->port_no, p->dev->name);
 
 	p->state = BR_STATE_LISTENING;
@@ -124,11 +125,7 @@ static void br_root_selection(struct net_bridge *br)
 	list_for_each_entry(p, &br->port_list, list) {
 		if (!br_should_become_root_port(p, root_port))
 			continue;
-
-		if (p->flags & BR_ROOT_BLOCK)
-			br_root_port_block(br, p);
-		else
-			root_port = p->port_no;
+		root_port = p->port_no;
 	}
 
 	br->root_port = root_port;
@@ -358,10 +355,74 @@ static void br_reply(struct net_bridge_port *p)
 	br_transmit_config(p);
 }
 
+static bool br_root_port_block_check(struct net_bridge_port *p)
+{
+	bool designated = false;
+
+	if (p->root_block_enabled &&
+	    !(p->flags & BR_ROOT_BLOCK)) {
+		br_notice(p->br, "port %u (%s) is root block toggled off",
+			  (unsigned int) p->port_no, p->dev->name);
+		return false;
+	}
+
+	if (p->flags & BR_ROOT_BLOCK &&
+	    (p->root_block_enabled))
+		return false;
+
+	if (!(p->flags & BR_ROOT_BLOCK))
+		return false;
+
+	if (br_is_designated_port(p)) {
+		designated = true;
+		p->flags = BR_ROOT_BLOCK |
+			   BR_LEARNING |
+			   BR_FLOOD;
+		p->priority = 0x8000 >> BR_PORT_BITS;
+		br_init_port(p);
+	}
+
+	br_root_port_block(p->br, p);
+	p->root_block_enabled = true;
+
+	return designated;
+}
+
+/* Updates block ports as required and returns true if one was designated */
+static bool br_update_block_ports(struct net_bridge *br)
+{
+	struct net_bridge_port *p;
+	bool designated = false;
+
+	list_for_each_entry(p, &br->port_list, list) {
+		designated = br_root_port_block_check(p);
+		if (designated)
+			return designated;
+	}
+
+	return false;
+}
+
+static void br_update_designated_port(struct net_bridge *br)
+{
+	struct net_bridge_port *p;
+
+	list_for_each_entry(p, &br->port_list, list)
+		br_become_designated_port(p);
+}
+
 /* called under bridge lock */
 void br_configuration_update(struct net_bridge *br)
 {
+	bool designated_was_blocked = false;
+
+	designated_was_blocked = br_update_block_ports(br);
+
 	br_root_selection(br);
+
+	if (designated_was_blocked)
+		br_update_designated_port(br);
+
 	br_designated_port_selection(br);
 }
 
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 4c9ad45..62b84af 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -230,10 +230,11 @@ bool br_stp_recalculate_bridge_id(struct net_bridge *br)
 		return false;
 
 	list_for_each_entry(p, &br->port_list, list) {
+		if (p->flags & BR_ROOT_BLOCK)
+			continue;
 		if (addr == br_mac_zero ||
 		    memcmp(p->dev->dev_addr, addr, ETH_ALEN) < 0)
 			addr = p->dev->dev_addr;
-
 	}
 
 	if (ether_addr_equal(br->bridge_id.addr, addr))
-- 
1.8.5.3

Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes

[Cong Wang]

On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
>                         spin_lock_bh(&p->br->lock);
>                         err = br_setport(p, tb);
> +                       changed = br_stp_recalculate_bridge_id(p->br);

Looks like you only want to check if the mac addr gets changed here,
but br_stp_recalculate_bridge_id() does more than just checking it,
are you sure the side-effects are all what you want here?

>                         spin_unlock_bh(&p->br->lock);
> +                       if (changed)
> +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
> +                                                        p->br->dev);
> +                       netdev_update_features(p->br->dev);

I think this is supposed to be in netdev event handler of br->dev
instead of here.

Re: [PATCH 3/3] bridge: fix bridge root block on designated port

[Stephen Hemminger]

On Wed, 12 Mar 2014 20:15:27 -0700
"Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:

> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -150,6 +150,7 @@ struct net_bridge_port
>  	u8				priority;
>  	u8				state;
>  	u16				port_no;
> +	bool				root_block_enabled;
>  	unsigned char			topology_change_ack;

It seems a bit confusing to have both a ROOT_BLOCK flag in the
data structure and and additional root_block_enabled flag.
If nothing else it is a waste of space.

Looks like you are changing the meaning slightly. is possible
to have BR_ROOT_BLOCK set but !root_block_enabled? and what about
the inverse?

Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes

[Luis R. Rodriguez]

[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]

On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
> <mcgrof@do-not-panic.com> wrote:
> >                         spin_lock_bh(&p->br->lock);
> >                         err = br_setport(p, tb);
> > +                       changed = br_stp_recalculate_bridge_id(p->br);
> 
> Looks like you only want to check if the mac addr gets changed here,

Nope, the reason why we want a full thorough check is that br_setport()
may change currently any of these:
                                                                                
  * IFLA_BRPORT_MODE
  * IFLA_BRPORT_GUARD
  * IFLA_BRPORT_FAST_LEAVE
  * IFLA_BRPORT_PROTECT
  * IFLA_BRPORT_LEARNING,
  * IFLA_BRPORT_UNICAST_FLOOD
  * IFLA_BRPORT_COST
  * IFLA_BRPORT_PRIORITY
  * IFLA_BRPORT_STATE

That's good reason to trigger a good inspection. Having the MAC address
change would be simply collateral and its just something we need to do
some additional work for outside of the locking context.

> but br_stp_recalculate_bridge_id() does more than just checking it,
> are you sure the side-effects are all what you want here?

Yeap.

> >                         spin_unlock_bh(&p->br->lock);
> > +                       if (changed)
> > +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
> > +                                                        p->br->dev);
> > +                       netdev_update_features(p->br->dev);
> 
> I think this is supposed to be in netdev event handler of br->dev
> instead of here.

Do you mean netdev_update_features() ? I mimic'd what was being done on
br_del_if() given that root blocking is doing something similar. If
we need to change something for the above then I suppose it means we need
to change br_del_if() too. Let me know if you see any reason for something
else.

  Luis

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 3/3] bridge: fix bridge root block on designated port

[Luis R. Rodriguez]

[-- Attachment #1: Type: text/plain, Size: 1596 bytes --]

On Thu, Mar 13, 2014 at 03:16:23PM -0700, Stephen Hemminger wrote:
> On Wed, 12 Mar 2014 20:15:27 -0700
> "Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:
> 
> > --- a/net/bridge/br_private.h
> > +++ b/net/bridge/br_private.h
> > @@ -150,6 +150,7 @@ struct net_bridge_port
> >  	u8				priority;
> >  	u8				state;
> >  	u16				port_no;
> > +	bool				root_block_enabled;
> >  	unsigned char			topology_change_ack;
> 
> It seems a bit confusing to have both a ROOT_BLOCK flag in the
> data structure and and additional root_block_enabled flag.
> If nothing else it is a waste of space.

Indeed, however there is a use for it. Consider the case where we loop
over each port and check to see if its root blocked and need to tickle it
or the bridge. In the case that root port block was enabled before and
someone is lifting it the flag would be removed and therefore not on
but it was root blocked though and we need a way to keep track of that.

The flag then is a toggle for userspace, while the bool tells us about
the current state.

> Looks like you are changing the meaning slightly. 

Let me know in what way. I can't see it.

> is possible to have BR_ROOT_BLOCK set but !root_block_enabled? 

Yeah in the case a new request to set it to root block then
BR_ROOT_BLOCK would be set but root_block_enabled would not be set.

> and what about the inverse?

BR_ROOT_BLOCK would not be set when userspace wants to disable root
port block and root_block_enabled would be enabled in this case if
it used to be enabled. So yes, both are possible.

  Luis

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 0/3] bridge: few enhancements and small fixes

[Luis R. Rodriguez]

On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> Here's a few fixes I've been carrying around. I've now tested them
> on as many systems / environments as I can. They should be ready.
>
> Luis R. Rodriguez (3):
>   bridge: preserve random init MAC address
>   bridge: trigger a bridge calculation upon port changes
>   bridge: fix bridge root block on designated port

Folks, let me know if there are any other questions or if you spot any
other possible issues.

  Luis

Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes

[Cong Wang]

On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
>> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
>> <mcgrof@do-not-panic.com> wrote:
>> >                         spin_unlock_bh(&p->br->lock);
>> > +                       if (changed)
>> > +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
>> > +                                                        p->br->dev);
>> > +                       netdev_update_features(p->br->dev);
>>
>> I think this is supposed to be in netdev event handler of br->dev
>> instead of here.
>
> Do you mean netdev_update_features() ? I mimic'd what was being done on
> br_del_if() given that root blocking is doing something similar. If
> we need to change something for the above then I suppose it means we need
> to change br_del_if() too. Let me know if you see any reason for something
> else.
>

Yeah, for me it looks like it's better to call netdev_update_features()
in the event handler of br->dev, rather than where calling
call_netdevice_notifiers(..., br->dev);.

Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes

[Luis R. Rodriguez]

[-- Attachment #1: Type: text/plain, Size: 2241 bytes --]

On Tue, Mar 18, 2014 at 01:46:49PM -0700, Cong Wang wrote:
> On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
> >> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
> >> <mcgrof@do-not-panic.com> wrote:
> >> >                         spin_unlock_bh(&p->br->lock);
> >> > +                       if (changed)
> >> > +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
> >> > +                                                        p->br->dev);
> >> > +                       netdev_update_features(p->br->dev);
> >>
> >> I think this is supposed to be in netdev event handler of br->dev
> >> instead of here.
> >
> > Do you mean netdev_update_features() ? I mimic'd what was being done on
> > br_del_if() given that root blocking is doing something similar. If
> > we need to change something for the above then I suppose it means we need
> > to change br_del_if() too. Let me know if you see any reason for something
> > else.
> >
> 
> Yeah, for me it looks like it's better to call netdev_update_features()
> in the event handler of br->dev, rather than where calling
> call_netdevice_notifiers(..., br->dev);.

I still don't see why, in fact trying to verify this I am wondering now
if instead we should actually fix br_features_recompute() to take into
consideration BR_ROOT_BLOCK as below. Notice how netdev_update_features()
is called above even if the MAC address did not change, just as is done
on br_del_if(). There is an NETDEV_FEAT_CHANGE event so would it be more
appropriate we just call

call_netdevice_notifiers(NETDEV_FEAT_CHANGE, p->br->dev)

for both the above then and also br_del_if()? How about the below
change?

diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 54d207d..dcd9378 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -315,6 +315,8 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
 	features &= ~NETIF_F_ONE_FOR_ALL;
 
 	list_for_each_entry(p, &br->port_list, list) {
+		if (p->flags & BR_ROOT_BLOCK)
+			continue;
 		features = netdev_increment_features(features,
 						     p->dev->features, mask);
 	}

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] bridge: preserve random init MAC address

[Toshiaki Makita]

(2014/03/13 12:15), Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> As it is now if you add create a bridge it gets started
> with a random MAC address and if you then add a net_device
> as a slave but later kick it out you end up with a zero
> MAC address. Instead preserve the original random MAC
> address and use it.
> 
> If you manually set the bridge address that will always
> be respected. This change only takes effect if at the time
> of computing the new root port we determine we have found
> no candidates.
> 
> Cc: Stephen Hemminger <stephen@networkplumber.org>
> Cc: bridge@lists.linux-foundation.org
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: xen-devel@lists.xenproject.org
> Cc: kvm@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>  net/bridge/br_device.c  | 1 +
>  net/bridge/br_private.h | 1 +
>  net/bridge/br_stp_if.c  | 3 +++
>  3 files changed, 5 insertions(+)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index b063050..5f13eac 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -368,6 +368,7 @@ void br_dev_setup(struct net_device *dev)
>  	br->bridge_id.prio[1] = 0x00;
>  
>  	ether_addr_copy(br->group_addr, eth_reserved_addr_base);
> +	ether_addr_copy(br->random_init_addr, dev->dev_addr);
>  
>  	br->stp_enabled = BR_NO_STP;
>  	br->group_fwd_mask = BR_GROUPFWD_DEFAULT;
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index e1ca1dc..32a06da 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -240,6 +240,7 @@ struct net_bridge
>  	unsigned long			bridge_hello_time;
>  	unsigned long			bridge_forward_delay;
>  
> +	u8				random_init_addr[ETH_ALEN];
>  	u8				group_addr[ETH_ALEN];
>  	u16				root_port;
>  
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index 189ba1e..4c9ad45 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -239,6 +239,9 @@ bool br_stp_recalculate_bridge_id(struct net_bridge *br)
>  	if (ether_addr_equal(br->bridge_id.addr, addr))
>  		return false;	/* no change */
>  
> +	if (ether_addr_equal(addr, br_mac_zero))
> +		addr = br->random_init_addr;
> +
>  	br_stp_change_bridge_id(br, addr);
>  	return true;
>  }

nit,
If the last detached port happens to have the same addr as
random_init_addr, this seems to call br_stp_change_bridge_id() even
though bridge_id is not changed.

Shouldn't the assignment of random_init_addr be done before the check of
"no change"?

Toshiaki Makita

Re: [PATCH 1/3] bridge: preserve random init MAC address

[Luis R. Rodriguez]

On Tue, Mar 18, 2014 at 5:42 PM, Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
> nit,
> If the last detached port happens to have the same addr as
> random_init_addr, this seems to call br_stp_change_bridge_id() even
> though bridge_id is not changed.

Ah good point.

> Shouldn't the assignment of random_init_addr be done before the check of
> "no change"?

Good question, should we even allow two ports to have the same MAC
address or should we complain and refuse to add it? If so that should
mean we should also have to monitor any manual address changes or
events for address changes on the ports.

Stephen?

  Luis

Re: [PATCH 1/3] bridge: preserve random init MAC address

[Toshiaki Makita]

(2014/03/19 9:50), Luis R. Rodriguez wrote:
> On Tue, Mar 18, 2014 at 5:42 PM, Toshiaki Makita
> <makita.toshiaki@lab.ntt.co.jp> wrote:
>> nit,
>> If the last detached port happens to have the same addr as
>> random_init_addr, this seems to call br_stp_change_bridge_id() even
>> though bridge_id is not changed.
> 
> Ah good point.
> 
>> Shouldn't the assignment of random_init_addr be done before the check of
>> "no change"?
> 
> Good question, should we even allow two ports to have the same MAC
> address or should we complain and refuse to add it? If so that should
> mean we should also have to monitor any manual address changes or
> events for address changes on the ports.

This was recently discussed by Stephen and me.
I'm thinking it should be allowed.

http://marc.info/?l=linux-netdev&m=139182743919257&w=2

Toshiaki Makita

> 
> Stephen?
> 
>   Luis

Re: [PATCH 1/3] bridge: preserve random init MAC address

[Luis R. Rodriguez]

On Tue, Mar 18, 2014 at 6:04 PM, Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
> (2014/03/19 9:50), Luis R. Rodriguez wrote:
>> On Tue, Mar 18, 2014 at 5:42 PM, Toshiaki Makita
>> <makita.toshiaki@lab.ntt.co.jp> wrote:
>>> nit,
>>> If the last detached port happens to have the same addr as
>>> random_init_addr, this seems to call br_stp_change_bridge_id() even
>>> though bridge_id is not changed.
>>
>> Ah good point.
>>
>>> Shouldn't the assignment of random_init_addr be done before the check of
>>> "no change"?
>>
>> Good question, should we even allow two ports to have the same MAC
>> address or should we complain and refuse to add it? If so that should
>> mean we should also have to monitor any manual address changes or
>> events for address changes on the ports.
>
> This was recently discussed by Stephen and me.
> I'm thinking it should be allowed.
>
> http://marc.info/?l=linux-netdev&m=139182743919257&w=2

Great now that that's sorted out though I still think calling
br_stp_change_bridge_id() is right just as calling the update features
as the device is different. It could however be confusing when this
situation is run and folks might report odd bugs unless we could tell
them apart clearly. Thoughts?

  Luis

Re: [PATCH 1/3] bridge: preserve random init MAC address

[Stephen Hemminger]

On Wed, 12 Mar 2014 20:15:25 -0700
"Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:

> As it is now if you add create a bridge it gets started
> with a random MAC address and if you then add a net_device
> as a slave but later kick it out you end up with a zero
> MAC address. Instead preserve the original random MAC
> address and use it.

What is supposed to happen is that the recalculate chooses
the lowest MAC address of the slaves. If there are no slaves
it might as well just calculate a new random value. There is
not great merit in preserving the original defunct address.

Or something like this that just keeps the old value.
The bridge is in a meaningless state when there are no ports,
and when the first port is added back it will be used as the
new bridge id.

--- a/net/bridge/br_stp_if.c	2014-02-12 08:21:56.733857356 -0800
+++ b/net/bridge/br_stp_if.c	2014-03-18 20:09:09.334388826 -0700
@@ -235,6 +235,9 @@ bool br_stp_recalculate_bridge_id(struct
 			addr = p->dev->dev_addr;
 
 	}
+
+	if (addr == br_mac_zero)
+		return false;  /* keep original address */
 
 	if (ether_addr_equal(br->bridge_id.addr, addr))
 		return false;	/* no change */

Re: [PATCH 1/3] bridge: preserve random init MAC address

[Luis R. Rodriguez]

On Tue, Mar 18, 2014 at 8:10 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Wed, 12 Mar 2014 20:15:25 -0700
> "Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:
>
>> As it is now if you add create a bridge it gets started
>> with a random MAC address and if you then add a net_device
>> as a slave but later kick it out you end up with a zero
>> MAC address. Instead preserve the original random MAC
>> address and use it.
>
> What is supposed to happen is that the recalculate chooses
> the lowest MAC address of the slaves. If there are no slaves
> it might as well just calculate a new random value. There is
> not great merit in preserving the original defunct address.

OK I'll go and add some changes for this then.

  Luis

Re: [Bridge] [PATCH 1/3] bridge: preserve random init MAC address

[Toshiaki Makita]

On Tue, 2014-03-18 at 18:10 -0700, Luis R. Rodriguez wrote:
> On Tue, Mar 18, 2014 at 6:04 PM, Toshiaki Makita
> <makita.toshiaki@lab.ntt.co.jp> wrote:
> > (2014/03/19 9:50), Luis R. Rodriguez wrote:
> >> On Tue, Mar 18, 2014 at 5:42 PM, Toshiaki Makita
> >> <makita.toshiaki@lab.ntt.co.jp> wrote:
> >>> nit,
> >>> If the last detached port happens to have the same addr as
> >>> random_init_addr, this seems to call br_stp_change_bridge_id() even
> >>> though bridge_id is not changed.
> >>
> >> Ah good point.
> >>
> >>> Shouldn't the assignment of random_init_addr be done before the check of
> >>> "no change"?
> >>
> >> Good question, should we even allow two ports to have the same MAC
> >> address or should we complain and refuse to add it? If so that should
> >> mean we should also have to monitor any manual address changes or
> >> events for address changes on the ports.
> >
> > This was recently discussed by Stephen and me.
> > I'm thinking it should be allowed.
> >
> > http://marc.info/?l=linux-netdev&m=139182743919257&w=2
> 
> Great now that that's sorted out though I still think calling
> br_stp_change_bridge_id() is right just as calling the update features
> as the device is different. It could however be confusing when this
> situation is run and folks might report odd bugs unless we could tell
> them apart clearly. Thoughts?

br_stp_change_bridge_id() is currently called only if bridge_id.addr
should be changed.
If the addr should not be changed but some updates are needed,
br_stp_recalculate_bridge_id() doesn't seem to fit into it.

Toshiaki Makita

Re: [PATCH 1/3] bridge: preserve random init MAC address

[Luis R. Rodriguez]

[-- Attachment #1: Type: text/plain, Size: 2356 bytes --]

On Tue, Mar 18, 2014 at 08:10:56PM -0700, Stephen Hemminger wrote:
> On Wed, 12 Mar 2014 20:15:25 -0700
> "Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:
> 
> > As it is now if you add create a bridge it gets started
> > with a random MAC address and if you then add a net_device
> > as a slave but later kick it out you end up with a zero
> > MAC address. Instead preserve the original random MAC
> > address and use it.
> 
> What is supposed to happen is that the recalculate chooses
> the lowest MAC address of the slaves. If there are no slaves
> it might as well just calculate a new random value. There is
> not great merit in preserving the original defunct address.
> 
> Or something like this
> --- a/net/bridge/br_stp_if.c	2014-02-12 08:21:56.733857356 -0800
> +++ b/net/bridge/br_stp_if.c	2014-03-18 20:09:09.334388826 -0700
> @@ -235,6 +235,9 @@ bool br_stp_recalculate_bridge_id(struct
>  			addr = p->dev->dev_addr;
>  
>  	}
> +
> +	if (addr == br_mac_zero)
> +		return false;  /* keep original address */
>  
>  	if (ether_addr_equal(br->bridge_id.addr, addr))
>  		return false;	/* no change */
> 
> that just keeps the old value.

The old value could be a port which got root blocked, I think
it can be confusing to see that happen. Either way feel free to
make the call, I'll provide more details below on perhaps one reason
to keep the original MAC address.

> The bridge is in a meaningless state when there are no ports,

Some virtualization topologies may want a backend with no link (or
perhaps one which is dynamic, with the option to have none) to the
internet but just a bridge so guests sharing the bridge can talk to 
each other. In this case bridging can be used only to link the
batch of guests.

In this case the bridge simply acts as a switch, but also can be used as the
interface for DHCP, for example. In such a case the guests will be doing
ARP to get to the DHCP server. There's a flurry of ways one can try to get
all this meshed together including enablign an ARP proxy but I'm looking
at ways to optimize this further -- but I'd like to address the current
usage cases first.

> and when the first port is added back it will be used as the
> new bridge id.

Sure. Let me know how you think we should proceed with this patch based
on the above.

  Luis

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH 1/3] bridge: preserve random init MAC address

[Luis R. Rodriguez]

On Wed, Mar 19, 2014 at 7:05 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Tue, Mar 18, 2014 at 08:10:56PM -0700, Stephen Hemminger wrote:
>> On Wed, 12 Mar 2014 20:15:25 -0700
>> "Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:
>>
>> > As it is now if you add create a bridge it gets started
>> > with a random MAC address and if you then add a net_device
>> > as a slave but later kick it out you end up with a zero
>> > MAC address. Instead preserve the original random MAC
>> > address and use it.
>>
>> What is supposed to happen is that the recalculate chooses
>> the lowest MAC address of the slaves. If there are no slaves
>> it might as well just calculate a new random value. There is
>> not great merit in preserving the original defunct address.
>>
>> Or something like this
>> --- a/net/bridge/br_stp_if.c  2014-02-12 08:21:56.733857356 -0800
>> +++ b/net/bridge/br_stp_if.c  2014-03-18 20:09:09.334388826 -0700
>> @@ -235,6 +235,9 @@ bool br_stp_recalculate_bridge_id(struct
>>                       addr = p->dev->dev_addr;
>>
>>       }
>> +
>> +     if (addr == br_mac_zero)
>> +             return false;  /* keep original address */
>>
>>       if (ether_addr_equal(br->bridge_id.addr, addr))
>>               return false;   /* no change */
>>
>> that just keeps the old value.
>
> The old value could be a port which got root blocked, I think
> it can be confusing to see that happen. Either way feel free to
> make the call, I'll provide more details below on perhaps one reason
> to keep the original MAC address.

Stephen, I'd like to respin this series to address all pending
feedback, I'd still like your feedback / call / judgement on this
part. I'm fine either way, just wanted to ensure I highlight the
reasoning of why I kept the original random MAC address. Please keep
in mind that at this point I'm convinced bridging is the *wrong*
solution to networking with guests but it is being used in a lot of
current topologies, this would just help with smoothing out corner
cases.

>> The bridge is in a meaningless state when there are no ports,
>
> Some virtualization topologies may want a backend with no link (or
> perhaps one which is dynamic, with the option to have none) to the
> internet but just a bridge so guests sharing the bridge can talk to
> each other. In this case bridging can be used only to link the
> batch of guests.
>
> In this case the bridge simply acts as a switch, but also can be used as the
> interface for DHCP, for example. In such a case the guests will be doing
> ARP to get to the DHCP server. There's a flurry of ways one can try to get
> all this meshed together including enablign an ARP proxy but I'm looking
> at ways to optimize this further -- but I'd like to address the current
> usage cases first.
>
>> and when the first port is added back it will be used as the
>> new bridge id.
>
> Sure. Let me know how you think we should proceed with this patch based
> on the above.

Thanks in advance.

  Luis

Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes

[Luis R. Rodriguez]

On Tue, Mar 18, 2014 at 02:22:43PM -0700, Luis R. Rodriguez wrote:
> On Tue, Mar 18, 2014 at 01:46:49PM -0700, Cong Wang wrote:
> > On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> > > On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
> > >> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
> > >> <mcgrof@do-not-panic.com> wrote:
> > >> >                         spin_unlock_bh(&p->br->lock);
> > >> > +                       if (changed)
> > >> > +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
> > >> > +                                                        p->br->dev);
> > >> > +                       netdev_update_features(p->br->dev);
> > >>
> > >> I think this is supposed to be in netdev event handler of br->dev
> > >> instead of here.
> > >
> > > Do you mean netdev_update_features() ? I mimic'd what was being done on
> > > br_del_if() given that root blocking is doing something similar. If
> > > we need to change something for the above then I suppose it means we need
> > > to change br_del_if() too. Let me know if you see any reason for something
> > > else.
> > >
> > 
> > Yeah, for me it looks like it's better to call netdev_update_features()
> > in the event handler of br->dev, rather than where calling
> > call_netdevice_notifiers(..., br->dev);.
> 
> I still don't see why, in fact trying to verify this I am wondering now
> if instead we should actually fix br_features_recompute() to take into
> consideration BR_ROOT_BLOCK as below. Notice how netdev_update_features()
> is called above even if the MAC address did not change, just as is done
> on br_del_if(). There is an NETDEV_FEAT_CHANGE event so would it be more
> appropriate we just call
> 
> call_netdevice_notifiers(NETDEV_FEAT_CHANGE, p->br->dev)
> 
> for both the above then and also br_del_if()? How about the below
> change?
> 
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index 54d207d..dcd9378 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -315,6 +315,8 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
>  	features &= ~NETIF_F_ONE_FOR_ALL;
>  
>  	list_for_each_entry(p, &br->port_list, list) {
> +		if (p->flags & BR_ROOT_BLOCK)
> +			continue;
>  		features = netdev_increment_features(features,
>  						     p->dev->features, mask);
>  	}

Cong, can you provide feedback on this? I tried to grow confidence on the
hunk above but its not clear but the other points still hold and I'd love
your feedback on those.

  Luis

Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes

[Luis R. Rodriguez]

On Tue, Apr 22, 2014 at 12:43 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Tue, Mar 18, 2014 at 02:22:43PM -0700, Luis R. Rodriguez wrote:
>> On Tue, Mar 18, 2014 at 01:46:49PM -0700, Cong Wang wrote:
>> > On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> > > On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
>> > >> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
>> > >> <mcgrof@do-not-panic.com> wrote:
>> > >> >                         spin_unlock_bh(&p->br->lock);
>> > >> > +                       if (changed)
>> > >> > +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
>> > >> > +                                                        p->br->dev);
>> > >> > +                       netdev_update_features(p->br->dev);
>> > >>
>> > >> I think this is supposed to be in netdev event handler of br->dev
>> > >> instead of here.
>> > >
>> > > Do you mean netdev_update_features() ? I mimic'd what was being done on
>> > > br_del_if() given that root blocking is doing something similar. If
>> > > we need to change something for the above then I suppose it means we need
>> > > to change br_del_if() too. Let me know if you see any reason for something
>> > > else.
>> > >
>> >
>> > Yeah, for me it looks like it's better to call netdev_update_features()
>> > in the event handler of br->dev, rather than where calling
>> > call_netdevice_notifiers(..., br->dev);.
>>
>> I still don't see why, in fact trying to verify this I am wondering now
>> if instead we should actually fix br_features_recompute() to take into
>> consideration BR_ROOT_BLOCK as below. Notice how netdev_update_features()
>> is called above even if the MAC address did not change, just as is done
>> on br_del_if(). There is an NETDEV_FEAT_CHANGE event so would it be more
>> appropriate we just call
>>
>> call_netdevice_notifiers(NETDEV_FEAT_CHANGE, p->br->dev)
>>
>> for both the above then and also br_del_if()? How about the below
>> change?
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index 54d207d..dcd9378 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -315,6 +315,8 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
>>       features &= ~NETIF_F_ONE_FOR_ALL;
>>
>>       list_for_each_entry(p, &br->port_list, list) {
>> +             if (p->flags & BR_ROOT_BLOCK)
>> +                     continue;
>>               features = netdev_increment_features(features,
>>                                                    p->dev->features, mask);
>>       }
>
> Cong, can you provide feedback on this? I tried to grow confidence on the
> hunk above but its not clear but the other points still hold and I'd love
> your feedback on those.

Re-poke.

  Luis

Re: [PATCH 1/3] bridge: preserve random init MAC address

[Luis R. Rodriguez]

On Tue, Apr 22, 2014 at 12:41 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> Stephen, I'd like to respin this series to address all pending
> feedback, I'd still like your feedback / call / judgement on this
> part. I'm fine either way, just wanted to ensure I highlight the
> reasoning of why I kept the original random MAC address.

Re-poke.

  Luis

Re: [PATCH 1/3] bridge: preserve random init MAC address

[Vlad Yasevich]

On 04/22/2014 03:41 PM, Luis R. Rodriguez wrote:
> On Wed, Mar 19, 2014 at 7:05 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> On Tue, Mar 18, 2014 at 08:10:56PM -0700, Stephen Hemminger wrote:
>>> On Wed, 12 Mar 2014 20:15:25 -0700
>>> "Luis R. Rodriguez" <mcgrof@do-not-panic.com> wrote:
>>>
>>>> As it is now if you add create a bridge it gets started
>>>> with a random MAC address and if you then add a net_device
>>>> as a slave but later kick it out you end up with a zero
>>>> MAC address. Instead preserve the original random MAC
>>>> address and use it.
>>>
>>> What is supposed to happen is that the recalculate chooses
>>> the lowest MAC address of the slaves. If there are no slaves
>>> it might as well just calculate a new random value. There is
>>> not great merit in preserving the original defunct address.
>>>
>>> Or something like this
>>> --- a/net/bridge/br_stp_if.c  2014-02-12 08:21:56.733857356 -0800
>>> +++ b/net/bridge/br_stp_if.c  2014-03-18 20:09:09.334388826 -0700
>>> @@ -235,6 +235,9 @@ bool br_stp_recalculate_bridge_id(struct
>>>                       addr = p->dev->dev_addr;
>>>
>>>       }
>>> +
>>> +     if (addr == br_mac_zero)
>>> +             return false;  /* keep original address */
>>>
>>>       if (ether_addr_equal(br->bridge_id.addr, addr))
>>>               return false;   /* no change */
>>>
>>> that just keeps the old value.
>>
>> The old value could be a port which got root blocked, I think
>> it can be confusing to see that happen. Either way feel free to
>> make the call, I'll provide more details below on perhaps one reason
>> to keep the original MAC address.
> 
> Stephen, I'd like to respin this series to address all pending
> feedback, I'd still like your feedback / call / judgement on this
> part. I'm fine either way, just wanted to ensure I highlight the
> reasoning of why I kept the original random MAC address. Please keep
> in mind that at this point I'm convinced bridging is the *wrong*
> solution to networking with guests but it is being used in a lot of
> current topologies, this would just help with smoothing out corner
> cases.
> 

Hi Luis

I took at this series again, I think it might make to generate
a new random address instead of storing the original.
There is not much point in storing the original since it's been
gone, and most likely not advertised anywhere anyway.

As for virtualization configuration you described, a lot of times
there is a single port in the down state that remains.  At least
that's how libvirt manages it's bridged networks.

Thanks
-vlad

>>> The bridge is in a meaningless state when there are no ports,
>>
>> Some virtualization topologies may want a backend with no link (or
>> perhaps one which is dynamic, with the option to have none) to the
>> internet but just a bridge so guests sharing the bridge can talk to
>> each other. In this case bridging can be used only to link the
>> batch of guests.
>>
>> In this case the bridge simply acts as a switch, but also can be used as the
>> interface for DHCP, for example. In such a case the guests will be doing
>> ARP to get to the DHCP server. There's a flurry of ways one can try to get
>> all this meshed together including enablign an ARP proxy but I'm looking
>> at ways to optimize this further -- but I'd like to address the current
>> usage cases first.
>>
>>> and when the first port is added back it will be used as the
>>> new bridge id.
>>
>> Sure. Let me know how you think we should proceed with this patch based
>> on the above.
> 
> Thanks in advance.
> 
>   Luis
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes

[Vlad Yasevich]

On 04/22/2014 03:43 PM, Luis R. Rodriguez wrote:
> On Tue, Mar 18, 2014 at 02:22:43PM -0700, Luis R. Rodriguez wrote:
>> On Tue, Mar 18, 2014 at 01:46:49PM -0700, Cong Wang wrote:
>>> On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>>> On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
>>>>> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
>>>>> <mcgrof@do-not-panic.com> wrote:
>>>>>>                         spin_unlock_bh(&p->br->lock);
>>>>>> +                       if (changed)
>>>>>> +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
>>>>>> +                                                        p->br->dev);
>>>>>> +                       netdev_update_features(p->br->dev);
>>>>>
>>>>> I think this is supposed to be in netdev event handler of br->dev
>>>>> instead of here.
>>>>
>>>> Do you mean netdev_update_features() ? I mimic'd what was being done on
>>>> br_del_if() given that root blocking is doing something similar. If
>>>> we need to change something for the above then I suppose it means we need
>>>> to change br_del_if() too. Let me know if you see any reason for something
>>>> else.
>>>>
>>>
>>> Yeah, for me it looks like it's better to call netdev_update_features()
>>> in the event handler of br->dev, rather than where calling
>>> call_netdevice_notifiers(..., br->dev);.
>>
>> I still don't see why, in fact trying to verify this I am wondering now
>> if instead we should actually fix br_features_recompute() to take into
>> consideration BR_ROOT_BLOCK as below. Notice how netdev_update_features()
>> is called above even if the MAC address did not change, just as is done
>> on br_del_if(). There is an NETDEV_FEAT_CHANGE event so would it be more
>> appropriate we just call
>>
>> call_netdevice_notifiers(NETDEV_FEAT_CHANGE, p->br->dev)
>>
>> for both the above then and also br_del_if()? How about the below
>> change?
>>
>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>> index 54d207d..dcd9378 100644
>> --- a/net/bridge/br_if.c
>> +++ b/net/bridge/br_if.c
>> @@ -315,6 +315,8 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
>>  	features &= ~NETIF_F_ONE_FOR_ALL;
>>  
>>  	list_for_each_entry(p, &br->port_list, list) {
>> +		if (p->flags & BR_ROOT_BLOCK)
>> +			continue;
>>  		features = netdev_increment_features(features,
>>  						     p->dev->features, mask);
>>  	}
> 
> Cong, can you provide feedback on this? I tried to grow confidence on the
> hunk above but its not clear but the other points still hold and I'd love
> your feedback on those.
> 

Hi Luis

The hunk above isn't right.  Just because you set ROOT_BLOCK on the port
doesn't mean that you should ignore it's device features.  If the device
you just added happens to disable or enable some device offload feature,
you should take that into account.

Thanks
-vlad

>   Luis
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes

[Luis R. Rodriguez]

On Wed, Apr 30, 2014 at 04:04:34PM -0400, Vlad Yasevich wrote:
> On 04/22/2014 03:43 PM, Luis R. Rodriguez wrote:
> > diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> > index 54d207d..dcd9378 100644
> > --- a/net/bridge/br_if.c
> > +++ b/net/bridge/br_if.c
> > @@ -315,6 +315,8 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
> >  	features &= ~NETIF_F_ONE_FOR_ALL;
> >  
> >  	list_for_each_entry(p, &br->port_list, list) {
> > +		if (p->flags & BR_ROOT_BLOCK)
> > +			continue;
> >  		features = netdev_increment_features(features,
> >  						     p->dev->features, mask);
> >  	}
> >
> Hi Luis
> 
> The hunk above isn't right.  Just because you set ROOT_BLOCK on the port
> doesn't mean that you should ignore it's device features.  If the device
> you just added happens to disable or enable some device offload feature,
> you should take that into account.

OK thanks, how about this part:

On 04/22/2014 03:43 PM, Luis R. Rodriguez wrote:
> On Tue, Mar 18, 2014 at 02:22:43PM -0700, Luis R. Rodriguez wrote:
> On Tue, Mar 18, 2014 at 01:46:49PM -0700, Cong Wang wrote:
>> On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>> On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
>>>> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
>>>> <mcgrof@do-not-panic.com> wrote:
>>>>>                         spin_unlock_bh(&p->br->lock);
>>>>> +                       if (changed)
>>>>> +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
>>>>> +                                                        p->br->dev);
>>>>> +                       netdev_update_features(p->br->dev);
>>>>
>>>> I think this is supposed to be in netdev event handler of br->dev
>>>> instead of here.
>>>
>>> Do you mean netdev_update_features() ? I mimic'd what was being done on
>>> br_del_if() given that root blocking is doing something similar. If
>>> we need to change something for the above then I suppose it means we need
>>> to change br_del_if() too. Let me know if you see any reason for something
>>> else.
>>>
>>
>> Yeah, for me it looks like it's better to call netdev_update_features()
>> in the event handler of br->dev, rather than where calling
>> call_netdevice_notifiers(..., br->dev);.
>
> I still don't see why, in fact trying to verify this I am wondering now
> if instead we should actually fix br_features_recompute() to take into
> consideration BR_ROOT_BLOCK as below. Notice how netdev_update_features()
> is called above even if the MAC address did not change, just as is done
> on br_del_if(). There is an NETDEV_FEAT_CHANGE event so would it be more
> appropriate we just call
>
> call_netdevice_notifiers(NETDEV_FEAT_CHANGE, p->br->dev)
>
> for both the above then and also br_del_if()?

  Luis

Re: [PATCH 2/3] bridge: trigger a bridge calculation upon port changes

[Vlad Yasevich]

On 04/30/2014 06:59 PM, Luis R. Rodriguez wrote:
> On Wed, Apr 30, 2014 at 04:04:34PM -0400, Vlad Yasevich wrote:
>> On 04/22/2014 03:43 PM, Luis R. Rodriguez wrote:
>>> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
>>> index 54d207d..dcd9378 100644
>>> --- a/net/bridge/br_if.c
>>> +++ b/net/bridge/br_if.c
>>> @@ -315,6 +315,8 @@ netdev_features_t br_features_recompute(struct net_bridge *br,
>>>  	features &= ~NETIF_F_ONE_FOR_ALL;
>>>  
>>>  	list_for_each_entry(p, &br->port_list, list) {
>>> +		if (p->flags & BR_ROOT_BLOCK)
>>> +			continue;
>>>  		features = netdev_increment_features(features,
>>>  						     p->dev->features, mask);
>>>  	}
>>>
>> Hi Luis
>>
>> The hunk above isn't right.  Just because you set ROOT_BLOCK on the port
>> doesn't mean that you should ignore it's device features.  If the device
>> you just added happens to disable or enable some device offload feature,
>> you should take that into account.
> 
> OK thanks, how about this part:
> 
> On 04/22/2014 03:43 PM, Luis R. Rodriguez wrote:
>> On Tue, Mar 18, 2014 at 02:22:43PM -0700, Luis R. Rodriguez wrote:
>> On Tue, Mar 18, 2014 at 01:46:49PM -0700, Cong Wang wrote:
>>> On Fri, Mar 14, 2014 at 6:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>>> On Thu, Mar 13, 2014 at 11:26:25AM -0700, Cong Wang wrote:
>>>>> On Wed, Mar 12, 2014 at 8:15 PM, Luis R. Rodriguez
>>>>> <mcgrof@do-not-panic.com> wrote:
>>>>>>                         spin_unlock_bh(&p->br->lock);
>>>>>> +                       if (changed)
>>>>>> +                               call_netdevice_notifiers(NETDEV_CHANGEADDR,
>>>>>> +                                                        p->br->dev);
>>>>>> +                       netdev_update_features(p->br->dev);
>>>>>

This is actually just a part of it.  You also need to handle the sysfs
changing the flag.

Look at the first 2 patches in this series:
http://www.spinics.net/lists/netdev/msg280863.html

You might need that functionality.

-vlad


>>>>> I think this is supposed to be in netdev event handler of br->dev
>>>>> instead of here.
>>>>
>>>> Do you mean netdev_update_features() ? I mimic'd what was being done on
>>>> br_del_if() given that root blocking is doing something similar. If
>>>> we need to change something for the above then I suppose it means we need
>>>> to change br_del_if() too. Let me know if you see any reason for something
>>>> else.
>>>>
>>>
>>> Yeah, for me it looks like it's better to call netdev_update_features()
>>> in the event handler of br->dev, rather than where calling
>>> call_netdevice_notifiers(..., br->dev);.
>>
>> I still don't see why, in fact trying to verify this I am wondering now
>> if instead we should actually fix br_features_recompute() to take into
>> consideration BR_ROOT_BLOCK as below. Notice how netdev_update_features()
>> is called above even if the MAC address did not change, just as is done
>> on br_del_if(). There is an NETDEV_FEAT_CHANGE event so would it be more
>> appropriate we just call
>>
>> call_netdevice_notifiers(NETDEV_FEAT_CHANGE, p->br->dev)
>>
>> for both the above then and also br_del_if()?
> 
>   Luis
>