[Bridge] [PATCH iproute2 v4] iplink: add support for STP xstats

[Vivien Didelot]

Add support for the BRIDGE_XSTATS_STP xstats, as follow:

# ip link xstats type bridge_slave dev lan4 stp
lan4
STP BPDU:  RX: 0 TX: 61
STP TCN:   RX: 0 TX: 0
STP Transitions: Blocked: 2 Forwarding: 1

Or below as JSON:

# ip -j -p link xstats type bridge_slave dev lan0 stp
[ {
"ifname": "lan0",
"stp": {
"rx_bpdu": 0,
"tx_bpdu": 500,
"rx_tcn": 0,
"tx_tcn": 0,
"transition_blk": 0,
    "transition_fwd": 0
}
} ]

Signed-off-by: Vivien Didelot 
---
 include/uapi/linux/if_bridge.h | 10 ++
 ip/iplink_bridge.c | 26 ++
 2 files changed, 36 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 31fc51bd..9fefc7f3 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
__u32 pad2;
 };
 
+struct bridge_stp_xstats {
+   __u64 transition_blk;
+   __u64 transition_fwd;
+   __u64 rx_bpdu;
+   __u64 tx_bpdu;
+   __u64 rx_tcn;
+   __u64 tx_tcn;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  * [MDBA_MDB_ENTRY] = {
@@ -262,6 +271,7 @@ enum {
BRIDGE_XSTATS_VLAN,
BRIDGE_XSTATS_MCAST,
BRIDGE_XSTATS_PAD,
+   BRIDGE_XSTATS_STP,
__BRIDGE_XSTATS_MAX
 };
 #define BRIDGE_XSTATS_MAX (__BRIDGE_XSTATS_MAX - 1)
diff --git a/ip/iplink_bridge.c b/ip/iplink_bridge.c
index 06f736d4..bbd6f3a8 100644
--- a/ip/iplink_bridge.c
+++ b/ip/iplink_bridge.c
@@ -688,6 +688,7 @@ static void bridge_print_xstats_help(struct link_util *lu, 
FILE *f)
 static void bridge_print_stats_attr(struct rtattr *attr, int ifindex)
 {
struct rtattr *brtb[LINK_XSTATS_TYPE_MAX+1];
+   struct bridge_stp_xstats *sstats;
struct br_mcast_stats *mstats;
struct rtattr *i, *list;
const char *ifname = "";
@@ -807,6 +808,29 @@ static void bridge_print_stats_attr(struct rtattr *attr, 
int ifindex)
  mstats->mld_parse_errors);
close_json_object();
break;
+   case BRIDGE_XSTATS_STP:
+   sstats = RTA_DATA(i);
+   open_json_object("stp");
+   print_string(PRINT_FP, NULL,
+"%-16sSTP BPDU:  ", "");
+   print_u64(PRINT_ANY, "rx_bpdu", "RX: %llu ",
+ sstats->rx_bpdu);
+   print_u64(PRINT_ANY, "tx_bpdu", "TX: %llu\n",
+ sstats->tx_bpdu);
+   print_string(PRINT_FP, NULL,
+"%-16sSTP TCN:   ", "");
+   print_u64(PRINT_ANY, "rx_tcn", "RX: %llu ",
+ sstats->rx_tcn);
+   print_u64(PRINT_ANY, "tx_tcn", "TX: %llu\n",
+ sstats->tx_tcn);
+   print_string(PRINT_FP, NULL,
+"%-16sSTP Transitions: ", "");
+   print_u64(PRINT_ANY, "transition_blk", "Blocked: %llu ",
+ sstats->transition_blk);
+   print_u64(PRINT_ANY, "transition_fwd", "Forwarding: 
%llu\n",
+ sstats->transition_fwd);
+   close_json_object();
+   break;
}
}
close_json_object();
@@ -843,6 +867,8 @@ int bridge_parse_xstats(struct link_util *lu, int argc, 
char **argv)
while (argc > 0) {
if (strcmp(*argv, "igmp") == 0 || strcmp(*argv, "mcast") == 0) {
xstats_print_attr = BRIDGE_XSTATS_MCAST;
+   } else if (strcmp(*argv, "stp") == 0) {
+   xstats_print_attr = BRIDGE_XSTATS_STP;
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
filter_index = ll_name_to_index(*argv);
-- 
2.24.0

[Bridge] [PATCH net-next v3] net: bridge: add STP xstats

[Vivien Didelot]

This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk,
transition_fwd xstats counters to the bridge ports copied over via
netlink, providing useful information for STP.

Signed-off-by: Vivien Didelot 
---
 include/uapi/linux/if_bridge.h | 10 ++
 net/bridge/br_netlink.c| 13 +
 net/bridge/br_private.h|  2 ++
 net/bridge/br_stp.c| 15 +++
 net/bridge/br_stp_bpdu.c   |  4 
 5 files changed, 44 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 1b3c2b643a02..4a58e3d7de46 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
__u32 pad2;
 };
 
+struct bridge_stp_xstats {
+   __u64 transition_blk;
+   __u64 transition_fwd;
+   __u64 rx_bpdu;
+   __u64 tx_bpdu;
+   __u64 rx_tcn;
+   __u64 tx_tcn;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  * [MDBA_MDB_ENTRY] = {
@@ -262,6 +271,7 @@ enum {
BRIDGE_XSTATS_VLAN,
BRIDGE_XSTATS_MCAST,
BRIDGE_XSTATS_PAD,
+   BRIDGE_XSTATS_STP,
__BRIDGE_XSTATS_MAX
 };
 #define BRIDGE_XSTATS_MAX (__BRIDGE_XSTATS_MAX - 1)
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index a0a54482aabc..60136575aea4 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1607,6 +1607,19 @@ static int br_fill_linkxstats(struct sk_buff *skb,
br_multicast_get_stats(br, p, nla_data(nla));
}
 #endif
+
+   if (p) {
+   nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
+   sizeof(p->stp_xstats),
+   BRIDGE_XSTATS_PAD);
+   if (!nla)
+   goto nla_put_failure;
+
+   spin_lock_bh(>lock);
+   memcpy(nla_data(nla), >stp_xstats, sizeof(p->stp_xstats));
+   spin_unlock_bh(>lock);
+   }
+
nla_nest_end(skb, nest);
*prividx = 0;
 
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 36b0367ca1e0..f540f3bdf294 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -283,6 +283,8 @@ struct net_bridge_port {
 #endif
u16 group_fwd_mask;
u16 backup_redirected_cnt;
+
+   struct bridge_stp_xstatsstp_xstats;
 };
 
 #define kobj_to_brport(obj)container_of(obj, struct net_bridge_port, kobj)
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 1f1410f8d312..6856a6d9282b 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -45,6 +45,17 @@ void br_set_state(struct net_bridge_port *p, unsigned int 
state)
br_info(p->br, "port %u(%s) entered %s state\n",
(unsigned int) p->port_no, p->dev->name,
br_port_state_names[p->state]);
+
+   if (p->br->stp_enabled == BR_KERNEL_STP) {
+   switch (p->state) {
+   case BR_STATE_BLOCKING:
+   p->stp_xstats.transition_blk++;
+   break;
+   case BR_STATE_FORWARDING:
+   p->stp_xstats.transition_fwd++;
+   break;
+   }
+   }
 }
 
 /* called under bridge lock */
@@ -484,6 +495,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
struct net_bridge *br;
int was_root;
 
+   p->stp_xstats.rx_bpdu++;
+
br = p->br;
was_root = br_is_root_bridge(br);
 
@@ -517,6 +530,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
 /* called under bridge lock */
 void br_received_tcn_bpdu(struct net_bridge_port *p)
 {
+   p->stp_xstats.rx_tcn++;
+
if (br_is_designated_port(p)) {
br_info(p->br, "port %u(%s) received tcn bpdu\n",
(unsigned int) p->port_no, p->dev->name);
diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
index 7796dd9d42d7..0e4572f31330 100644
--- a/net/bridge/br_stp_bpdu.c
+++ b/net/bridge/br_stp_bpdu.c
@@ -118,6 +118,8 @@ void br_send_config_bpdu(struct net_bridge_port *p, struct 
br_config_bpdu *bpdu)
br_set_ticks(buf+33, bpdu->forward_delay);
 
br_send_bpdu(p, buf, 35);
+
+   p->stp_xstats.tx_bpdu++;
 }
 
 /* called under bridge lock */
@@ -133,6 +135,8 @@ void br_send_tcn_bpdu(struct net_bridge_port *p)
buf[2] = 0;
buf[3] = BPDU_TYPE_TCN;
br_send_bpdu(p, buf, 4);
+
+   p->stp_xstats.tx_tcn++;
 }
 
 /*
-- 
2.24.0

Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats

[Vivien Didelot]

Hi David,

On Wed, 11 Dec 2019 14:16:58 -0800 (PST), David Miller  
wrote:
> > To be more precise, what I don't get is that when
> > I move the BRIDGE_XSTATS_STP definition *after* BRIDGE_XSTATS_PAD, the STP
> > xstats don't show up anymore in iproute2.
> 
> Because you ahve to recompile iproute2 so that it uses the corrected value
> in the kernel header, did you do that?

Meh you were correct, my rebuild didn't pick up the header change :-/

I also moved the STP xstats copy below the mcast xstats copy to be consistent
with the order. I'll respin right away.


Thanks,

Vivien

Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats

[Vivien Didelot]

Hi David,

On Wed, 11 Dec 2019 12:01:20 -0800 (PST), David Miller  
wrote:
> >> >>  /* Bridge multicast database attributes
> >> >>   * [MDBA_MDB] = {
> >> >>   * [MDBA_MDB_ENTRY] = {
> >> >> @@ -261,6 +270,7 @@ enum {
> >> >> BRIDGE_XSTATS_UNSPEC,
> >> >> BRIDGE_XSTATS_VLAN,
> >> >> BRIDGE_XSTATS_MCAST,
> >> >> +   BRIDGE_XSTATS_STP,
> >> >> BRIDGE_XSTATS_PAD,
> >> >> __BRIDGE_XSTATS_MAX
> >> >>  };
> >> > 
> >> > Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
> >> > 
> >> 
> >> Oh yes, good catch. That has to be fixed, too.
> >> 
> > 
> > This I don't get. Why new attributes must come between BRIDGE_XSTATS_PAD
> > and __BRIDGE_XSTATS_MAX?
> 
> Because, just like any other attribute value, BRIDGE_XSTATS_PAD is an
> API and fixed in stone.  You can't add things before it which change
> it's value.

I thought the whole point of using enums was to avoid caring about fixed
numeric values, but well. To be more precise, what I don't get is that when
I move the BRIDGE_XSTATS_STP definition *after* BRIDGE_XSTATS_PAD, the STP
xstats don't show up anymore in iproute2.


Thanks,

Vivien

Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats

[Vivien Didelot]

Hi David, Nikolay,

On Wed, 11 Dec 2019 17:42:33 +0200, Nikolay Aleksandrov 
 wrote:
> >>  /* Bridge multicast database attributes
> >>   * [MDBA_MDB] = {
> >>   * [MDBA_MDB_ENTRY] = {
> >> @@ -261,6 +270,7 @@ enum {
> >>BRIDGE_XSTATS_UNSPEC,
> >>BRIDGE_XSTATS_VLAN,
> >>BRIDGE_XSTATS_MCAST,
> >> +  BRIDGE_XSTATS_STP,
> >>BRIDGE_XSTATS_PAD,
> >>__BRIDGE_XSTATS_MAX
> >>  };
> > 
> > Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
> > 
> 
> Oh yes, good catch. That has to be fixed, too.
> 

This I don't get. Why new attributes must come between BRIDGE_XSTATS_PAD
and __BRIDGE_XSTATS_MAX?


Thanks,

Vivien

Re: [Bridge] [PATCH net-next v2] net: bridge: add STP xstats

[Vivien Didelot]

Hi Nikolay,

On Tue, 10 Dec 2019 23:45:13 +0200, Nikolay Aleksandrov 
 wrote:
> > +   if (p) {
> > +   nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
> > +   sizeof(p->stp_xstats),
> > +   BRIDGE_XSTATS_PAD);
> > +   if (!nla)
> > +   goto nla_put_failure;
> > +
> > +   memcpy(nla_data(nla), >stp_xstats, sizeof(p->stp_xstats));
> 
> You need to take the STP lock here to get a proper snapshot of the values.

Good catch! I see a br->multicast_lock but no br->stp_lock. Is this what
you expect?

spin_lock_bh(>lock);
memcpy(nla_data(nla), >stp_xstats, sizeof(p->stp_xstats));
spin_unlock_bh(>lock);


Thanks,

Vivien

[Bridge] [PATCH iproute2 v3] iplink: add support for STP xstats

[Vivien Didelot]

Add support for the BRIDGE_XSTATS_STP xstats, as follow:

# ip link xstats type bridge_slave dev lan4 stp
lan4
STP BPDU:  RX: 0 TX: 61
STP TCN:   RX: 0 TX: 0
STP Transitions: Blocked: 2 Forwarding: 1

Or below as JSON:

# ip -j -p link xstats type bridge_slave dev lan0 stp
[ {
"ifname": "lan0",
"stp": {
"rx_bpdu": 0,
"tx_bpdu": 500,
"rx_tcn": 0,
"tx_tcn": 0,
"transition_blk": 0,
    "transition_fwd": 0
}
} ]

Signed-off-by: Vivien Didelot 
---
 include/uapi/linux/if_bridge.h | 10 ++
 ip/iplink_bridge.c | 26 ++
 2 files changed, 36 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 31fc51bd..42f76697 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
__u32 pad2;
 };
 
+struct bridge_stp_xstats {
+   __u64 transition_blk;
+   __u64 transition_fwd;
+   __u64 rx_bpdu;
+   __u64 tx_bpdu;
+   __u64 rx_tcn;
+   __u64 tx_tcn;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  * [MDBA_MDB_ENTRY] = {
@@ -261,6 +270,7 @@ enum {
BRIDGE_XSTATS_UNSPEC,
BRIDGE_XSTATS_VLAN,
BRIDGE_XSTATS_MCAST,
+   BRIDGE_XSTATS_STP,
BRIDGE_XSTATS_PAD,
__BRIDGE_XSTATS_MAX
 };
diff --git a/ip/iplink_bridge.c b/ip/iplink_bridge.c
index 06f736d4..bbd6f3a8 100644
--- a/ip/iplink_bridge.c
+++ b/ip/iplink_bridge.c
@@ -688,6 +688,7 @@ static void bridge_print_xstats_help(struct link_util *lu, 
FILE *f)
 static void bridge_print_stats_attr(struct rtattr *attr, int ifindex)
 {
struct rtattr *brtb[LINK_XSTATS_TYPE_MAX+1];
+   struct bridge_stp_xstats *sstats;
struct br_mcast_stats *mstats;
struct rtattr *i, *list;
const char *ifname = "";
@@ -807,6 +808,29 @@ static void bridge_print_stats_attr(struct rtattr *attr, 
int ifindex)
  mstats->mld_parse_errors);
close_json_object();
break;
+   case BRIDGE_XSTATS_STP:
+   sstats = RTA_DATA(i);
+   open_json_object("stp");
+   print_string(PRINT_FP, NULL,
+"%-16sSTP BPDU:  ", "");
+   print_u64(PRINT_ANY, "rx_bpdu", "RX: %llu ",
+ sstats->rx_bpdu);
+   print_u64(PRINT_ANY, "tx_bpdu", "TX: %llu\n",
+ sstats->tx_bpdu);
+   print_string(PRINT_FP, NULL,
+"%-16sSTP TCN:   ", "");
+   print_u64(PRINT_ANY, "rx_tcn", "RX: %llu ",
+ sstats->rx_tcn);
+   print_u64(PRINT_ANY, "tx_tcn", "TX: %llu\n",
+ sstats->tx_tcn);
+   print_string(PRINT_FP, NULL,
+"%-16sSTP Transitions: ", "");
+   print_u64(PRINT_ANY, "transition_blk", "Blocked: %llu ",
+ sstats->transition_blk);
+   print_u64(PRINT_ANY, "transition_fwd", "Forwarding: 
%llu\n",
+ sstats->transition_fwd);
+   close_json_object();
+   break;
}
}
close_json_object();
@@ -843,6 +867,8 @@ int bridge_parse_xstats(struct link_util *lu, int argc, 
char **argv)
while (argc > 0) {
if (strcmp(*argv, "igmp") == 0 || strcmp(*argv, "mcast") == 0) {
xstats_print_attr = BRIDGE_XSTATS_MCAST;
+   } else if (strcmp(*argv, "stp") == 0) {
+   xstats_print_attr = BRIDGE_XSTATS_STP;
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
filter_index = ll_name_to_index(*argv);
-- 
2.24.0

[Bridge] [PATCH net-next v2] net: bridge: add STP xstats

[Vivien Didelot]

This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk,
transition_fwd xstats counters to the bridge ports copied over via
netlink, providing useful information for STP.

Signed-off-by: Vivien Didelot 
---
 include/uapi/linux/if_bridge.h | 10 ++
 net/bridge/br_netlink.c| 10 ++
 net/bridge/br_private.h|  2 ++
 net/bridge/br_stp.c| 15 +++
 net/bridge/br_stp_bpdu.c   |  4 
 5 files changed, 41 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 1b3c2b643a02..e7f2bb782006 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
__u32 pad2;
 };
 
+struct bridge_stp_xstats {
+   __u64 transition_blk;
+   __u64 transition_fwd;
+   __u64 rx_bpdu;
+   __u64 tx_bpdu;
+   __u64 rx_tcn;
+   __u64 tx_tcn;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  * [MDBA_MDB_ENTRY] = {
@@ -261,6 +270,7 @@ enum {
BRIDGE_XSTATS_UNSPEC,
BRIDGE_XSTATS_VLAN,
BRIDGE_XSTATS_MCAST,
+   BRIDGE_XSTATS_STP,
BRIDGE_XSTATS_PAD,
__BRIDGE_XSTATS_MAX
 };
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index a0a54482aabc..d339cc314357 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1597,6 +1597,16 @@ static int br_fill_linkxstats(struct sk_buff *skb,
}
}
 
+   if (p) {
+   nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
+   sizeof(p->stp_xstats),
+   BRIDGE_XSTATS_PAD);
+   if (!nla)
+   goto nla_put_failure;
+
+   memcpy(nla_data(nla), >stp_xstats, sizeof(p->stp_xstats));
+   }
+
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
if (++vl_idx >= *prividx) {
nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_MCAST,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 36b0367ca1e0..f540f3bdf294 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -283,6 +283,8 @@ struct net_bridge_port {
 #endif
u16 group_fwd_mask;
u16 backup_redirected_cnt;
+
+   struct bridge_stp_xstatsstp_xstats;
 };
 
 #define kobj_to_brport(obj)container_of(obj, struct net_bridge_port, kobj)
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 1f1410f8d312..6856a6d9282b 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -45,6 +45,17 @@ void br_set_state(struct net_bridge_port *p, unsigned int 
state)
br_info(p->br, "port %u(%s) entered %s state\n",
(unsigned int) p->port_no, p->dev->name,
br_port_state_names[p->state]);
+
+   if (p->br->stp_enabled == BR_KERNEL_STP) {
+   switch (p->state) {
+   case BR_STATE_BLOCKING:
+   p->stp_xstats.transition_blk++;
+   break;
+   case BR_STATE_FORWARDING:
+   p->stp_xstats.transition_fwd++;
+   break;
+   }
+   }
 }
 
 /* called under bridge lock */
@@ -484,6 +495,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
struct net_bridge *br;
int was_root;
 
+   p->stp_xstats.rx_bpdu++;
+
br = p->br;
was_root = br_is_root_bridge(br);
 
@@ -517,6 +530,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
 /* called under bridge lock */
 void br_received_tcn_bpdu(struct net_bridge_port *p)
 {
+   p->stp_xstats.rx_tcn++;
+
if (br_is_designated_port(p)) {
br_info(p->br, "port %u(%s) received tcn bpdu\n",
(unsigned int) p->port_no, p->dev->name);
diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
index 7796dd9d42d7..0e4572f31330 100644
--- a/net/bridge/br_stp_bpdu.c
+++ b/net/bridge/br_stp_bpdu.c
@@ -118,6 +118,8 @@ void br_send_config_bpdu(struct net_bridge_port *p, struct 
br_config_bpdu *bpdu)
br_set_ticks(buf+33, bpdu->forward_delay);
 
br_send_bpdu(p, buf, 35);
+
+   p->stp_xstats.tx_bpdu++;
 }
 
 /* called under bridge lock */
@@ -133,6 +135,8 @@ void br_send_tcn_bpdu(struct net_bridge_port *p)
buf[2] = 0;
buf[3] = BPDU_TYPE_TCN;
br_send_bpdu(p, buf, 4);
+
+   p->stp_xstats.tx_tcn++;
 }
 
 /*
-- 
2.24.0

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

On Tue, 10 Dec 2019 22:52:59 +0200, Nikolay Aleksandrov 
 wrote:
> >> Why do you need percpu ? All of these seem to be incremented with the
> >> bridge lock held. A few more comments below.
> >
> > All other xstats are incremented percpu, I simply followed the pattern.
> >
> 
>  We have already a lock, we can use it and avoid the whole per-cpu memory 
>  handling.
>  It seems to be acquired in all cases where these counters need to be 
>  changed.
> >>>
> >>> Since the other xstats counters are currently implemented this way, I 
> >>> prefer
> >>> to keep the code as is, until we eventually change them all if percpu is 
> >>> in
> >>> fact not needed anymore.
> >>>
> >>> The new series is ready and I can submit it now if there's no objection.
> >>
> >> There is a reason other counters use per-cpu - they're incremented without 
> >> any locking from fast-path.
> >> The bridge STP code already has a lock which is acquired in all of these 
> >> paths and we don't need
> >> this overhead and the per-cpu memory allocations. Unless you can find a 
> >> STP codepath which actually
> >> needs per-cpu, I'd prefer you drop it.
> > 
> > Ho ok I understand what you mean now. I'll drop the percpu attribute.
> 
> Great, thanks again.
> I think it's clear, but I'll add just in case to avoid extra work - you can 
> drop
> the dynamic memory allocation altogether and make the struct part of 
> net_bridge_port.

Yup, that's what I've done and it makes the patch shamely small now ;)


Thanks,

Vivien

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

On Tue, 10 Dec 2019 22:15:26 +0200, Nikolay Aleksandrov 
 wrote:
>  Why do you need percpu ? All of these seem to be incremented with the
>  bridge lock held. A few more comments below.
> >>>
> >>> All other xstats are incremented percpu, I simply followed the pattern.
> >>>
> >>
> >> We have already a lock, we can use it and avoid the whole per-cpu memory 
> >> handling.
> >> It seems to be acquired in all cases where these counters need to be 
> >> changed.
> > 
> > Since the other xstats counters are currently implemented this way, I prefer
> > to keep the code as is, until we eventually change them all if percpu is in
> > fact not needed anymore.
> > 
> > The new series is ready and I can submit it now if there's no objection.
> 
> There is a reason other counters use per-cpu - they're incremented without 
> any locking from fast-path.
> The bridge STP code already has a lock which is acquired in all of these 
> paths and we don't need
> this overhead and the per-cpu memory allocations. Unless you can find a STP 
> codepath which actually
> needs per-cpu, I'd prefer you drop it.

Ho ok I understand what you mean now. I'll drop the percpu attribute.


Thanks,

Vivien

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

Hi Nikolay,

On Tue, 10 Dec 2019 21:50:10 +0200, Nikolay Aleksandrov 
 wrote:
> >> Why do you need percpu ? All of these seem to be incremented with the
> >> bridge lock held. A few more comments below.
> > 
> > All other xstats are incremented percpu, I simply followed the pattern.
> > 
> 
> We have already a lock, we can use it and avoid the whole per-cpu memory 
> handling.
> It seems to be acquired in all cases where these counters need to be changed.

Since the other xstats counters are currently implemented this way, I prefer
to keep the code as is, until we eventually change them all if percpu is in
fact not needed anymore.

The new series is ready and I can submit it now if there's no objection.


Thanks,

Vivien

Re: [Bridge] [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

Hi Nikolay,

On Tue, 10 Dec 2019 09:49:59 +0200, Nikolay Aleksandrov 
 wrote:

> Why did you send the bridge patch again ? Does it have any changes ?

The second iproute2 patch does not include the include guards update, but
I kept the bridge_stp_stats structure and the BRIDGE_XSTATS_STP definition
otherwise iproute2 wouldn't compile.

> 
> Why do you need percpu ? All of these seem to be incremented with the
> bridge lock held. A few more comments below.

All other xstats are incremented percpu, I simply followed the pattern.

> > struct net_bridge_port *p
> > = container_of(kobj, struct net_bridge_port, kobj);
> > +   free_percpu(p->stp_stats);
> 
> Please leave a new line between local var declaration and the code. I know
> it was missing, but you can add it now. :)

OK.

> > +   if (p) {
> > +   struct bridge_stp_xstats xstats;
> 
> Please rename the local var here, using just xstats is misleading.
> Maybe stp_xstats ?

This isn't misleading to me since its scope is limited to the current block
and not the entire function. The block above dumping the VLAN xstats is
using a local "struct br_vlan_stats stats" variable for example.

> 
> > +
> > +   br_stp_get_xstats(p, );
> > +
> > +   if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), ))
> > +   goto nla_put_failure;
> 
> Could you please follow how mcast xstats are dumped and do something similar ?
> It'd be nice to have similar code to audit.

Sure. I would also love to have easily auditable code in net/bridge. For
the bridge STP xstats I followed the VLAN xstats code above, which does:

if (nla_put(skb, BRIDGE_XSTATS_VLAN, sizeof(vxi), ))
goto nla_put_failure;

But I can change the STP xstats code to the following:

if (p) {
nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_STP,
sizeof(struct bridge_stp_xstats),
BRIDGE_XSTATS_PAD);
if (!nla)
goto nla_put_failure;

br_stp_get_xstats(p, nla_data(nla));
}

Would that be preferred?


Thanks,

Vivien

Re: [Bridge] [PATCH iproute2 v2] iplink: add support for STP xstats

[Vivien Didelot]

Hi Stephen,

On Mon, 9 Dec 2019 16:13:45 -0800, Stephen Hemminger 
 wrote:
> On Mon,  9 Dec 2019 18:05:22 -0500
> Vivien Didelot  wrote:
> 
> > Add support for the BRIDGE_XSTATS_STP xstats, as follow:
> > 
> > # ip link xstats type bridge_slave dev lan5
> > STP BPDU:
> >   RX: 0
> >   TX: 39
> > STP TCN:
> >   RX: 0
> >   TX: 0
> > STP Transitions:
> >   Blocked: 0
> >   Forwarding: 1
> > IGMP queries:
> >   RX: v1 0 v2 0 v3 0
> >   TX: v1 0 v2 0 v3 0
> > ...
> 
> Might I suggest a more concise format:
>   STP BPDU:  RX: 0 TX: 39
>   STP TCN:   RX: 0 TX:0
>   STP Transitions: Blocked: 0 Forwarding: 1
> ...

I don't mind if you prefer this format ;-)

Re: [Bridge] [PATCH iproute2] iplink: add support for STP xstats

[Vivien Didelot]

On Mon, 9 Dec 2019 14:34:23 -0800, Stephen Hemminger 
 wrote:
> On Mon,  9 Dec 2019 16:18:41 -0500
> Vivien Didelot  wrote:
> 
> > diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> > index 31fc51bd..e7f2bb78 100644
> > --- a/include/uapi/linux/if_bridge.h
> > +++ b/include/uapi/linux/if_bridge.h
> 
> These headers are semi-automatically updated from the kernel.
> Do not make changes here.

OK, I respin the series with only the minimal changes required for iproute2
to compile correctly.

Thank you,
Vivien

[Bridge] [PATCH iproute2 v2] iplink: add support for STP xstats

[Vivien Didelot]

Add support for the BRIDGE_XSTATS_STP xstats, as follow:

# ip link xstats type bridge_slave dev lan5
STP BPDU:
  RX: 0
  TX: 39
STP TCN:
  RX: 0
  TX: 0
STP Transitions:
  Blocked: 0
  Forwarding: 1
IGMP queries:
  RX: v1 0 v2 0 v3 0
  TX: v1 0 v2 0 v3 0
...

Or below as JSON:

# ip -j -p link xstats type bridge_slave dev lan0 | head
[ {
"ifname": "lan0",
"stp": {
"rx_bpdu": 0,
"tx_bpdu": 500,
"rx_tcn": 0,
"tx_tcn": 0,
"transition_blk": 0,
        "transition_fwd": 1
},
...

Signed-off-by: Vivien Didelot 
---
 include/uapi/linux/if_bridge.h | 10 ++
 ip/iplink_bridge.c | 32 
 2 files changed, 42 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 31fc51bd..42f76697 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
__u32 pad2;
 };
 
+struct bridge_stp_xstats {
+   __u64 transition_blk;
+   __u64 transition_fwd;
+   __u64 rx_bpdu;
+   __u64 tx_bpdu;
+   __u64 rx_tcn;
+   __u64 tx_tcn;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  * [MDBA_MDB_ENTRY] = {
@@ -261,6 +270,7 @@ enum {
BRIDGE_XSTATS_UNSPEC,
BRIDGE_XSTATS_VLAN,
BRIDGE_XSTATS_MCAST,
+   BRIDGE_XSTATS_STP,
BRIDGE_XSTATS_PAD,
__BRIDGE_XSTATS_MAX
 };
diff --git a/ip/iplink_bridge.c b/ip/iplink_bridge.c
index 06f736d4..92e051c8 100644
--- a/ip/iplink_bridge.c
+++ b/ip/iplink_bridge.c
@@ -688,6 +688,7 @@ static void bridge_print_xstats_help(struct link_util *lu, 
FILE *f)
 static void bridge_print_stats_attr(struct rtattr *attr, int ifindex)
 {
struct rtattr *brtb[LINK_XSTATS_TYPE_MAX+1];
+   struct bridge_stp_xstats *sstats;
struct br_mcast_stats *mstats;
struct rtattr *i, *list;
const char *ifname = "";
@@ -807,6 +808,35 @@ static void bridge_print_stats_attr(struct rtattr *attr, 
int ifindex)
  mstats->mld_parse_errors);
close_json_object();
break;
+   case BRIDGE_XSTATS_STP:
+   sstats = RTA_DATA(i);
+   open_json_object("stp");
+   print_string(PRINT_FP, NULL,
+"%-16sSTP BPDU:\n", "");
+   print_string(PRINT_FP, NULL, "%-16s  ", "");
+   print_u64(PRINT_ANY, "rx_bpdu", "RX: %llu\n",
+ sstats->rx_bpdu);
+   print_string(PRINT_FP, NULL, "%-16s  ", "");
+   print_u64(PRINT_ANY, "tx_bpdu", "TX: %llu\n",
+ sstats->tx_bpdu);
+   print_string(PRINT_FP, NULL,
+"%-16sSTP TCN:\n", "");
+   print_string(PRINT_FP, NULL, "%-16s  ", "");
+   print_u64(PRINT_ANY, "rx_tcn", "RX: %llu\n",
+ sstats->rx_tcn);
+   print_string(PRINT_FP, NULL, "%-16s  ", "");
+   print_u64(PRINT_ANY, "tx_tcn", "TX: %llu\n",
+ sstats->tx_tcn);
+   print_string(PRINT_FP, NULL,
+"%-16sSTP Transitions:\n", "");
+   print_string(PRINT_FP, NULL, "%-16s  ", "");
+   print_u64(PRINT_ANY, "transition_blk", "Blocked: 
%llu\n",
+ sstats->transition_blk);
+   print_string(PRINT_FP, NULL, "%-16s  ", "");
+   print_u64(PRINT_ANY, "transition_fwd", "Forwarding: 
%llu\n",
+ sstats->transition_fwd);
+   close_json_object();
+   break;
}
}
close_json_object();
@@ -843,6 +873,8 @@ int bridge_parse_xstats(struct link_util *lu, int argc, 
char **argv)
while (argc > 0) {
if (strcmp(*argv, "igmp") == 0 || strcmp(*argv, "mcast") == 0) {
xstats_print_attr = BRIDGE_XSTATS_MCAST;
+   } else if (strcmp(*argv, "stp") == 0) {
+   xstats_print_attr = BRIDGE_XSTATS_STP;
} else if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
filter_index = ll_name_to_index(*argv);
-- 
2.24.0

[Bridge] [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk,
transition_fwd xstats counters to the bridge ports copied over via
netlink, providing useful information for STP.

Signed-off-by: Vivien Didelot 
---
 include/uapi/linux/if_bridge.h | 10 ++
 net/bridge/br_if.c |  8 
 net/bridge/br_netlink.c|  9 +
 net/bridge/br_private.h|  9 +
 net/bridge/br_stp.c| 25 +
 net/bridge/br_stp_bpdu.c   | 12 
 net/bridge/br_stp_if.c | 27 +++
 7 files changed, 100 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 1b3c2b643a02..e7f2bb782006 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
__u32 pad2;
 };
 
+struct bridge_stp_xstats {
+   __u64 transition_blk;
+   __u64 transition_fwd;
+   __u64 rx_bpdu;
+   __u64 tx_bpdu;
+   __u64 rx_tcn;
+   __u64 tx_tcn;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  * [MDBA_MDB_ENTRY] = {
@@ -261,6 +270,7 @@ enum {
BRIDGE_XSTATS_UNSPEC,
BRIDGE_XSTATS_VLAN,
BRIDGE_XSTATS_MCAST,
+   BRIDGE_XSTATS_STP,
BRIDGE_XSTATS_PAD,
__BRIDGE_XSTATS_MAX
 };
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 4fe30b182ee7..3eb214ef9763 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -250,6 +250,7 @@ static void release_nbp(struct kobject *kobj)
 {
struct net_bridge_port *p
= container_of(kobj, struct net_bridge_port, kobj);
+   free_percpu(p->stp_stats);
kfree(p);
 }
 
@@ -419,6 +420,12 @@ static struct net_bridge_port *new_nbp(struct net_bridge 
*br,
if (p == NULL)
return ERR_PTR(-ENOMEM);
 
+   p->stp_stats = netdev_alloc_pcpu_stats(struct br_stp_stats);
+   if (!p->stp_stats) {
+   kfree(p);
+   return ERR_PTR(-ENOMEM);
+   }
+
p->br = br;
dev_hold(dev);
p->dev = dev;
@@ -432,6 +439,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge 
*br,
err = br_multicast_add_port(p);
if (err) {
dev_put(dev);
+   free_percpu(p->stp_stats);
kfree(p);
p = ERR_PTR(err);
}
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index a0a54482aabc..03aced1f862b 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1597,6 +1597,15 @@ static int br_fill_linkxstats(struct sk_buff *skb,
}
}
 
+   if (p) {
+   struct bridge_stp_xstats xstats;
+
+   br_stp_get_xstats(p, );
+
+   if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), ))
+   goto nla_put_failure;
+   }
+
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
if (++vl_idx >= *prividx) {
nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_MCAST,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 36b0367ca1e0..af5f28f0f2ef 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -95,6 +95,11 @@ struct br_vlan_stats {
struct u64_stats_sync syncp;
 };
 
+struct br_stp_stats {
+   struct bridge_stp_xstats xstats;
+   struct u64_stats_sync syncp;
+};
+
 struct br_tunnel_info {
__be64  tunnel_id;
struct metadata_dst *tunnel_dst;
@@ -283,6 +288,8 @@ struct net_bridge_port {
 #endif
u16 group_fwd_mask;
u16 backup_redirected_cnt;
+
+   struct br_stp_stats __percpu *stp_stats;
 };
 
 #define kobj_to_brport(obj)container_of(obj, struct net_bridge_port, kobj)
@@ -1146,6 +1153,8 @@ void br_stp_change_bridge_id(struct net_bridge *br, const 
unsigned char *a);
 void br_stp_set_bridge_priority(struct net_bridge *br, u16 newprio);
 int br_stp_set_port_priority(struct net_bridge_port *p, unsigned long newprio);
 int br_stp_set_path_cost(struct net_bridge_port *p, unsigned long path_cost);
+void br_stp_get_xstats(const struct net_bridge_port *p,
+  struct bridge_stp_xstats *xstats);
 ssize_t br_show_bridge_id(char *buf, const struct bridge_id *id);
 
 /* br_stp_bpdu.c */
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 1f1410f8d312..8bcdab29442d 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -45,6 +45,18 @@ void br_set_state(struct net_bridge_port *p, unsigned int 
state)
br_info(p->br, "port %u(%s) entered %s state\n",
(unsigned int) p->port_no, p->dev->name,
br_port_state_names[p->state]);
+
+   if (p->br->stp_enabled == BR_KERNEL_STP) {
+   struct br_stp_stats *stats;
+
+   stats = this_cpu_ptr(p

[Bridge] [PATCH iproute2] iplink: add support for STP xstats

[Vivien Didelot]

Add support for the BRIDGE_XSTATS_STP xstats, as follow:

# ip link xstats type bridge_slave dev lan5
STP BPDU:
  RX: 0
  TX: 39
STP TCN:
  RX: 0
  TX: 0
STP Transitions:
  Blocked: 0
  Forwarding: 1
IGMP queries:
  RX: v1 0 v2 0 v3 0
  TX: v1 0 v2 0 v3 0
...

Or below as JSON:

# ip -j -p link xstats type bridge_slave dev lan0 | head
[ {
"ifname": "lan0",
"stp": {
"rx_bpdu": 0,
"tx_bpdu": 500,
"rx_tcn": 0,
"tx_tcn": 0,
"transition_blk": 0,
        "transition_fwd": 1
},
...

Signed-off-by: Vivien Didelot 
---
 include/uapi/linux/if_bridge.h | 16 +---
 ip/iplink_bridge.c | 32 
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 31fc51bd..e7f2bb78 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -11,8 +11,8 @@
  * 2 of the License, or (at your option) any later version.
  */
 
-#ifndef _LINUX_IF_BRIDGE_H
-#define _LINUX_IF_BRIDGE_H
+#ifndef _UAPI_LINUX_IF_BRIDGE_H
+#define _UAPI_LINUX_IF_BRIDGE_H
 
 #include 
 #include 
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
__u32 pad2;
 };
 
+struct bridge_stp_xstats {
+   __u64 transition_blk;
+   __u64 transition_fwd;
+   __u64 rx_bpdu;
+   __u64 tx_bpdu;
+   __u64 rx_tcn;
+   __u64 tx_tcn;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  * [MDBA_MDB_ENTRY] = {
@@ -261,6 +270,7 @@ enum {
BRIDGE_XSTATS_UNSPEC,
BRIDGE_XSTATS_VLAN,
BRIDGE_XSTATS_MCAST,
+   BRIDGE_XSTATS_STP,
BRIDGE_XSTATS_PAD,
__BRIDGE_XSTATS_MAX
 };
@@ -314,4 +324,4 @@ struct br_boolopt_multi {
__u32 optval;
__u32 optmask;
 };
-#endif /* _LINUX_IF_BRIDGE_H */
+#endif /* _UAPI_LINUX_IF_BRIDGE_H */
diff --git a/ip/iplink_bridge.c b/ip/iplink_bridge.c
index 06f736d4..92e051c8 100644
--- a/ip/iplink_bridge.c
+++ b/ip/iplink_bridge.c
@@ -688,6 +688,7 @@ static void bridge_print_xstats_help(struct link_util *lu, 
FILE *f)
 static void bridge_print_stats_attr(struct rtattr *attr, int ifindex)
 {
struct rtattr *brtb[LINK_XSTATS_TYPE_MAX+1];
+   struct bridge_stp_xstats *sstats;
struct br_mcast_stats *mstats;
struct rtattr *i, *list;
const char *ifname = "";
@@ -807,6 +808,35 @@ static void bridge_print_stats_attr(struct rtattr *attr, 
int ifindex)
  mstats->mld_parse_errors);
close_json_object();
break;
+   case BRIDGE_XSTATS_STP:
+   sstats = RTA_DATA(i);
+   open_json_object("stp");
+   print_string(PRINT_FP, NULL,
+"%-16sSTP BPDU:\n", "");
+   print_string(PRINT_FP, NULL, "%-16s  ", "");
+   print_u64(PRINT_ANY, "rx_bpdu", "RX: %llu\n",
+ sstats->rx_bpdu);
+   print_string(PRINT_FP, NULL, "%-16s  ", "");
+   print_u64(PRINT_ANY, "tx_bpdu", "TX: %llu\n",
+ sstats->tx_bpdu);
+   print_string(PRINT_FP, NULL,
+"%-16sSTP TCN:\n", "");
+   print_string(PRINT_FP, NULL, "%-16s  ", "");
+   print_u64(PRINT_ANY, "rx_tcn", "RX: %llu\n",
+ sstats->rx_tcn);
+   print_string(PRINT_FP, NULL, "%-16s  ", "");
+   print_u64(PRINT_ANY, "tx_tcn", "TX: %llu\n",
+ sstats->tx_tcn);
+   print_string(PRINT_FP, NULL,
+"%-16sSTP Transitions:\n", "");
+   print_string(PRINT_FP, NULL, "%-16s  ", "");
+   print_u64(PRINT_ANY, "transition_blk", "Blocked: 
%llu\n",
+ sstats->transition_blk);
+   print_string(PRINT_FP, NULL, "%-16s  ", "");
+   print_u64(PRINT_ANY

[Bridge] [PATCH net-next] net: bridge: add STP xstats

[Vivien Didelot]

This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk,
transition_fwd xstats counters to the bridge ports copied over via
netlink, providing useful information for STP.

Signed-off-by: Vivien Didelot 
---
 include/uapi/linux/if_bridge.h | 10 ++
 net/bridge/br_if.c |  8 
 net/bridge/br_netlink.c|  9 +
 net/bridge/br_private.h|  9 +
 net/bridge/br_stp.c| 25 +
 net/bridge/br_stp_bpdu.c   | 12 
 net/bridge/br_stp_if.c | 27 +++
 7 files changed, 100 insertions(+)

diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
index 1b3c2b643a02..e7f2bb782006 100644
--- a/include/uapi/linux/if_bridge.h
+++ b/include/uapi/linux/if_bridge.h
@@ -156,6 +156,15 @@ struct bridge_vlan_xstats {
__u32 pad2;
 };
 
+struct bridge_stp_xstats {
+   __u64 transition_blk;
+   __u64 transition_fwd;
+   __u64 rx_bpdu;
+   __u64 tx_bpdu;
+   __u64 rx_tcn;
+   __u64 tx_tcn;
+};
+
 /* Bridge multicast database attributes
  * [MDBA_MDB] = {
  * [MDBA_MDB_ENTRY] = {
@@ -261,6 +270,7 @@ enum {
BRIDGE_XSTATS_UNSPEC,
BRIDGE_XSTATS_VLAN,
BRIDGE_XSTATS_MCAST,
+   BRIDGE_XSTATS_STP,
BRIDGE_XSTATS_PAD,
__BRIDGE_XSTATS_MAX
 };
diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
index 4fe30b182ee7..3eb214ef9763 100644
--- a/net/bridge/br_if.c
+++ b/net/bridge/br_if.c
@@ -250,6 +250,7 @@ static void release_nbp(struct kobject *kobj)
 {
struct net_bridge_port *p
= container_of(kobj, struct net_bridge_port, kobj);
+   free_percpu(p->stp_stats);
kfree(p);
 }
 
@@ -419,6 +420,12 @@ static struct net_bridge_port *new_nbp(struct net_bridge 
*br,
if (p == NULL)
return ERR_PTR(-ENOMEM);
 
+   p->stp_stats = netdev_alloc_pcpu_stats(struct br_stp_stats);
+   if (!p->stp_stats) {
+   kfree(p);
+   return ERR_PTR(-ENOMEM);
+   }
+
p->br = br;
dev_hold(dev);
p->dev = dev;
@@ -432,6 +439,7 @@ static struct net_bridge_port *new_nbp(struct net_bridge 
*br,
err = br_multicast_add_port(p);
if (err) {
dev_put(dev);
+   free_percpu(p->stp_stats);
kfree(p);
p = ERR_PTR(err);
}
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index a0a54482aabc..03aced1f862b 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -1597,6 +1597,15 @@ static int br_fill_linkxstats(struct sk_buff *skb,
}
}
 
+   if (p) {
+   struct bridge_stp_xstats xstats;
+
+   br_stp_get_xstats(p, );
+
+   if (nla_put(skb, BRIDGE_XSTATS_STP, sizeof(xstats), ))
+   goto nla_put_failure;
+   }
+
 #ifdef CONFIG_BRIDGE_IGMP_SNOOPING
if (++vl_idx >= *prividx) {
nla = nla_reserve_64bit(skb, BRIDGE_XSTATS_MCAST,
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 36b0367ca1e0..af5f28f0f2ef 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -95,6 +95,11 @@ struct br_vlan_stats {
struct u64_stats_sync syncp;
 };
 
+struct br_stp_stats {
+   struct bridge_stp_xstats xstats;
+   struct u64_stats_sync syncp;
+};
+
 struct br_tunnel_info {
__be64  tunnel_id;
struct metadata_dst *tunnel_dst;
@@ -283,6 +288,8 @@ struct net_bridge_port {
 #endif
u16 group_fwd_mask;
u16 backup_redirected_cnt;
+
+   struct br_stp_stats __percpu *stp_stats;
 };
 
 #define kobj_to_brport(obj)container_of(obj, struct net_bridge_port, kobj)
@@ -1146,6 +1153,8 @@ void br_stp_change_bridge_id(struct net_bridge *br, const 
unsigned char *a);
 void br_stp_set_bridge_priority(struct net_bridge *br, u16 newprio);
 int br_stp_set_port_priority(struct net_bridge_port *p, unsigned long newprio);
 int br_stp_set_path_cost(struct net_bridge_port *p, unsigned long path_cost);
+void br_stp_get_xstats(const struct net_bridge_port *p,
+  struct bridge_stp_xstats *xstats);
 ssize_t br_show_bridge_id(char *buf, const struct bridge_id *id);
 
 /* br_stp_bpdu.c */
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 1f1410f8d312..8bcdab29442d 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -45,6 +45,18 @@ void br_set_state(struct net_bridge_port *p, unsigned int 
state)
br_info(p->br, "port %u(%s) entered %s state\n",
(unsigned int) p->port_no, p->dev->name,
br_port_state_names[p->state]);
+
+   if (p->br->stp_enabled == BR_KERNEL_STP) {
+   struct br_stp_stats *stats;
+
+   stats = this_cpu_ptr(p

[Bridge] [PATCH net-next] net: bridge: add STP stat counters

[Vivien Didelot]

This adds rx_bpdu, tx_bpdu, rx_tcn, tx_tcn, transition_blk,
transition_fwd stat counters to the bridge ports, along with sysfs
statistics nodes under a "statistics" directory of the "brport" entry,
providing useful information for STP, for example:

# cat /sys/class/net/lan0/brport/statistics/tx_bpdu
26
# cat /sys/class/net/lan5/brport/statistics/transition_fwd
3

At the same time, make BRPORT_ATTR define a non-const attribute as
this is required by the attribute group structure.

Signed-off-by: Vivien Didelot 
---
 net/bridge/br_private.h  |  8 
 net/bridge/br_stp.c  |  8 
 net/bridge/br_stp_bpdu.c |  4 
 net/bridge/br_sysfs_if.c | 35 ++-
 4 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 36b0367ca1e0..360d8030e3b2 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -283,6 +283,14 @@ struct net_bridge_port {
 #endif
u16 group_fwd_mask;
u16 backup_redirected_cnt;
+
+   /* Statistics */
+   atomic_long_t   rx_bpdu;
+   atomic_long_t   tx_bpdu;
+   atomic_long_t   rx_tcn;
+   atomic_long_t   tx_tcn;
+   atomic_long_t   transition_blk;
+   atomic_long_t   transition_fwd;
 };
 
 #define kobj_to_brport(obj)container_of(obj, struct net_bridge_port, kobj)
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 1f1410f8d312..63568ee2a9cd 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -403,6 +403,8 @@ static void br_make_blocking(struct net_bridge_port *p)
 
del_timer(>forward_delay_timer);
}
+
+   atomic_long_inc(>transition_blk);
 }
 
 /* called under bridge lock */
@@ -426,6 +428,8 @@ static void br_make_forwarding(struct net_bridge_port *p)
 
if (br->forward_delay != 0)
mod_timer(>forward_delay_timer, jiffies + br->forward_delay);
+
+   atomic_long_inc(>transition_fwd);
 }
 
 /* called under bridge lock */
@@ -512,6 +516,8 @@ void br_received_config_bpdu(struct net_bridge_port *p,
} else if (br_is_designated_port(p)) {
br_reply(p);
}
+
+   atomic_long_inc(>rx_bpdu);
 }
 
 /* called under bridge lock */
@@ -524,6 +530,8 @@ void br_received_tcn_bpdu(struct net_bridge_port *p)
br_topology_change_detection(p->br);
br_topology_change_acknowledge(p);
}
+
+   atomic_long_inc(>rx_tcn);
 }
 
 /* Change bridge STP parameter */
diff --git a/net/bridge/br_stp_bpdu.c b/net/bridge/br_stp_bpdu.c
index 7796dd9d42d7..e824e040846c 100644
--- a/net/bridge/br_stp_bpdu.c
+++ b/net/bridge/br_stp_bpdu.c
@@ -118,6 +118,8 @@ void br_send_config_bpdu(struct net_bridge_port *p, struct 
br_config_bpdu *bpdu)
br_set_ticks(buf+33, bpdu->forward_delay);
 
br_send_bpdu(p, buf, 35);
+
+   atomic_long_inc(>tx_bpdu);
 }
 
 /* called under bridge lock */
@@ -133,6 +135,8 @@ void br_send_tcn_bpdu(struct net_bridge_port *p)
buf[2] = 0;
buf[3] = BPDU_TYPE_TCN;
br_send_bpdu(p, buf, 4);
+
+   atomic_long_inc(>tx_tcn);
 }
 
 /*
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 7a59c3ce..1fcd42ffa0ff 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -33,7 +33,7 @@ const struct brport_attribute brport_attr_##_name = { 
\
 };
 
 #define BRPORT_ATTR(_name, _mode, _show, _store)   \
-const struct brport_attribute brport_attr_##_name = {  \
+struct brport_attribute brport_attr_##_name = {\
.attr = {.name = __stringify(_name),\
 .mode = _mode },   \
.show   = _show,\
@@ -52,6 +52,13 @@ static int store_##_name(struct net_bridge_port *p, unsigned 
long v) \
 static BRPORT_ATTR(_name, 0644,\
   show_##_name, store_##_name)
 
+#define BRPORT_ATTR_STAT(_name)
\
+static ssize_t show_##_name(struct net_bridge_port *p, char *buf)  \
+{  \
+   return sprintf(buf, "%lu\n", atomic_long_read(>_name));  \
+}  \
+static BRPORT_ATTR(_name, 0444, show_##_name, NULL)
+
 static int store_flag(struct net_bridge_port *p, unsigned long v,
  unsigned long mask)
 {
@@ -352,6 +359,28 @@ const struct sysfs_ops brport_sysfs_ops = {
.store = brport_store,
 };
 
+BRPORT_ATTR_STAT(rx_bpdu);
+BRPORT_ATTR_STAT(tx_bpdu);
+BRPORT_ATTR_STAT(rx_tcn);
+BRPORT_ATTR_STAT(tx_tcn)

Re: [PATCH] net: bridge: Allow bridge to joing multicast groups

[Vivien Didelot]

Hi Horatiu,

On Thu, 1 Aug 2019 21:48:02 +0200, Horatiu Vultur 
 wrote:
> > I'm a bit late in the conversation. Isn't this what you want?
> > 
> > ip address add  dev br0 autojoin
> > 
> 
> Not really, I was looking in a way to register the ports to link layer
> multicast address. Sorry for the confusion, my description of the patch
> was totally missleaning.
> 
> If you follow this thread you will get a better idea what we wanted to
> achive. We got some really good comments and based on these we send a
> RFC[1]. 

OK great! Keep me in the loop, I enjoy bridge and multicast very much ;-)


Thanks,

Vivien

Re: [PATCH] net: bridge: Allow bridge to joing multicast groups

[Vivien Didelot]

Hi Horatiu,

On Thu, 25 Jul 2019 13:44:04 +0200, Horatiu Vultur 
 wrote:
> There is no way to configure the bridge, to receive only specific link
> layer multicast addresses. From the description of the command 'bridge
> fdb append' is supposed to do that, but there was no way to notify the
> network driver that the bridge joined a group, because LLADDR was added
> to the unicast netdev_hw_addr_list.
> 
> Therefore update fdb_add_entry to check if the NLM_F_APPEND flag is set
> and if the source is NULL, which represent the bridge itself. Then add
> address to multicast netdev_hw_addr_list for each bridge interfaces.
> And then the .ndo_set_rx_mode function on the driver is called. To notify
> the driver that the list of multicast mac addresses changed.
> 
> Signed-off-by: Horatiu Vultur 
> ---
>  net/bridge/br_fdb.c | 49 ++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index b1d3248..d93746d 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -175,6 +175,29 @@ static void fdb_add_hw_addr(struct net_bridge *br, const 
> unsigned char *addr)
>   }
>  }
>  
> +static void fdb_add_hw_maddr(struct net_bridge *br, const unsigned char 
> *addr)
> +{
> + int err;
> + struct net_bridge_port *p;
> +
> + ASSERT_RTNL();
> +
> + list_for_each_entry(p, >port_list, list) {
> + if (!br_promisc_port(p)) {
> + err = dev_mc_add(p->dev, addr);
> + if (err)
> + goto undo;
> + }
> + }
> +
> + return;
> +undo:
> + list_for_each_entry_continue_reverse(p, >port_list, list) {
> + if (!br_promisc_port(p))
> + dev_mc_del(p->dev, addr);
> + }
> +}
> +
>  /* When a static FDB entry is deleted, the HW address from that entry is
>   * also removed from the bridge private HW address list and updates all
>   * the ports with needed information.
> @@ -192,13 +215,27 @@ static void fdb_del_hw_addr(struct net_bridge *br, 
> const unsigned char *addr)
>   }
>  }
>  
> +static void fdb_del_hw_maddr(struct net_bridge *br, const unsigned char 
> *addr)
> +{
> + struct net_bridge_port *p;
> +
> + ASSERT_RTNL();
> +
> + list_for_each_entry(p, >port_list, list) {
> + if (!br_promisc_port(p))
> + dev_mc_del(p->dev, addr);
> + }
> +}
> +
>  static void fdb_delete(struct net_bridge *br, struct net_bridge_fdb_entry *f,
>  bool swdev_notify)
>  {
>   trace_fdb_delete(br, f);
>  
> - if (f->is_static)
> + if (f->is_static) {
>   fdb_del_hw_addr(br, f->key.addr.addr);
> + fdb_del_hw_maddr(br, f->key.addr.addr);
> + }
>  
>   hlist_del_init_rcu(>fdb_node);
>   rhashtable_remove_fast(>fdb_hash_tbl, >rhnode,
> @@ -843,13 +880,19 @@ static int fdb_add_entry(struct net_bridge *br, struct 
> net_bridge_port *source,
>   fdb->is_local = 1;
>   if (!fdb->is_static) {
>   fdb->is_static = 1;
> - fdb_add_hw_addr(br, addr);
> + if (flags & NLM_F_APPEND && !source)
> + fdb_add_hw_maddr(br, addr);
> + else
> + fdb_add_hw_addr(br, addr);
>   }
>   } else if (state & NUD_NOARP) {
>   fdb->is_local = 0;
>   if (!fdb->is_static) {
>   fdb->is_static = 1;
> - fdb_add_hw_addr(br, addr);
> + if (flags & NLM_F_APPEND && !source)
> + fdb_add_hw_maddr(br, addr);
> + else
> + fdb_add_hw_addr(br, addr);
>   }
>   } else {
>   fdb->is_local = 0;
> -- 
> 2.7.4
> 

I'm a bit late in the conversation. Isn't this what you want?

ip address add  dev br0 autojoin


Thanks,
Vivien

Re: [Bridge] [PATCH net-next v3 11/12] net: dsa: Implement ndo_get_port_parent_id()

[Vivien Didelot]

Hi Florian,

On Tue,  5 Feb 2019 15:53:25 -0800, Florian Fainelli  
wrote:
> DSA implements SWITCHDEV_ATTR_ID_PORT_PARENT_ID and we want to get rid
> of switchdev_ops eventually, ease that migration by implementing a
> ndo_get_port_parent_id() function which returns what
> switchdev_port_attr_get() would do.
> 
> Signed-off-by: Florian Fainelli 
> ---
>  net/dsa/slave.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 91de3a663226..70395a0ae52e 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -362,18 +362,23 @@ static int dsa_slave_port_obj_del(struct net_device 
> *dev,
>   return err;
>  }
>  
> -static int dsa_slave_port_attr_get(struct net_device *dev,
> -struct switchdev_attr *attr)
> +static int dsa_slave_get_port_parent_id(struct net_device *dev,
> + struct netdev_phys_item_id *ppid)
>  {
>   struct dsa_port *dp = dsa_slave_to_port(dev);
>   struct dsa_switch *ds = dp->ds;
>   struct dsa_switch_tree *dst = ds->dst;
>  
> + ppid->id_len = sizeof(dst->index);
> + memcpy(>id, >index, ppid->id_len);
> +
> + return 0;
> +}

Finally this will give us a way to distinguish two ports with the same switch
and port IDs on a system with two disjoint switch trees, thanks!

Reviewed-by: Vivien Didelot 

Re: [Bridge] [PATCH net-next v3 1/7] net: bridge: Extract boilerplate around switchdev_port_obj_*()

[Vivien Didelot]

Hi Petr,

Petr Machata  writes:

> A call to switchdev_port_obj_add() or switchdev_port_obj_del() involves
> initializing a struct switchdev_obj_port_vlan, a piece of code that
> repeats on each call site almost verbatim. While in the current codebase
> there is just one duplicated add call, the follow-up patches add more of
> both add and del calls.
>
> Thus to remove the duplication, extract the repetition into named
> functions and reuse.
>
> Signed-off-by: Petr Machata 

Considering Dan's comment as well:

Reviewed-by: Vivien Didelot 

Thanks,

Vivien

Re: [Bridge] [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Vivien Didelot]

Hi Petr,

Petr Machata <pe...@mellanox.com> writes:

> Vivien Didelot <vivien.dide...@savoirfairelinux.com> writes:
>
>>> +   } else {
>>> +   err = br_switchdev_port_obj_add(dev, v->vid, flags);
>>> +   if (err && err != -EOPNOTSUPP)
>>> +   goto out;
>>> }
>>
>> Except that br_switchdev_port_obj_add taking vid and flags arguments
>> seems confusing to me, the change looks good:
>
> I'm not sure what you're aiming at. Both VID and flags are sent with the
> notification, so they need to be passed on to the function somehow. Do
> you have a counterproposal for the API?

I'm only questioning the code organization here, not the functional
aspect which I do agree with. What I'm saying is that you name a new
switchdev helper br_switchdev_port_OBJ_add, which takes VLAN arguments
(vid and flags.) How would you call another eventual helper taking MDB
arguments, br_switchdev_port_OBJ_add again? So something like
br_switchdev_port_VLAN_add would be more intuitive.

At the same time there's an effort to centralize all switchdev helpers
of the bridge layer (i.e. the software -> hardware bridge calls) into
net/bridge/br_switchdev.c, so that file would be more adequate.

You may discard my comments but I think it'd be beneficial to us all to
finally keep a bit of consistency in that bridge layer code.


Thanks,

Vivien

Re: [Bridge] [PATCH net-next 0/7] net: bridge: Notify about bridge VLANs

[Vivien Didelot]

Hi Florian,

Florian Fainelli  writes:

> Andrew, Vivien, if the following hunks get applied are we possibly
> breaking mv88e6xxx? This is the use case that is really missing IMHO at
> the moment in DSA: we cannot control the VLAN membership and attributes
> of the CPU port(s), so either we make it always tagged in every VLAN
> (not great), or we introduce the ability to target the CPU port which is
> what Petr's patches + mine do.

Your change looks good to me. mv88e6xxx programs the DSA and CPU ports'
membership as "unmodified" (i.e. "as-is", which is a Marvell feature),
so that shouldn't change the current behavior.


Thanks,

Vivien

Re: [Bridge] [PATCH net-next 6/7] net: bridge: Notify about bridge VLANs

[Vivien Didelot]

Hi Petr,

Petr Machata <pe...@mellanox.com> writes:

> A driver might need to react to changes in settings of brentry VLANs.
> Therefore send switchdev port notifications for these as well. Reuse
> SWITCHDEV_OBJ_ID_PORT_VLAN for this purpose. Listeners should use
> netif_is_bridge_master() on orig_dev to determine whether the
> notification is about a bridge port or a bridge.
>
> Signed-off-by: Petr Machata <pe...@mellanox.com>

> + } else {
> + err = br_switchdev_port_obj_add(dev, v->vid, flags);
> + if (err && err != -EOPNOTSUPP)
> + goto out;
>   }

Except that br_switchdev_port_obj_add taking vid and flags arguments
seems confusing to me, the change looks good:

Reviewed-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>

Thanks,

Vivien

Re: [Bridge] [PATCH net-next 4/7] dsa: port: Ignore bridge VLAN events

[Vivien Didelot]

Hi Petr,

Petr Machata <pe...@mellanox.com> writes:

> Ignore VLAN events where the orig_dev is the bridge device itself.
>
> Signed-off-by: Petr Machata <pe...@mellanox.com>

Reviewed-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>

Thanks,

Vivien

[Bridge] [PATCH net-next 1/6] net: bridge: pass net_bridge_port to __br_mdb_add

[Vivien Didelot]

The __br_mdb_add function uses exactly the same mechanism as its caller
br_mdb_add to retrieve the net_bridge_port pointer from br_mdb_entry.

Remove it and pass directly the net_bridge_port pointer to __br_mdb_add.

Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 18 --
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index b0845480a3ae..bef0331f46a4 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -542,25 +542,15 @@ static int br_mdb_add_group(struct net_bridge *br, struct 
net_bridge_port *port,
return 0;
 }
 
-static int __br_mdb_add(struct net *net, struct net_bridge *br,
-   struct br_mdb_entry *entry)
+static int __br_mdb_add(struct net_bridge_port *p, struct br_mdb_entry *entry)
 {
+   struct net_bridge *br = p->br;
struct br_ip ip;
-   struct net_device *dev;
-   struct net_bridge_port *p;
int ret;
 
if (!netif_running(br->dev) || br->multicast_disabled)
return -EINVAL;
 
-   dev = __dev_get_by_index(net, entry->ifindex);
-   if (!dev)
-   return -ENODEV;
-
-   p = br_port_get_rtnl(dev);
-   if (!p || p->br != br || p->state == BR_STATE_DISABLED)
-   return -EINVAL;
-
__mdb_entry_to_br_ip(entry, );
 
spin_lock_bh(>multicast_lock);
@@ -602,13 +592,13 @@ static int br_mdb_add(struct sk_buff *skb, struct 
nlmsghdr *nlh,
if (br_vlan_enabled(br) && vg && entry->vid == 0) {
list_for_each_entry(v, >vlan_list, vlist) {
entry->vid = v->vid;
-   err = __br_mdb_add(net, br, entry);
+   err = __br_mdb_add(p, entry);
if (err)
break;
__br_mdb_notify(dev, p, entry, RTM_NEWMDB);
}
} else {
-   err = __br_mdb_add(net, br, entry);
+   err = __br_mdb_add(p, entry);
if (!err)
__br_mdb_notify(dev, p, entry, RTM_NEWMDB);
}
-- 
2.13.0

[Bridge] [PATCH net-next 4/6] net: bridge: add __br_mdb_do

[Vivien Didelot]

br_mdb_add and br_mdb_del call respectively __br_mdb_add and
__br_mdb_del and notify the message type on success.

Factorize this in a new __br_mdb_do function which add or delete a
br_mdb_entry depending on the message type, then notify it.

Note that the dev argument is p->br->dev, so no need to pass it twice.

Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 33 +++--
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 24fb4179..a72d5e6f339f 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -556,6 +556,9 @@ static int __br_mdb_add(struct net_bridge_port *p, struct 
br_mdb_entry *entry)
return ret;
 }
 
+static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
+  int msgtype);
+
 static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr *nlh,
  struct netlink_ext_ack *extack)
 {
@@ -592,15 +595,12 @@ static int br_mdb_add(struct sk_buff *skb, struct 
nlmsghdr *nlh,
if (br_vlan_enabled(br) && vg && entry->vid == 0) {
list_for_each_entry(v, >vlan_list, vlist) {
entry->vid = v->vid;
-   err = __br_mdb_add(p, entry);
+   err = __br_mdb_do(p, entry, RTM_NEWMDB);
if (err)
break;
-   __br_mdb_notify(dev, p, entry, RTM_NEWMDB);
}
} else {
-   err = __br_mdb_add(p, entry);
-   if (!err)
-   __br_mdb_notify(dev, p, entry, RTM_NEWMDB);
+   err = __br_mdb_do(p, entry, RTM_NEWMDB);
}
 
return err;
@@ -651,6 +651,22 @@ static int __br_mdb_del(struct net_bridge *br, struct 
br_mdb_entry *entry)
return err;
 }
 
+static int __br_mdb_do(struct net_bridge_port *p, struct br_mdb_entry *entry,
+  int msgtype)
+{
+   int err = -EINVAL;
+
+   if (msgtype == RTM_NEWMDB)
+   err = __br_mdb_add(p, entry);
+   else if (msgtype == RTM_DELMDB)
+   err = __br_mdb_del(p->br, entry);
+
+   if (!err)
+   __br_mdb_notify(p->br->dev, p, entry, msgtype);
+
+   return err;
+}
+
 static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr *nlh,
  struct netlink_ext_ack *extack)
 {
@@ -687,15 +703,12 @@ static int br_mdb_del(struct sk_buff *skb, struct 
nlmsghdr *nlh,
if (br_vlan_enabled(br) && vg && entry->vid == 0) {
list_for_each_entry(v, >vlan_list, vlist) {
entry->vid = v->vid;
-   err = __br_mdb_del(br, entry);
+   err = __br_mdb_do(p, entry, RTM_DELMDB);
if (err)
break;
-   __br_mdb_notify(dev, p, entry, RTM_DELMDB);
}
} else {
-   err = __br_mdb_del(br, entry);
-   if (!err)
-   __br_mdb_notify(dev, p, entry, RTM_DELMDB);
+   err = __br_mdb_do(p, entry, RTM_DELMDB);
}
 
return err;
-- 
2.13.0

[Bridge] [PATCH net-next 2/6] net: bridge: check multicast bridge only once

[Vivien Didelot]

__br_mdb_add and __br_mdb_del both check that the bridge is up and
running and that multicast is enabled on it before doing any operation.

Since they can be called multiple times by br_mdb_add and br_mdb_del,
move the check in their caller functions so that it is done just once.

Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index bef0331f46a4..d20a01622b20 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -548,9 +548,6 @@ static int __br_mdb_add(struct net_bridge_port *p, struct 
br_mdb_entry *entry)
struct br_ip ip;
int ret;
 
-   if (!netif_running(br->dev) || br->multicast_disabled)
-   return -EINVAL;
-
__mdb_entry_to_br_ip(entry, );
 
spin_lock_bh(>multicast_lock);
@@ -577,6 +574,9 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr 
*nlh,
 
br = netdev_priv(dev);
 
+   if (!netif_running(br->dev) || br->multicast_disabled)
+   return -EINVAL;
+
/* If vlan filtering is enabled and VLAN is not specified
 * install mdb entry on all vlans configured on the port.
 */
@@ -615,9 +615,6 @@ static int __br_mdb_del(struct net_bridge *br, struct 
br_mdb_entry *entry)
struct br_ip ip;
int err = -EINVAL;
 
-   if (!netif_running(br->dev) || br->multicast_disabled)
-   return -EINVAL;
-
__mdb_entry_to_br_ip(entry, );
 
spin_lock_bh(>multicast_lock);
@@ -672,6 +669,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr 
*nlh,
 
br = netdev_priv(dev);
 
+   if (!netif_running(br->dev) || br->multicast_disabled)
+   return -EINVAL;
+
/* If vlan filtering is enabled and VLAN is not specified
 * delete mdb entry on all vlans configured on the port.
 */
-- 
2.13.0

[Bridge] [PATCH net-next 3/6] net: bridge: break if __br_mdb_del fails

[Vivien Didelot]

Be symmetric with br_mdb_add and break if __br_mdb_del returns an error.

Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index d20a01622b20..24fb4179 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -688,8 +688,9 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr 
*nlh,
list_for_each_entry(v, >vlan_list, vlist) {
entry->vid = v->vid;
err = __br_mdb_del(br, entry);
-   if (!err)
-   __br_mdb_notify(dev, p, entry, RTM_DELMDB);
+   if (err)
+   break;
+   __br_mdb_notify(dev, p, entry, RTM_DELMDB);
}
} else {
err = __br_mdb_del(br, entry);
-- 
2.13.0

[Bridge] [PATCH net-next 5/6] net: bridge: get msgtype from nlmsghdr in mdb ops

[Vivien Didelot]

Retrieve the message type from the nlmsghdr structure instead of
hardcoding it in both br_mdb_add and br_mdb_del.

Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 net/bridge/br_mdb.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index a72d5e6f339f..d280b20587cb 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -569,6 +569,7 @@ static int br_mdb_add(struct sk_buff *skb, struct nlmsghdr 
*nlh,
struct net_bridge_port *p;
struct net_bridge_vlan *v;
struct net_bridge *br;
+   int msgtype = nlh->nlmsg_type;
int err;
 
err = br_mdb_parse(skb, nlh, , );
@@ -595,12 +596,12 @@ static int br_mdb_add(struct sk_buff *skb, struct 
nlmsghdr *nlh,
if (br_vlan_enabled(br) && vg && entry->vid == 0) {
list_for_each_entry(v, >vlan_list, vlist) {
entry->vid = v->vid;
-   err = __br_mdb_do(p, entry, RTM_NEWMDB);
+   err = __br_mdb_do(p, entry, msgtype);
if (err)
break;
}
} else {
-   err = __br_mdb_do(p, entry, RTM_NEWMDB);
+   err = __br_mdb_do(p, entry, msgtype);
}
 
return err;
@@ -677,6 +678,7 @@ static int br_mdb_del(struct sk_buff *skb, struct nlmsghdr 
*nlh,
struct net_bridge_port *p;
struct net_bridge_vlan *v;
struct net_bridge *br;
+   int msgtype = nlh->nlmsg_type;
int err;
 
err = br_mdb_parse(skb, nlh, , );
@@ -703,12 +705,12 @@ static int br_mdb_del(struct sk_buff *skb, struct 
nlmsghdr *nlh,
if (br_vlan_enabled(br) && vg && entry->vid == 0) {
list_for_each_entry(v, >vlan_list, vlist) {
entry->vid = v->vid;
-   err = __br_mdb_do(p, entry, RTM_DELMDB);
+   err = __br_mdb_do(p, entry, msgtype);
if (err)
break;
}
} else {
-   err = __br_mdb_do(p, entry, RTM_DELMDB);
+   err = __br_mdb_do(p, entry, msgtype);
}
 
return err;
-- 
2.13.0

[Bridge] [PATCH net 1/3] net: bridge: add helper to offload ageing time

[Vivien Didelot]

The SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME switchdev attr is actually set
when initializing a bridge port, and when configuring the bridge ageing
time from ioctl/netlink/sysfs.

Add a __set_ageing_time helper to offload the ageing time to physical
switches, and add the SWITCHDEV_F_DEFER flag since it can be called
under bridge lock.

Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 net/bridge/br_private.h |  1 +
 net/bridge/br_stp.c | 28 
 net/bridge/br_stp_if.c  | 12 +++-
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1b63177..3c294b4 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -992,6 +992,7 @@ void __br_set_forward_delay(struct net_bridge *br, unsigned 
long t);
 int br_set_forward_delay(struct net_bridge *br, unsigned long x);
 int br_set_hello_time(struct net_bridge *br, unsigned long x);
 int br_set_max_age(struct net_bridge *br, unsigned long x);
+int __set_ageing_time(struct net_device *dev, unsigned long t);
 int br_set_ageing_time(struct net_bridge *br, clock_t ageing_time);
 
 
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 9258b8e..6ebe2a0 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -562,6 +562,24 @@ int br_set_max_age(struct net_bridge *br, unsigned long 
val)
 
 }
 
+/* called under bridge lock */
+int __set_ageing_time(struct net_device *dev, unsigned long t)
+{
+   struct switchdev_attr attr = {
+   .orig_dev = dev,
+   .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
+   .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP | SWITCHDEV_F_DEFER,
+   .u.ageing_time = jiffies_to_clock_t(t),
+   };
+   int err;
+
+   err = switchdev_port_attr_set(dev, );
+   if (err && err != -EOPNOTSUPP)
+   return err;
+
+   return 0;
+}
+
 /* Set time interval that dynamic forwarding entries live
  * For pure software bridge, allow values outside the 802.1
  * standard specification for special cases:
@@ -572,17 +590,11 @@ int br_set_max_age(struct net_bridge *br, unsigned long 
val)
  */
 int br_set_ageing_time(struct net_bridge *br, clock_t ageing_time)
 {
-   struct switchdev_attr attr = {
-   .orig_dev = br->dev,
-   .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
-   .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
-   .u.ageing_time = ageing_time,
-   };
unsigned long t = clock_t_to_jiffies(ageing_time);
int err;
 
-   err = switchdev_port_attr_set(br->dev, );
-   if (err && err != -EOPNOTSUPP)
+   err = __set_ageing_time(br->dev, t);
+   if (err)
return err;
 
br->ageing_time = t;
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index d8ad73b..2efbba5 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -36,12 +36,6 @@ static inline port_id br_make_port_id(__u8 priority, __u16 
port_no)
 /* called under bridge lock */
 void br_init_port(struct net_bridge_port *p)
 {
-   struct switchdev_attr attr = {
-   .orig_dev = p->dev,
-   .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
-   .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP | SWITCHDEV_F_DEFER,
-   .u.ageing_time = jiffies_to_clock_t(p->br->ageing_time),
-   };
int err;
 
p->port_id = br_make_port_id(p->priority, p->port_no);
@@ -50,9 +44,9 @@ void br_init_port(struct net_bridge_port *p)
p->topology_change_ack = 0;
p->config_pending = 0;
 
-   err = switchdev_port_attr_set(p->dev, );
-   if (err && err != -EOPNOTSUPP)
-   netdev_err(p->dev, "failed to set HW ageing time\n");
+   err = __set_ageing_time(p->dev, p->br->ageing_time);
+   if (err)
+   netdev_err(p->dev, "failed to offload ageing time\n");
 }
 
 /* NO locks held */
-- 
2.10.2

[Bridge] [PATCH net 0/3] net: bridge: fast ageing on topology change

[Vivien Didelot]

802.1D [1] specifies that the bridges in a network must use a short
value to age out dynamic entries in the Filtering Database for a period,
once a topology change has been communicated by the root bridge.

This patchset fixes this for the in-kernel STP implementation.

Once the topology change flag is set in a net_bridge instance, the
ageing time value is shorten to twice the forward delay used by the
topology.

When the topology change flag is cleared, the ageing time configured for
the bridge is restored.

To accomplish that, a new bridge_ageing_time member is added to the
net_bridge structure, to store the user configured bridge ageing time.

Two helpers are added to offload the ageing time and set the topology
change flag in the net_bridge instance. Then the required logic is added
in the topology change helper if in-kernel STP is used.

This has been tested on the following topology:

+--+
| root bridge  |
|  1  2  3  4  |
+--+--+--+--+--+
   |  |  |  |  ++
   |  |  |  +--| laptop |
   |  |  | ++
+--+--+--+-+
|  1  2  3 |
| slave bridge |
+--+

When unplugging/replugging the laptop, the slave bridge (under test)
gets the topology change flag sent by the root bridge, and fast ageing
is triggered on the bridges. Once the topology change timer of the root
bridge expires, the topology change flag is cleared and the configured
ageing time is restored on the bridges.

A similar test has been done between two bridges under test.
When changing the forward delay of the root bridge with:

# echo 3000 > /sys/class/net/br0/bridge/forward_delay

the ageing time correctly changes on both bridges from 300s to 60s while
the TOPOLOGY_CHANGE flag is present.

[1] "8.3.5 Notifying topology changes",
http://profesores.elo.utfsm.cl/~agv/elo309/doc/802.1D-1998.pdf

No change since RFC: https://lkml.org/lkml/2016/10/19/828

Vivien Didelot (3):
  net: bridge: add helper to offload ageing time
  net: bridge: add helper to set topology change
  net: bridge: shorten ageing time on topology change

 net/bridge/br_device.c  |  2 +-
 net/bridge/br_private.h |  4 ++-
 net/bridge/br_private_stp.h |  1 +
 net/bridge/br_stp.c | 65 ++---
 net/bridge/br_stp_if.c  | 14 +++---
 net/bridge/br_stp_timer.c   |  2 +-
 6 files changed, 65 insertions(+), 23 deletions(-)

-- 
2.10.2

[Bridge] [PATCH net 2/3] net: bridge: add helper to set topology change

[Vivien Didelot]

Add a __br_set_topology_change helper to set the topology change value.

This can be later extended to add actions when the topology change flag
is set or cleared.

Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 net/bridge/br_private_stp.h |  1 +
 net/bridge/br_stp.c | 10 --
 net/bridge/br_stp_if.c  |  2 +-
 net/bridge/br_stp_timer.c   |  2 +-
 4 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_private_stp.h b/net/bridge/br_private_stp.h
index 2fe910c..3f7543a 100644
--- a/net/bridge/br_private_stp.h
+++ b/net/bridge/br_private_stp.h
@@ -61,6 +61,7 @@ void br_received_tcn_bpdu(struct net_bridge_port *p);
 void br_transmit_config(struct net_bridge_port *p);
 void br_transmit_tcn(struct net_bridge *br);
 void br_topology_change_detection(struct net_bridge *br);
+void __br_set_topology_change(struct net_bridge *br, unsigned char val);
 
 /* br_stp_bpdu.c */
 void br_send_config_bpdu(struct net_bridge_port *, struct br_config_bpdu *);
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 6ebe2a0..8d7b4c7 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -234,7 +234,7 @@ static void br_record_config_timeout_values(struct 
net_bridge *br,
br->max_age = bpdu->max_age;
br->hello_time = bpdu->hello_time;
br->forward_delay = bpdu->forward_delay;
-   br->topology_change = bpdu->topology_change;
+   __br_set_topology_change(br, bpdu->topology_change);
 }
 
 /* called under bridge lock */
@@ -344,7 +344,7 @@ void br_topology_change_detection(struct net_bridge *br)
isroot ? "propagating" : "sending tcn bpdu");
 
if (isroot) {
-   br->topology_change = 1;
+   __br_set_topology_change(br, 1);
mod_timer(>topology_change_timer, jiffies
  + br->bridge_forward_delay + br->bridge_max_age);
} else if (!br->topology_change_detected) {
@@ -603,6 +603,12 @@ int br_set_ageing_time(struct net_bridge *br, clock_t 
ageing_time)
return 0;
 }
 
+/* called under bridge lock */
+void __br_set_topology_change(struct net_bridge *br, unsigned char val)
+{
+   br->topology_change = val;
+}
+
 void __br_set_forward_delay(struct net_bridge *br, unsigned long t)
 {
br->bridge_forward_delay = t;
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 2efbba5..6c1e214 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -81,7 +81,7 @@ void br_stp_disable_bridge(struct net_bridge *br)
 
}
 
-   br->topology_change = 0;
+   __br_set_topology_change(br, 0);
br->topology_change_detected = 0;
spin_unlock_bh(>lock);
 
diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
index da058b8..7ddb38e 100644
--- a/net/bridge/br_stp_timer.c
+++ b/net/bridge/br_stp_timer.c
@@ -125,7 +125,7 @@ static void br_topology_change_timer_expired(unsigned long 
arg)
br_debug(br, "topo change timer expired\n");
spin_lock(>lock);
br->topology_change_detected = 0;
-   br->topology_change = 0;
+   __br_set_topology_change(br, 0);
spin_unlock(>lock);
 }
 
-- 
2.10.2

Re: [Bridge] [RFC net-next 2/3] net: dsa: Propagate VLAN add/del to CPU port(s)

[Vivien Didelot]

Hi Florian,

Open question: will we need to do the same for FDB and MDB objects?

Florian Fainelli <f.faine...@gmail.com> writes:

> Now that the bridge layer can call into switchdev to signal programming
> requests targeting the bridge master device itself, allow the switch
> drivers to implement separate programming of downstream and
> upstream/management ports.
>
> Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>
> ---
>  net/dsa/slave.c | 45 +
>  1 file changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index d0c7bce88743..18288261b964 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -223,35 +223,30 @@ static int dsa_slave_set_mac_address(struct net_device 
> *dev, void *a)
>   return 0;
>  }
>  
> -static int dsa_slave_port_vlan_add(struct net_device *dev,
> +static int dsa_slave_port_vlan_add(struct dsa_switch *ds, int port,
>  const struct switchdev_obj_port_vlan *vlan,
>  struct switchdev_trans *trans)
>  {
> - struct dsa_slave_priv *p = netdev_priv(dev);
> - struct dsa_switch *ds = p->parent;
>  

Extra newline ^.

>   if (switchdev_trans_ph_prepare(trans)) {
>   if (!ds->ops->port_vlan_prepare || !ds->ops->port_vlan_add)
>   return -EOPNOTSUPP;
>  
> - return ds->ops->port_vlan_prepare(ds, p->port, vlan, trans);
> + return ds->ops->port_vlan_prepare(ds, port, vlan, trans);
>   }
>  
> - ds->ops->port_vlan_add(ds, p->port, vlan, trans);
> + ds->ops->port_vlan_add(ds, port, vlan, trans);
>  
>   return 0;
>  }
>  
> -static int dsa_slave_port_vlan_del(struct net_device *dev,
> +static int dsa_slave_port_vlan_del(struct dsa_switch *ds, int port,
>  const struct switchdev_obj_port_vlan *vlan)
>  {
> - struct dsa_slave_priv *p = netdev_priv(dev);
> - struct dsa_switch *ds = p->parent;
> -
>   if (!ds->ops->port_vlan_del)
>   return -EOPNOTSUPP;
>  
> - return ds->ops->port_vlan_del(ds, p->port, vlan);
> + return ds->ops->port_vlan_del(ds, port, vlan);
>  }
>  
>  static int dsa_slave_port_vlan_dump(struct net_device *dev,
> @@ -465,8 +460,21 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
> const struct switchdev_obj *obj,
> struct switchdev_trans *trans)
>  {
> + struct dsa_slave_priv *p = netdev_priv(dev);
> + struct dsa_switch *ds = p->parent;
> + int port = p->port;
>   int err;
>  
> + /* Here we may be called with an orig_dev which is different from dev,
> +  * on purpose, to receive request coming from e.g the bridge master
> +  * device. Although there are no network device associated with CPU/DSA
> +  * ports, we may still have programming operation for these ports.
> +  */
> + if (obj->orig_dev == p->bridge_dev) {
> + ds = ds->dst->ds[0];
> + port = ds->dst->cpu_port;
> + }
> +
>   /* For the prepare phase, ensure the full set of changes is feasable in
>* one go in order to signal a failure properly. If an operation is not
>* supported, return -EOPNOTSUPP.
> @@ -483,7 +491,7 @@ static int dsa_slave_port_obj_add(struct net_device *dev,
>trans);
>   break;
>   case SWITCHDEV_OBJ_ID_PORT_VLAN:
> - err = dsa_slave_port_vlan_add(dev,
> + err = dsa_slave_port_vlan_add(ds, port,
> SWITCHDEV_OBJ_PORT_VLAN(obj),
> trans);

Note that dsa_slave_port_vlan_add() will be called N times, N being the
number of bridge ports. This is not an issue for the moment though.
Programming it only once requires caching, so leave it for an eventual
future patch.

When issuing the following command (lan0 being a member of br0):

# bridge vlan add vid 42 dev lan0

the CPU port is also programmed as tagged in VLAN 42. Is that expected?

Thanks,

Vivien

Re: [Bridge] [RFC net-next 1/3] net: bridge: Allow bridge master device to configure switch CPU port

[Vivien Didelot]

Hi Florian,

Florian Fainelli  writes:

> bridge vlan add vid 2 dev br0 self
>   -> CPU port gets programmed
> bridge vlan add vid 2 dev port0
>   -> port0 (switch port 0) gets programmed

Although this is not specific to this patch, I'd like to point out that
this seems not to be the behavior bridge expects.

The bridge manpage says:

bridge vlan add - add a new vlan filter entry
...

   self   the vlan is configured on the specified physical device.
  Required if the device is the bridge device.

   master the vlan is configured on the software bridge (default).

So if I'm not mistaken, the switch chip must be programmed only when the
bridge command is called with the "self" attribute. Without it, only
software configuration must be made, like what happens when the driver
returns -EOPNOTSUPP.

Currently, both commands below program the hardware:

# bridge vlan add vid 2 dev port0 [master]
# bridge vlan add vid 2 dev port0 [master] self

Jiri, what do you think? Is there a reason for switchdev not to be
consistent with the bridge doc, or should this be fixed?

Thanks,

Vivien

Re: [Bridge] [RFC net-next 0/3] net: bridge: Allow CPU port configuration

[Vivien Didelot]

Hi Florian,

Florian Fainelli  writes:

> This patch series allows using the bridge master interface to configure
> an Ethernet switch port's CPU/management port with different VLAN attributes 
> than
> those of the bridge downstream ports/members.
>
> Jiri, Ido, Andrew, Vivien, please review the impact on mlxsw and mv88e6xxx, I
> tested this with b53 and a mockup DSA driver.

Patchset looks fine to me overall. I'm cooking a patch similar to 3/3
for mv88e6xxx to put on top of this patchset.

Minor comments in individual patchs will follow.

> Open questions:
>
> - if we have more than one bridge on top of a physical switch, the driver
>   should keep track of that and verify that we are not going to change
>   the CPU port VLAN attributes in a way that results in incompatible settings
>   to be applied

In mv88e6xxx, mv88e6xxx_port_check_hw_vlan() does that. It needs a small
adjustment though.

> - if the default behavior is to have all VLANs associated with the CPU port
>   be ingressing/egressing tagged to the CPU, is this really useful?

I have no strong opinion on this. Intuitively I'd expect the CPU port to
be excluded until I add it myself, but I didn't think much about it.

Thanks,

Vivien

[Bridge] [RFC net 3/3] net: dsa: shorten ageing time on topology change

[Vivien Didelot]

802.1D [1] specifies that the bridges must use a short value to age out
dynamic entries in the Filtering Database for a period, once a topology
change has been communicated by the root bridge.

Add a bridge_ageing_time member in the net_bridge structure to store the
bridge ageing time value configured by the user (ioctl/netlink/sysfs).

If we are using in-kernel STP, shorten the ageing time value to twice
the forward delay used by the topology when the topology change flag is
set. When the flag is cleared, restore the configured ageing time.

[1] "8.3.5 Notifying topology changes ",
http://profesores.elo.utfsm.cl/~agv/elo309/doc/802.1D-1998.pdf

Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 net/bridge/br_device.c  |  2 +-
 net/bridge/br_private.h |  3 ++-
 net/bridge/br_stp.c | 27 +++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 89a687f..207318a 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -409,7 +409,7 @@ void br_dev_setup(struct net_device *dev)
br->bridge_max_age = br->max_age = 20 * HZ;
br->bridge_hello_time = br->hello_time = 2 * HZ;
br->bridge_forward_delay = br->forward_delay = 15 * HZ;
-   br->ageing_time = BR_DEFAULT_AGEING_TIME;
+   br->bridge_ageing_time = br->ageing_time = BR_DEFAULT_AGEING_TIME;
 
br_netfilter_rtable_init(br);
br_stp_timer_init(br);
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 3c294b4..43efeb9 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -300,10 +300,11 @@ struct net_bridge
unsigned long   max_age;
unsigned long   hello_time;
unsigned long   forward_delay;
-   unsigned long   bridge_max_age;
unsigned long   ageing_time;
+   unsigned long   bridge_max_age;
unsigned long   bridge_hello_time;
unsigned long   bridge_forward_delay;
+   unsigned long   bridge_ageing_time;
 
u8  group_addr[ETH_ALEN];
boolgroup_addr_set;
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 8d7b4c7..71fd1a4 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -597,7 +597,11 @@ int br_set_ageing_time(struct net_bridge *br, clock_t 
ageing_time)
if (err)
return err;
 
+   spin_lock_bh(>lock);
+   br->bridge_ageing_time = t;
br->ageing_time = t;
+   spin_unlock_bh(>lock);
+
mod_timer(>gc_timer, jiffies);
 
return 0;
@@ -606,6 +610,29 @@ int br_set_ageing_time(struct net_bridge *br, clock_t 
ageing_time)
 /* called under bridge lock */
 void __br_set_topology_change(struct net_bridge *br, unsigned char val)
 {
+   unsigned long t;
+   int err;
+
+   if (br->stp_enabled == BR_KERNEL_STP && br->topology_change != val) {
+   /* On topology change, set the bridge ageing time to twice the
+* forward delay. Otherwise, restore its default ageing time.
+*/
+
+   if (val) {
+   t = 2 * br->forward_delay;
+   br_debug(br, "decreasing ageing time to %lu\n", t);
+   } else {
+   t = br->bridge_ageing_time;
+   br_debug(br, "restoring ageing time to %lu\n", t);
+   }
+
+   err = __set_ageing_time(br->dev, t);
+   if (err)
+   br_warn(br, "error offloading ageing time\n");
+   else
+   br->ageing_time = t;
+   }
+
br->topology_change = val;
 }
 
-- 
2.10.0

[Bridge] [RFC net 1/3] net: bridge: add helper to offload ageing time

[Vivien Didelot]

The SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME switchdev attr is actually set
when initializing a bridge port, and when configuring the bridge ageing
time from ioctl/netlink/sysfs.

Add a __set_ageing_time helper to offload the ageing time to physical
switches, and add the SWITCHDEV_F_DEFER flag since it can be called
under bridge lock.

Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 net/bridge/br_private.h |  1 +
 net/bridge/br_stp.c | 28 
 net/bridge/br_stp_if.c  | 12 +++-
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1b63177..3c294b4 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -992,6 +992,7 @@ void __br_set_forward_delay(struct net_bridge *br, unsigned 
long t);
 int br_set_forward_delay(struct net_bridge *br, unsigned long x);
 int br_set_hello_time(struct net_bridge *br, unsigned long x);
 int br_set_max_age(struct net_bridge *br, unsigned long x);
+int __set_ageing_time(struct net_device *dev, unsigned long t);
 int br_set_ageing_time(struct net_bridge *br, clock_t ageing_time);
 
 
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 9258b8e..6ebe2a0 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -562,6 +562,24 @@ int br_set_max_age(struct net_bridge *br, unsigned long 
val)
 
 }
 
+/* called under bridge lock */
+int __set_ageing_time(struct net_device *dev, unsigned long t)
+{
+   struct switchdev_attr attr = {
+   .orig_dev = dev,
+   .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
+   .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP | SWITCHDEV_F_DEFER,
+   .u.ageing_time = jiffies_to_clock_t(t),
+   };
+   int err;
+
+   err = switchdev_port_attr_set(dev, );
+   if (err && err != -EOPNOTSUPP)
+   return err;
+
+   return 0;
+}
+
 /* Set time interval that dynamic forwarding entries live
  * For pure software bridge, allow values outside the 802.1
  * standard specification for special cases:
@@ -572,17 +590,11 @@ int br_set_max_age(struct net_bridge *br, unsigned long 
val)
  */
 int br_set_ageing_time(struct net_bridge *br, clock_t ageing_time)
 {
-   struct switchdev_attr attr = {
-   .orig_dev = br->dev,
-   .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
-   .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP,
-   .u.ageing_time = ageing_time,
-   };
unsigned long t = clock_t_to_jiffies(ageing_time);
int err;
 
-   err = switchdev_port_attr_set(br->dev, );
-   if (err && err != -EOPNOTSUPP)
+   err = __set_ageing_time(br->dev, t);
+   if (err)
return err;
 
br->ageing_time = t;
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index d8ad73b..2efbba5 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -36,12 +36,6 @@ static inline port_id br_make_port_id(__u8 priority, __u16 
port_no)
 /* called under bridge lock */
 void br_init_port(struct net_bridge_port *p)
 {
-   struct switchdev_attr attr = {
-   .orig_dev = p->dev,
-   .id = SWITCHDEV_ATTR_ID_BRIDGE_AGEING_TIME,
-   .flags = SWITCHDEV_F_SKIP_EOPNOTSUPP | SWITCHDEV_F_DEFER,
-   .u.ageing_time = jiffies_to_clock_t(p->br->ageing_time),
-   };
int err;
 
p->port_id = br_make_port_id(p->priority, p->port_no);
@@ -50,9 +44,9 @@ void br_init_port(struct net_bridge_port *p)
p->topology_change_ack = 0;
p->config_pending = 0;
 
-   err = switchdev_port_attr_set(p->dev, );
-   if (err && err != -EOPNOTSUPP)
-   netdev_err(p->dev, "failed to set HW ageing time\n");
+   err = __set_ageing_time(p->dev, p->br->ageing_time);
+   if (err)
+   netdev_err(p->dev, "failed to offload ageing time\n");
 }
 
 /* NO locks held */
-- 
2.10.0

[Bridge] [RFC net 2/3] net: bridge: add helper to set topology change

[Vivien Didelot]

Add a __br_set_topology_change helper to set the topology change value.

This can be later extended to add actions when the topology change flag
is set or cleared.

Signed-off-by: Vivien Didelot <vivien.dide...@savoirfairelinux.com>
---
 net/bridge/br_private_stp.h |  1 +
 net/bridge/br_stp.c | 10 --
 net/bridge/br_stp_if.c  |  2 +-
 net/bridge/br_stp_timer.c   |  2 +-
 4 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/bridge/br_private_stp.h b/net/bridge/br_private_stp.h
index 2fe910c..3f7543a 100644
--- a/net/bridge/br_private_stp.h
+++ b/net/bridge/br_private_stp.h
@@ -61,6 +61,7 @@ void br_received_tcn_bpdu(struct net_bridge_port *p);
 void br_transmit_config(struct net_bridge_port *p);
 void br_transmit_tcn(struct net_bridge *br);
 void br_topology_change_detection(struct net_bridge *br);
+void __br_set_topology_change(struct net_bridge *br, unsigned char val);
 
 /* br_stp_bpdu.c */
 void br_send_config_bpdu(struct net_bridge_port *, struct br_config_bpdu *);
diff --git a/net/bridge/br_stp.c b/net/bridge/br_stp.c
index 6ebe2a0..8d7b4c7 100644
--- a/net/bridge/br_stp.c
+++ b/net/bridge/br_stp.c
@@ -234,7 +234,7 @@ static void br_record_config_timeout_values(struct 
net_bridge *br,
br->max_age = bpdu->max_age;
br->hello_time = bpdu->hello_time;
br->forward_delay = bpdu->forward_delay;
-   br->topology_change = bpdu->topology_change;
+   __br_set_topology_change(br, bpdu->topology_change);
 }
 
 /* called under bridge lock */
@@ -344,7 +344,7 @@ void br_topology_change_detection(struct net_bridge *br)
isroot ? "propagating" : "sending tcn bpdu");
 
if (isroot) {
-   br->topology_change = 1;
+   __br_set_topology_change(br, 1);
mod_timer(>topology_change_timer, jiffies
  + br->bridge_forward_delay + br->bridge_max_age);
} else if (!br->topology_change_detected) {
@@ -603,6 +603,12 @@ int br_set_ageing_time(struct net_bridge *br, clock_t 
ageing_time)
return 0;
 }
 
+/* called under bridge lock */
+void __br_set_topology_change(struct net_bridge *br, unsigned char val)
+{
+   br->topology_change = val;
+}
+
 void __br_set_forward_delay(struct net_bridge *br, unsigned long t)
 {
br->bridge_forward_delay = t;
diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
index 2efbba5..6c1e214 100644
--- a/net/bridge/br_stp_if.c
+++ b/net/bridge/br_stp_if.c
@@ -81,7 +81,7 @@ void br_stp_disable_bridge(struct net_bridge *br)
 
}
 
-   br->topology_change = 0;
+   __br_set_topology_change(br, 0);
br->topology_change_detected = 0;
spin_unlock_bh(>lock);
 
diff --git a/net/bridge/br_stp_timer.c b/net/bridge/br_stp_timer.c
index da058b8..7ddb38e 100644
--- a/net/bridge/br_stp_timer.c
+++ b/net/bridge/br_stp_timer.c
@@ -125,7 +125,7 @@ static void br_topology_change_timer_expired(unsigned long 
arg)
br_debug(br, "topo change timer expired\n");
spin_lock(>lock);
br->topology_change_detected = 0;
-   br->topology_change = 0;
+   __br_set_topology_change(br, 0);
spin_unlock(>lock);
 }
 
-- 
2.10.0

[Bridge] [RFC net 0/3] net: bridge: fast ageing on topology change

[Vivien Didelot]

802.1D [1] specifies that the bridges in a network must use a short
value to age out dynamic entries in the Filtering Database for a period,
once a topology change has been communicated by the root bridge.

This patchset fixes this for the in-kernel STP implementation.

Once the topology change flag is set in a net_bridge instance, the
ageing time value is shorten to twice the forward delay used by the
topology.

When the topology change flag is cleared, the ageing time configured for
the bridge is restored.

To accomplish that, a new bridge_ageing_time member is added to the
net_bridge structure, to store the user configured bridge ageing time.

Two helpers are added to offload the ageing time and set the topology
change flag in the net_bridge instance. Then the required logic is added
in the topology change helper if in-kernel STP is used.

This has been tested on the following topology:

+--+
| root bridge  |
|  1  2  3  4  |
+--+--+--+--+--+
   |  |  |  |  ++
   |  |  |  +--| laptop |
   |  |  | ++
+--+--+--+-+
|  1  2  3 |
| slave bridge |
+--+

When unplugging/replugging the laptop, the slave bridge (under test)
gets the topology change flag sent by the root bridge, and fast ageing
is triggered on the bridges. Once the topology change timer of the root
bridge expires, the topology change flag is cleared and the configured
ageing time is restored on the bridges.

A similar test has been done between two bridges under test.
When changing the forward delay of the root bridge with:

# echo 3000 > /sys/class/net/br0/bridge/forward_delay

the ageing time correctly changes on both bridges from 300s to 60s while
the TOPOLOGY_CHANGE flag is present.

[1] "8.3.5 Notifying topology changes",
http://profesores.elo.utfsm.cl/~agv/elo309/doc/802.1D-1998.pdf

[ Feedbacks are needed, especially for the usage of the bridge lock and
the defered ageing time attribute. It works fine so far but might raise
concerns. ]

Vivien Didelot (3):
  net: bridge: add helper to offload ageing time
  net: bridge: add helper to set topology change
  net: dsa: shorten ageing time on topology change

 net/bridge/br_device.c  |  2 +-
 net/bridge/br_private.h |  4 ++-
 net/bridge/br_private_stp.h |  1 +
 net/bridge/br_stp.c | 65 ++---
 net/bridge/br_stp_if.c  | 14 +++---
 net/bridge/br_stp_timer.c   |  2 +-
 6 files changed, 65 insertions(+), 23 deletions(-)

-- 
2.10.0

Re: [Bridge] [PATCH net-next 4/4] bridge: vlan: combine (br|nbp)_vlan_flush into one

[Vivien Didelot]

On Oct. Monday 12 (42) 09:27 PM, Ido Schimmel wrote:
> Mon, Oct 12, 2015 at 09:15:39PM IDT, vivien.dide...@savoirfairelinux.com 
> wrote:
> >Hi,
> >
> >On Oct. Monday 12 (42) 08:51 PM, Ido Schimmel wrote:
> >> Mon, Oct 12, 2015 at 02:41:09PM IDT, ra...@blackwall.org wrote:
> >> >From: Nikolay Aleksandrov 
> >> >
> >> >As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
> >> >unnecessary (and is actually a remnant of the old vlan code) so we can
> >> >remove it and combine both br/nbp vlan_flush functions into one.
> >> Just a small note to Scott and Vivien:
> >> 
> >> One of the side effects of Nik's recent patchsets is that when VLANs are
> >> flushed on a port the deletion is propagated to the driver via
> >> switchdev ops, as __vlan_vid_del is called.
> >> 
> >> Therefore there is no need to do internal bookkeeping and remove VLANs
> >> yourself when port is removed from bridge.
> >
> >I was thinking about caching VLAN entries in the mv88e6xxx driver to
> >improve look up on VLAN and FDB operations, but it's a bit prematurate.
> >
> >But when VLAN are flushed, we still need to remove them from the
> >hardware table, right?
> Hi,
> 
> Not sure I'm following. You'll simply get a SWITCHDEV_OBJ_ID_PORT_VLAN
> (del) for each VLAN configured on the port you just removed from the bridge.
> I guess you remove them from your hardware table in the implementation
> of these ops?

Yes we do remove VLAN entries from the hardware table when
switchdev_port_obj_del(SWITCHDEV_OBJ_ID_PORT_VLAN) is called.

I may have misunderstood your previous note, I thought we were talking
about implementing the flush operation in switchdev drivers.

> >
> >Flushing is interesting though, most hardware have flush operations and
> >it would be interesting to have switchdev fdb_flush and vlan_flush ops.

Thanks,
-v

Re: [Bridge] [PATCH net-next 4/4] bridge: vlan: combine (br|nbp)_vlan_flush into one

[Vivien Didelot]

Hi,

On Oct. Monday 12 (42) 08:51 PM, Ido Schimmel wrote:
> Mon, Oct 12, 2015 at 02:41:09PM IDT, ra...@blackwall.org wrote:
> >From: Nikolay Aleksandrov 
> >
> >As Ido Schimmel pointed out the vlan_vid_del() loop in nbp_vlan_flush is
> >unnecessary (and is actually a remnant of the old vlan code) so we can
> >remove it and combine both br/nbp vlan_flush functions into one.
> Just a small note to Scott and Vivien:
> 
> One of the side effects of Nik's recent patchsets is that when VLANs are
> flushed on a port the deletion is propagated to the driver via
> switchdev ops, as __vlan_vid_del is called.
> 
> Therefore there is no need to do internal bookkeeping and remove VLANs
> yourself when port is removed from bridge.

I was thinking about caching VLAN entries in the mv88e6xxx driver to
improve look up on VLAN and FDB operations, but it's a bit prematurate.

But when VLAN are flushed, we still need to remove them from the
hardware table, right?

Flushing is interesting though, most hardware have flush operations and
it would be interesting to have switchdev fdb_flush and vlan_flush ops.

Thanks!
-v