| Message ID | 20191210212050.1470909-1-vivien.didelot@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v2] net: bridge: add STP xstats | expand |
On 10/12/2019 23:20, Vivien Didelot wrote: > 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 <vivien.didelot@gmail.com> > --- > 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(+) > Hi, I like it! Unfortunately there is one issue still, more below. > 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), &p->stp_xstats, sizeof(p->stp_xstats)); You need to take the STP lock here to get a proper snapshot of the values. > + } > + > #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_xstats stp_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++; > } > > /* >
Hi Nikolay, On Tue, 10 Dec 2019 23:45:13 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> 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), &p->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(&br->lock); memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats)); spin_unlock_bh(&br->lock); Thanks, Vivien
On 11/12/2019 04:02, Vivien Didelot wrote: > Hi Nikolay, > > On Tue, 10 Dec 2019 23:45:13 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> 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), &p->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(&br->lock); > memcpy(nla_data(nla), &p->stp_xstats, sizeof(p->stp_xstats)); > spin_unlock_bh(&br->lock); > Yeah, this is a very old lock (pre-git) which needs some attention. :) That is the lock and the above looks good to me. > > Thanks, > > Vivien > Cheers, Nik
On 12/10/19 2:20 PM, Vivien Didelot wrote: > 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 > }; Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD
On 11/12/2019 17:38, David Ahern wrote: > On 12/10/19 2:20 PM, Vivien Didelot wrote: >> 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 >> }; > > Shouldn't the new entry be appended to the end - after BRIDGE_XSTATS_PAD > Oh yes, good catch. That has to be fixed, too.
Hi David, Nikolay, On Wed, 11 Dec 2019 17:42:33 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> 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
From: Vivien Didelot <vivien.didelot@gmail.com> Date: Wed, 11 Dec 2019 13:41:33 -0500 > Hi David, Nikolay, > > On Wed, 11 Dec 2019 17:42:33 +0200, Nikolay Aleksandrov <nikolay@cumulusnetworks.com> 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.
Hi David, On Wed, 11 Dec 2019 12:01:20 -0800 (PST), David Miller <davem@davemloft.net> 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
From: Vivien Didelot <vivien.didelot@gmail.com> Date: Wed, 11 Dec 2019 16:47:54 -0500 > I thought the whole point of using enums was to avoid caring about fixed > numeric values, but well. I don't get where you got that idea from. Each and every netlink attribute value is like IPPROTO_TCP in an ipv4 header, the exact values matter, and therefore you cannot make changes that modify existing values. Therefore, you must add new attributes to the end of the enumberation, right before the __MAX value. > 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?
Hi David, On Wed, 11 Dec 2019 14:16:58 -0800 (PST), David Miller <davem@davemloft.net> 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
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), &p->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_xstats stp_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++; } /*
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 <vivien.didelot@gmail.com> --- 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(+)